glob expand extra-src-files when tracking for rebuild #7608
Conversation
| @@ -138,13 +142,14 @@ needBuildInfo pkg_descr bi modules = do | |||
| -- A.hs-boot; need to track both. | |||
| findNeededModules ["hs", "lhs", "hsig", "lhsig"] | |||
| findNeededModules ["hs-boot", "lhs-boot"] | |||
| expandedExtraSrcFiles <- liftIO $ fmap concat . for (extraSrcFiles pkg_descr) $ \fpath -> matchDirFileGlobWithDie silent (\ _ _ -> return []) (specVersion pkg_descr) "." fpath | |||
There was a problem hiding this comment.
Are we sure we don't want to die with an error message? Is there any risk of false positives that would justify that?
There was a problem hiding this comment.
in this setting where we're just tracking setting stuff, dying seems weird.
The only other place this gets glob expanded is in the sdist step, where dying makes sense, because you're sdisting something with missing files. But here we're just checking if we need to trigger a reconfigure...
There was a problem hiding this comment.
Still, if the user thinks the compiler rebuilds his stuff, but in fact it's not, because the glob is incorrect, we should die ASAP, right? Unless false positives are possible. E.g., the user writes too general globs predicting some files/directories that are not all there yet [edit: and so it dies, even though it won't once the files are in]. Is that or any other false positive possible and likely?
There was a problem hiding this comment.
The only error would be if the glob matches nothing. I'm extremely chary of having any sort of death or error reporting on a purely administrative step. I don't think there's anywhere else inside the Rebuild monad that we die, so it seems like it would be a significant change from how things now occur. A user will notice if a rebuild happens or not. The behavior no rebuild happening is not substantially different than if they put nothing at all in their extra-src-files, or put in something that matches but doesn't match everything they intended, etc.
That it so say, we're not in a good position here to catch if this is correct. The only way to catch if it is correct is to see if the tarball generated by sdist actually builds.
There was a problem hiding this comment.
Good point about sdist tarball building. Let it be so.
Regarding possible errors, I thought it can fail in a variety of ways, both due to a syntax error in the glob and due to misssing directories, etc., as in https://github.com/haskell/cabal/blob/master/Cabal/src/Distribution/Simple/Glob.hs#L243-L250
There was a problem hiding this comment.
thanks for the clarification on possible errors, I'm not particularly familiar with the glob code.
There was a problem hiding this comment.
On a second thought, I hope syntax errors in globs are caught already when .cabal file is parsed (it definitely is at cabal check, but that would not be enough). So, no point preparing to handle these errors again.
|
added |
|
@fgaz: ping :) |
fgaz
added
squash+merge me
|
@Mergifyio backport 3.6 |
🟠 Waiting for conditions
|
|
Hmm, I think this needs a rebase for CI to start @Mergifyio rebase |
✅ Branch has been successfully rebased |
byorgey
force-pushed
the
gb/glob-extra-src-tracking
branch
from
a72bbd9 to
6644c24
Compare
* glob expand extra-src-files when tracking for rebuild * changelog Co-authored-by: Gershom Bazerman <gershom@arista.com> (cherry picked from commit 751308c)
✅ Backports have been created
|
glob expand extra-src-files when tracking for rebuild (backport #7608)
Fixes partially the issue discussed in #4746 by expanding globs when tracking extra-src-files