Further refactoring around monitoring cabal files #3347
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 16, 2016
|
LGTM. |
| @@ -55,8 +55,7 @@ data GlobPiece = WildCard | |||
|
|
|||
| data FilePathRoot | |||
| = FilePathRelative | |||
| | FilePathUnixRoot | |||
| | FilePathWinDrive Char | |||
| | FilePathRoot String -- e.g. "/", "c:\" or result of FilePath.takeDrive | |||
There was a problem hiding this comment.
This is very minor, but why not a FilePath here?
|
OK LGTM. |
|
Ok, will squash the fixes in and merge. @23Skidoo would you mind considering this for 1.24 too. As explained at the top, I think the impact is that new .cabal files dropped in to match a glob will not be picked up, affecting the use case of |
|
Sure, this can go into 1.24. |
Previously we represented unix and windows roots/drives explicitly but this isn't necessary and it makes it inconvenient to use portable functions like FilePath.takeDrive (which returns "/" on unix or things like "c:\\" on windows). So instead we just use a FilePath as the root.
Fixes haskell#3323 and haskell#3324, ensuring we monitor the project Cabal files. Original fix by Edward Z. Yang. The approach in this patch is to fix an underlying problem. Subsequent patches use a more consistent approach to the monitoring as suggested by Edward. The motivating example is: matches <- matchFileGlob dirname glob where matchFileGlob is defined in the RebuildMonad module as matchFileGlob root glob = do monitorFiles [monitorFileGlob glob] liftIO $ Glob.matchFileGlob root glob This usage is wrong because the root used to match the glob is not the same as the root that will be used later when checking the file monitor for changes. You can see this is suspicious because the declaration of the monitor does not take an root dir paramater but the immediate matching does. That's because the root for the monitors is specified when we do the rerunIfChanged to check the monitor. So the only correct usage involves passing in the correct root. This is a ripe source of bugs. So this refactoring moves the root into the Rebuild monad directly, so the example becomes: matches <- matchFileGlob glob The root is implicit, so you can't accidentally pick a different root for the immediate match vs the later monitor check. Of course the root still matters, but if you get that wrong you'll notice immediately because you will not get the match results you were expecting. So the root is now passed in with runRebuild, not with rerunIfChanged. Also change the incorrect use of matchFileGlob. This use case now relies on the adjusted representation of glob roots, using FilePath.splitDrive to obtain the root (if any).
Keep the paths relative when the user specifies them as relative. This is more consitent for file monitoring and error reporting. Convert to absolute later (as expected by other code).
Since the matchFileGlob function returns names of files and not content, actions using it don't actually depend on the content, so it's more consistent not to monitor the content. In particular, following Edward's recent fix, the code that reads the package cabal files already monitors the content. This is the appropriate separation of concerns, the code that finds the .cabal files monitors the search glob for file existence, while the code that reads the .cabal files monitors the file content.
Without this change, Edward's original patch to monitor the contents of Cabal files wouldn't actually be needed to make the test pass. Originally the test only covered the case of specifying a package by a glob that matches a .cabal file, and that case already worked because the glob matching already included a monitor on the glob matches (ie the .cabal file). What did not work was a glob that matched a directory, since in that case we did the the glob match for $thedir/*.cabal incorrectly, meaning that we didn't end up monitoring the files properly (a path mismatch meant we were monitoring different files).
dcoutts
force-pushed
the
monitor-cabal-files
branch
from
671ee00
to
518b9b3
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This builds on the partial fix from #3325. That patch makes sure that the content of the .cabal files is monitored.
In fact if the original code were working properly the conent would already be being monitored, and not just the content but also the search for the file in the case of a file glob (ie detecting changes in the set of files that match the glob).
So these patches fix the original problem plus some refactoring to eliminate the class of bug at work here. We do not revert the basic idea from #3325 however, we follow the idea that the code that searches for the files should monitor existence of the set of files that match the glob, while the code that reads the files should monitor the file content. So we keep the bit of #3325 that adds monitoring of the file content, and remove file content monitoring from the glob, keeping just monitoring file existence.
This ought to merge into 1.24 without too much problem. It's probably wise to do so otherwise when people use globs like:
packages: ./*/*.cabal(which indeed is the default/implicit project config) then we'll fail to spot new files matching the glob (e.g. as a result of cabal unpack), since the fix in #3325 only covers the content of files we did find, not the glob.