Fix file globbing with multiple dots (#784) #1343
Conversation
Setting the "data-files" field in a .cabal file to pull in "resources/*.js" would previously not work for any file containing more than one dot, for example "resources/jquery-1.9.1.js". This commit changes this behaviour so that a glob counts as a match, provided that its extension is a suffix of the filename. So, if resources/ contains foo.js and foo.bar.js: * "resources/*.js" will match both "resources/foo.js" and "resources/foo.bar.js" * "resources/*.bar.js" will match "resources/foo.bar.js" but not "resources/foo.js" It should be noted that I've only tested this by calling matchFileDirGlob in ghci -- I'm fairly new to Cabal, and can't see how to test it properly.
|
refs #784 |
|
Looks good. @dcoutts, can we enable this for all packages or do you want to add a |
|
Ok, so originally this design was deliberate, and similarly the lack of support for recursive directory globs (the goal was to avoid accidentally including things). But it seems like it's just too annoying so I'm happy to relent on both points. As for whether it needs a backwards compat thing, that's a bit tricky. In principle we could check against existing packages to see if the meaning for any of them would change. If we can't be bothered to do that (and fair enough!) then the safe thing to do would be for the different behaviour to be conditional on the |
|
@dcoutts I think this is blocked on you. |
|
@dcoutts Can you please take a look? |
|
Is this subsumed by #1344 or are they orthogonal? |
|
Ok so in principle I'm fine with this so long as we can handle backwards compat properly. The concern is that this does change the behaviour of existing syntax so that it includes more than it did before. Though at least it's only changing at the point where you do sdist, not for builds of existing packages. So perhaps the cases where it will include things unexpectedly is so small that we can ignore it? I think I'm inclined to let it go without having the behaviour be dependent on the version in the .cabal file. What do other people think? |
|
If you're really concerned, you could issue a warning to stderr if the globs include more than they would have in previous versions? (Subject to a cabal version check, of course.) (I personally don't think it's necessary, but sometimes paranoia pays off in SW development.) EDIT: Of course an enthusiastic +1 from me, either way! :) |
|
Is there a disadvantage to requiring a minimum Also might it be better to continue this in #1975? |
|
@hdgarrood requiring a Cabal-version constraint seems right the right thing to do if older Cabal versions will do the wrong thing interpreting the .cabal file with the new feature/syntax. |
|
Hmm, this is still open but the later one #1975 was closed. I don't want to loose track of this improved globbing, it'd be useful. @hdgarrood any status update? If you don't have time anymore is this something you could hand over to someone else? |
|
Sorry, I don't quite remember what happened here, but I don't think I have time to pick this up at the moment. I'd be more than happy to hand over. |
|
Closing in favour of #2522 |
Setting the "data-files" field in a .cabal file to pull in
"resources/*.js" would previously not work for any file containing more
than one dot, for example "resources/jquery-1.9.1.js".
This commit changes this behaviour so that a glob counts as a match,
provided that its extension is a suffix of the filename. So, if
resources/ contains foo.js and foo.bar.js:
"resources/foo.bar.js"
"resources/foo.js"
It should be noted that I've only tested this by calling
matchFileDirGlob in ghci -- I'm fairly new to Cabal, and can't see how
to test it properly.