cabal icon indicating copy to clipboard operation
cabal copied to clipboard

New `extra-files` field for neutral `sdist` files

Open tbidne opened this issue 1 year ago • 5 comments

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:

  • [x] Patches conform to the coding conventions.
  • [x] 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.
  • [x] 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!)

tbidne avatar Jun 12 '24 21:06 tbidne

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

andreabedini avatar Jun 17 '24 15:06 andreabedini

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

tbidne avatar Jun 21 '24 23:06 tbidne

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

geekosaur avatar Jun 25 '24 17:06 geekosaur

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).

tbidne avatar Jul 03 '24 09:07 tbidne

@tbidne sorry about that. https://github.com/haskell/cabal/pull/10172 is supposed to fix it.

ulysses4ever avatar Jul 03 '24 20:07 ulysses4ever

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

ulysses4ever avatar Jul 30 '24 23:07 ulysses4ever

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

Mikolaj avatar Jul 31 '24 07:07 Mikolaj

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.

tbidne avatar Jul 31 '24 20:07 tbidne

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

ulysses4ever avatar Jul 31 '24 22:07 ulysses4ever

Oh right, no notification from label changes ☹️

geekosaur avatar Jul 31 '24 22:07 geekosaur