Merge #3347 into 1.24 #3358
Merged
Merge #3347 into 1.24 #3358
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
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. (cherry picked from commit 2e17ea2)
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). (cherry picked from commit 518b9b3)
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). (cherry picked from commit bba5a81)
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). (cherry picked from commit fcb61b7)
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. (cherry picked from commit ed1cc2f)
|
OK LGTM. |
|
@23Skidoo Thanks! |
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.
See #3347.