-
-
Notifications
You must be signed in to change notification settings - Fork 397
Reload .cabal files when they are modified #4630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
a948772
to
4bd3727
Compare
4bd3727
to
b9250a5
Compare
@@ -586,7 +586,7 @@ loadSessionWithOptions recorder SessionLoadingOptions{..} rootDir que = do | |||
unless (null new_deps || not checkProject) $ do | |||
cfps' <- liftIO $ filterM (IO.doesFileExist . fromNormalizedFilePath) (concatMap targetLocations all_targets) | |||
void $ shakeEnqueue extras $ mkDelayedAction "InitialLoad" Debug $ void $ do | |||
mmt <- uses GetModificationTime cfps' | |||
mmt <- uses GetPhysicalModificationTime cfps' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how ignoring version in the VFS help us here🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because cabal only works with the physical file system. If the VFS changes, then cabal simply can't perform a reload, as cabal repl
will still observe the same .cabal
file. That's why we only want to invalidate the shake session, if the .cabal
file on disk has been modified.
Further, let's have a look at getModificationTimeImpl
:
getModificationTimeImpl
:: Bool
-> NormalizedFilePath
-> Action (Maybe BS.ByteString, ([FileDiagnostic], Maybe FileVersion))
getModificationTimeImpl missingFileDiags file = do
let file' = fromNormalizedFilePath file
let wrap time = (Just $ LBS.toStrict $ B.encode $ toRational time, ([], Just $ ModificationTime time))
mbVf <- getVirtualFile file
case mbVf of
Just (virtualFileVersion -> ver) -> do
alwaysRerun
pure (Just $ LBS.toStrict $ B.encode ver, ([], Just $ VFSVersion ver))
Nothing -> do
isWF <- use_ AddWatchedFile file
if isWF
then -- the file is watched so we can rely on FileWatched notifications,
-- but also need a dependency on IsFileOfInterest to reinstall
-- alwaysRerun when the file becomes VFS
void (use_ IsFileOfInterest file)
else if isInterface file
then -- interface files are tracked specially using the closed world assumption
pure ()
else -- in all other cases we will need to freshly check the file system
alwaysRerun
...
First, we don't care about the isInterface file
check. Then, if the file isn't open in the VFS, we will never rerun this rule, unless the IsFileOfInterest
rule is marked dirty, which it never will for .cabal
files.
In my understanding, isWF
will be True
for .cabal
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, much clear to me now.
Just some more interesting thoughts, do you think we can make GetModificationTime rule use GetPhysicalModificationTime rule in the end to remove some duplicated file time query? |
I am not sure, maybe. The whole file watcher architecture requires a thorough overhaul, it feels very brittle. |
Partially addresses #4236
It doesn't work with
sessionLoading
multipleComponents
becausehie-bios
doesn't give us the correct cradle dependencies.A workaround is to declare the cradle dependencies in the
hie.yaml
. We need to fix this in hie-bios proper, but it is no longer an HLS bug.