For non-Custom packages, replace sdist with hand-rolled rebuild checking #3985
Conversation
ezyang
force-pushed
the
pr/nix-sdist-replacement
branch
from
f103542 to
64da514
Compare
|
Sorry, I accidentally restarted the build. Appveyor passed the first time: https://ci.appveyor.com/project/23Skidoo/cabal/build/%232330%20(master) |
| let trySdist = allPackageSourceFiles verbosity scriptOptions srcdir | ||
| -- This is just a hack, to get semi-reasonable file | ||
| -- listings for the monitor | ||
| tryFallback = do |
There was a problem hiding this comment.
Our fallback could now be the needElaboratedConfiguredPackage rather than the really dumb getDirectoryContentsRecursive
| ElabPackage epkg -> needElaboratedPackage elab epkg | ||
|
|
||
| needElaboratedPackage :: ElaboratedConfiguredPackage -> ElaboratedPackage -> Rebuild () | ||
| needElaboratedPackage elab epkg = do |
| root <- askRoot | ||
| exists <- liftIO $ doesFileExist (root </> f) | ||
| -- TODO: If the file exists, should we really monitor the entire | ||
| -- file?! |
There was a problem hiding this comment.
In principle we'd take an arg, passed through findFileMonitored etc that says if we care about content or mere existence, and all the new uses of it would use the full content. So I wouldn't bother, but we can update the TODO here to say this.
There was a problem hiding this comment.
I don't understand this comment.
There was a problem hiding this comment.
Well whether we should monitorFileHashed or monitorFile depends on how the caller is going to want to use it. As it happens we always want one, but if we wanted different cases at different call sites then we'd want a param.
There was a problem hiding this comment.
The gist of this comment is different. monitorFile sets up a dependence on the timestamp of the file. But consider a build process which tests if a file exists, and does A if it does and B otherwise. We do NOT need to rebuild if the timestamp on the file updates. But there is no way to express this in the current MonitorFile data structure.
There was a problem hiding this comment.
Ahh! Well, funny you should ask for it. Internally this is supported (and is used for globs), it just wasn't exposed for individual files since we didn't have a use case (and I couldn't think of one).
So just add to the FileMonitor module:
-- | Monitor a single file for existence only. The monitored file is
-- considered to have changed if it no longer exists.
--
monitorFileExistence :: FilePath -> MonitorFilePath
monitorFileExistence = MonitorFile FileExists DirNotExists
| then need "Setup.hs" | ||
| else do | ||
| lhsExists <- doesFileExistMonitored "Setup.lhs" | ||
| when lhsExists $ need "Setup.lhs" |
There was a problem hiding this comment.
Or:
_ <- findFirstFileMonitored id ["Setup.hs", "Setup.lhs"]
return ()
| -- extension. | ||
| Nothing -> findFileMonitored (hsSourceDirs bi) mainPath | ||
| >>= maybe (return ()) need | ||
| Just pp -> need pp |
| -- in extra-src-files changes (most of these won't affect | ||
| -- compilation). It would be even better if we knew on a | ||
| -- per-component basis which headers would be used but that | ||
| -- seems to be too difficult. |
There was a problem hiding this comment.
Yeah. Eventually we should make extraSrcFiles be per-component, and data files.
ezyang
force-pushed
the
pr/nix-sdist-replacement
branch
from
64da514 to
9d90e3e
Compare
|
OK, addressed all comments except the last one. |
|
Are we going to deprecate sdist hooks for 2.0 as well? |
|
...I wasn't planning to. Sounds tricky, because it seems like sometimes that is an operation you want to hook? |
|
Though we could do better in terms of an API than re-using |
|
|
||
| -- | Monitor a single file | ||
| need :: FilePath -> Rebuild () | ||
| need f = monitorFiles [monitorFile f] |
There was a problem hiding this comment.
Oh, this should be monitorFileHashed in all cases shouldn't it? Well, ok, it's only used once, but in that one place it should be monitoring the content too (since it'll be a source file, and so the usual git checkout use case applies).
ezyang
force-pushed
the
pr/nix-sdist-replacement
branch
2 times, most recently
from
dec2282 to
6ffd85f
Compare
|
Added a suggestion about |
Signed-off-by: Edward Z. Yang <[email protected]>
…ing. New module Distribution.Client.SourceFiles implements 'needElaboratedConfiguredPackage', which if run in the 'Rebuild' monad is sufficient to ensure all source files that participate in a build are monitored. Fixes haskell#3401. It also fixes the "we didn't detect a new file appearing" problem. Signed-off-by: Edward Z. Yang <[email protected]>
ezyang
force-pushed
the
pr/nix-sdist-replacement
branch
from
6ffd85f to
19b8c68
Compare
|
FileMonitor handled |
|
LGTM. |
For non-Custom packages, replace sdist with hand-rolled rebuild checking.
New module Distribution.Client.SourceFiles implements
'needElaboratedConfiguredPackage', which if run in the 'Rebuild'
monad is sufficient to ensure all source files that participate
in a build are monitored.
Fixes #3401. It also fixes the "we didn't detect a new file
appearing" problem.