Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New extra-files field for neutral sdist files #10107

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

tbidne
Copy link
Contributor

@tbidne tbidne commented Jun 12, 2024

Resolves #8817.

This PR adds a new extra-files field that provides a way to specify extra files that should be included in sdist, without adding any other semantics (cf. extra-source-files are tracked by cabal build).

Field name bikeshedding

I decided to go with extra-files. Alternatives considered:

  • Structured e.g. from the issue:

    extra-files:
      source:
        config.mk  --whatever, I never use this
        ...
      doc:
        ...
      other: -- this would be the new field
        ...

    I actually like this idea in principle, but of course we have to consider how this fits into the existing spec. I cannot imagine anyone has the appetite to replace the current top-level extra-*-files, and I don't think adding an alternative here makes sense just for adding a "neutral option".

  • extra-sdist-files:

    I think this is the best sounding name that fits the extra-<something>-files format. The only downside is if there is a future possibility that such neutral extra files would ever be wanted for something other than sdist. It would be a shame if the answer to a question like

    How do I add extra files for X?

    Had an answer of

    Add them to extra-sdist-files.

    Thus my preference for something more general. Though maybe that future scenario would likely suggest a new extra-?-files option, so perhaps this fear isn't warranted.

  • extra-other-files, extra-misc-files, etc.: I can't really come up with a great alternative here. other fits well with the structured approach above, but extra-other-files sounds weird, imo. I'm ambivalent on extra-misc-files.

Hence I went with extra-files. But I am pretty undecided on this, so please do offer suggestions.

Other notes

  • After writing the test in the cabal-testsuite/PackageTests/SDist directory, I noticed that there is also a cabal-testsuite/PackageTests/NewSdist dir. Just checking that it's in the right place.

  • I added an entry in file-format-changelog.rst, though I'm unsure of the next cabal version (I presume this is not going in 3.12). Maybe 3.14?

  • Probably there are other documentation changes needed (e.g. wiki), though I don't know exactly where those should be.

Thanks!


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

@andreabedini
Copy link
Collaborator

Thanks! This requires a bump in cabal-version, also it needs to be discussed.

@tbidne
Copy link
Contributor Author

tbidne commented Jun 21, 2024

I was wondering about that (3.14?). And indeed, happy to discuss 🙂 .

@geekosaur
Copy link
Collaborator

As noted by CI's tests, this changes the hashes of GenericPackageDescription and LocalBuildInfo, which will need to be updated.

@tbidne tbidne force-pushed the extra-files branch 3 times, most recently from 462cd0d to 9318243 Compare July 3, 2024 07:46
@tbidne
Copy link
Contributor Author

tbidne commented Jul 3, 2024

Made the version change and updated the hashes (I got these locally from the validate script. I assume that or CI is fine).

Note that the validate script is failing:

rejecting: cabal-testsuite:setup.Cabal-3.13.0.0 (conflict: cabal-testsuite => cabal-testsuite:setup.Cabal>=3.10 && <3.11)

My guess is this is due to CI choosing the new cabal 3.12, and something needs to be updated (e.g. version bound, index-state).

@ulysses4ever
Copy link
Collaborator

@tbidne sorry about that. #10172 is supposed to fix it.

@ulysses4ever
Copy link
Collaborator

We need to decide on this. I'm not looking forward for bike-shedding the name and am happy with the current state.

@tbidne tbidne force-pushed the extra-files branch 2 times, most recently from 7444ed5 to 4c90694 Compare July 31, 2024 02:43
@Mikolaj
Copy link
Member

Mikolaj commented Jul 31, 2024

Yes, extra-files seems good enough and we need it now. Thanks for the PR!

@tbidne
Copy link
Contributor Author

tbidne commented Jul 31, 2024

I am satisfied with the PR, so unless there is anything else, my understanding is that someone with the right permissions adds the merge me label.

@geekosaur geekosaur added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Jul 31, 2024
@ulysses4ever
Copy link
Collaborator

Done. It will take several days for the bot to merge it.

@geekosaur
Copy link
Collaborator

Oh right, no notification from label changes ☹️

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 2, 2024
The new `extra-files` field provides a way to specify extra files that
should be included in sdist, without adding any other semantics
(cf. `extra-source-files` are tracked by `cabal build`).

Resolves haskell#8817.
@mergify mergify bot merged commit e931ca1 into haskell:master Aug 2, 2024
44 checks passed
@tbidne tbidne deleted the extra-files branch August 2, 2024 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cabal: file format merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge re: extra-doc-files re: extra-source-files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Neutral field (like extra-files or extra-sdist-files) to add files to sdist
5 participants