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

Improve file globbing logic (fixes #5057) #5061

Closed
wants to merge 2 commits into from
Closed

Improve file globbing logic (fixes #5057) #5061

wants to merge 2 commits into from

Conversation

mitchellwrosen
Copy link

I tested this change manually by running cabal sdist on https://github.com/RyanGlScott/cabal-sdist-bug/tree/910078306b4e127996bd7c9208a7e741b9f74351:

$ cabal sdist
Building source dist for cabal-sdist-bug-0.1.0...
Source tarball created: dist/cabal-sdist-bug-0.1.0.tar.gz

@hvr
Copy link
Member

hvr commented Jan 21, 2018

This likely also needs a cabal check, as old cabals will choke.

Copy link
Member

@hvr hvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • needs tests (& make sure that the documented limitations for * in filenames remain in place).
  • possibly a cabal check compat-warning
  • changelog
  • possibly user guide update (to document that starting with version x.y, fileglobbing supports multi-part extensions)

see also #5057 (comment)

@mitchellwrosen
Copy link
Author

Sorry, I'm unclear on which came first, these TODOs or your comment about how the current behavior was actually intended. Should I close this PR?

@23Skidoo
Copy link
Member

That behaviour was intended originally, but there seems to be a consensus that we should relax it a bit.

@mitchellwrosen
Copy link
Author

I've added a test and updated changelog. Could I have some guidance on these remaining TODOs?

(& make sure that the documented limitations for * in filenames remain in place).

What are the documented limitations for * in filenames?

possibly a cabal check compat-warning

What does this mean?

possibly user guide update (to document that starting with version x.y, fileglobbing supports multi-part extensions)

Where is the user guide located?

Thanks!

@phadej
Copy link
Collaborator

phadej commented Jan 26, 2018

I'm really sorry that we (me and @hvr) are demanding. I try to explain:


possibly a cabal check compat-warning`

run cabal check. The implementation of that is in Distribution.PackageDescription.Check.
There is e.g. check use of "data-files: data/*.txt" syntax.

The issue is that:

  • the change is good for ergonomics
  • but if someone else developing the package would use older cabal-install, their cabal sdist won't pick all the files, resulting in broken package.
    • unfortunately there isn't cabal check which checks that globs actually pick at least some files. There should be.
    • new version should warn, that 'extended globbing' is needed, and cabal-version is too low.
      • that's tricky, but doable

The scenario is not that different to #5073:
if some CI setup (for any reason) uses older cabal-install for some job, than cabal sdist will produce broken tarballs, resulting in confusingly failing builds.

Where is the user guide located?

Cabal/doc

quasicomputational added a commit to quasicomputational/cabal that referenced this pull request Jun 10, 2018
This has the effect of allowing a glob `*.html` to match the file
`foo.en.html`. For compatibility, this is only allowed with
`cabal-version: 3.0` or later; for earlier spec versions, a warning
will be generated by `cabal check` if there are files affected by this
change in behaviour.

Fixes haskell#5057. Fixes haskell#784. Closes haskell#5061.
quasicomputational added a commit to quasicomputational/cabal that referenced this pull request Jun 11, 2018
This has the effect of allowing a glob `*.html` to match the file
`foo.en.html`. For compatibility, this is only allowed with
`cabal-version: 3.0` or later; for earlier spec versions, a warning
will be generated by `cabal check` if there are files affected by this
change in behaviour.

Fixes haskell#5057. Fixes haskell#784. Closes haskell#5061.
23Skidoo pushed a commit to quasicomputational/cabal that referenced this pull request Jun 13, 2018
This has the effect of allowing a glob `*.html` to match the file
`foo.en.html`. For compatibility, this is only allowed with
`cabal-version: 3.0` or later; for earlier spec versions, a warning
will be generated by `cabal check` if there are files affected by this
change in behaviour.

Fixes haskell#5057. Fixes haskell#784. Closes haskell#5061.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants