Multi Component #522
Multi Component #522
Conversation
|
I seem to experience an intermittent error using this branch of After outputting the above I made a commit of the code that exhibits the above symptoms here: runeksvendsen/rule-lang@6bdea22. I'm wondering if there's some non-determinism in the way multiple components are loaded, such that if an executable or test (which depend on the library) is loaded before the library then ghcide will complain that the library can't be found. Full log output of running |
|
@runeksvendsen I tested your branch using cabal and it worked for me (after I fixed the build errors in the library component). |
|
I noticed the logic to ensure that multiple initialisations do not happen concurrently is not currently correct. |
|
I'm no longer able to reproduce the above behaviour. I'm guessing the problem appeared when I edit the library component, so that it doesn't compile, and then go to a file in the executable target, which would result in VS Code hanging with the status "Processing x/y..." since ghcide didn't exit. |
There was a problem hiding this comment.
What is the right thing to do with autogen Cabal modules?
I got an error on the ghcide workspace (using this branch):
Shelling out to cabal "/home/pepe/scratch/ghcide/dist-newstyle/build/x86_64-linux/ghc-8.8.3/ghcide-0.1.0/build/autogen/Paths_ghcide.hs"
ghcide: CradleError ExitSuccess ["Multi Cradle: No prefixes matched","pwd: /home/pepe/scratch/ghcide","filepath: /home/pepe/scratch/ghcide/dist-newstyle/build/x86_64-linux/ghc-8.8.3/ghcide-0.1.0/build/autogen/Paths_ghcide.hs","prefixes:","(\"./src\",Cabal {component = Just \"ghcide:lib:ghcide\"})","(\"./exe\",Cabal {component = Just \"ghcide:exe:ghcide\"})","(\"./test\",Cabal {component = Just \"ghcide:test:ghcide-tests\"})","(\"./test/preprocessor\",Cabal {component = Just \"ghcide:exe:ghcide-test-preprocessor\"})"]
exe/Main.hs
Outdated
| print opts | ||
| res <- fst <$> session (hieYaml, toNormalizedFilePath' cfp, opts) | ||
| signalBarrier finished_barrier res | ||
| waitBarrier finished_barrier |
There was a problem hiding this comment.
if the thread never signals the barrier, ghcide could lock up in line 382 if this function is being called masked.
Consider using forkFinally or async instead of forkIO
There was a problem hiding this comment.
After further testing I don't think this code works as I intended at all. Need to rework it.
exe/Main.hs
Outdated
| modifyVar_ hscEnvs (return . Map.adjust (\(h, _) -> (h, [])) hieYaml ) | ||
| Nothing -> return () | ||
| -- We sort so exact matches come first. | ||
| case HM.lookup (toNormalizedFilePath' cfp) v of |
There was a problem hiding this comment.
this lookup is also performed in line 358. Did you forget to readVar fileToFlags?
There was a problem hiding this comment.
Yeah this doesn’t look right to me.
exe/Main.hs
Outdated
| return res | ||
|
|
||
| lock <- newLock | ||
| cradle_lock <- newLock |
There was a problem hiding this comment.
These locks could use a comment explaining what shared resource they are protecting
exe/Main.hs
Outdated
| -- We will modify the unitId and DynFlags used for | ||
| -- compilation but these are the true source of | ||
| -- information. | ||
| new_deps = (thisInstalledUnitId df, df, targets, cfp, dep_info) : maybe [] snd oldDeps |
There was a problem hiding this comment.
time to upgrade this 5-tuple to a record?
There was a problem hiding this comment.
Sounds like a good idea 👍
How do you trigger that error? Did you open that file in an editor? What I would expect to happen is that if you previously loaded any other file from the component then it should work. Otherwise it is a limitation of the manual multi-cradle that you are manually specifying the directories. |
I did not open that file directly. Was trying to get a hover in |
|
I tested this in combination with Reproduce
ObservationsThe first two seem to be working fine. Also opening these files in an editor gives expected IDE features, like hover and goto definition. (There's an issue with goto-definition across files where it tries to open files under an absolute path, e.g. However, the third one freezes after printing the following to the terminal: The trouble seems to be that the third one depends on Fortunately, this works when using the convenience symlink Issues
|
|
@aherrmann Is there a reason you are using the multi-cradle AND bios cradle together? You should ideally modify your bios script so that given a filepath it returns the set of options for the component which contains that file. Just so we are clear here, the multi-cradle is intended to patch over the fact that build tools do not in general support querying for a component based on filepaths. It is also possible your cradle is not listing all the targets for the component it is trying to load. If it doesn't then it will not work with this branch. Also the specific behaviour that I have implemented about what should/shouldn't happen in the following situations is up for discussion. What I have implemented in this patch are the foundations which mean making these other decision is easier.
Ultimately it is not just up to me to decide on these things and to also implement them. The branch is working for my purposes working on GHC and with cabal projects so it will take a wider community effort to bring it to a state where it can be merged. |
|
@aherrmann-da I looked at your PR a little and it seems the You can probably also fix the issue with go-to definition by using absolute paths for the include directories ( |
That's a very useful feature (that the filepath is passed to the bios script). I was not aware that this PR also introduced it. Note, this is not available as of
Apart from the issue around the path prefix of generated files the targets are complete. Note, that the additional entry I added for the generated files points to the same
That PR is targeting I'll experiment with using the argument to
Yes, it was a builtin cradle (not bios) that, given a source file, queried for the corresponding However, this means there is no longer a 1:1 mapping from source to REPL target, as a user might have multiple REPL targets (indirectly) including the same source file. That made multi cradle look like an obvious candidate to let the user define this mapping. Defining that mapping in |
All cradles have had this interface since the earliest iteration of You are right there is an issue about deciding how to map a FilePath to a component, but that is for the author of the build tool to determine, not ghcide. |
exe/Main.hs
Outdated
| setNameCache nc hsc = hsc { hsc_NC = nc } | ||
|
|
||
| -- This is the key function which implements multi-component support. All | ||
| -- components mapping to the same hie,yaml file are mapped to the same |
There was a problem hiding this comment.
typo
| -- components mapping to the same hie,yaml file are mapped to the same | |
| -- components mapping to the same hie.yaml file are mapped to the same |
|
I am using this branch as my daily driver for about two weeks now. Except for the occasional GHC bug, #529, my expierence has been vastly positive. |
|
Even if imperfect, it is better than no multi-component support :) |
mpickering
force-pushed
the
wip/multi-rebase
branch
from
c43a078
to
256f8b5
Compare
|
This needs rebasing. |
|
I have some more fixes on the staging branch and waiting for one PR to be ready. |
There was a problem hiding this comment.
First of all, I’m very sorry for doing such a terrible job at reviewing this in a timely fashion.
Second, fantastic work, thank you so much!
I’ve left a bunch of comments and it looks like this also needs to get rebased on top of the header-only parsing. Ideally, I’d also like to see a small test but if this turns out to be too complex, I could be persuaded to omit this for now.
exe/Main.hs
Outdated
| import Rules | ||
| import RuleTypes | ||
| import Data.Either | ||
| --import Outputable (pprTraceM, ppr, text) |
There was a problem hiding this comment.
| --import Outputable (pprTraceM, ppr, text) |
exe/Main.hs
Outdated
| --import Outputable (pprTraceM, ppr, text) | ||
| import qualified Crypto.Hash.SHA1 as H | ||
| import qualified Data.ByteString.Char8 as B | ||
| import Data.ByteString.Base16 (encode) |
There was a problem hiding this comment.
Please don’t align imports. It just makes diffs more noisy.
There was a problem hiding this comment.
Ok, but there are other places where imports are already aligned..
There was a problem hiding this comment.
Yep, the formatting on this code reveals the history of multiple different authors. One day it would be great to run a real formatter over it, since that's the only way to recover from the mess we have now.
exe/Main.hs
Outdated
| --import Rules | ||
| --import RuleTypes | ||
| -- |
There was a problem hiding this comment.
| --import Rules | |
| --import RuleTypes | |
| -- |
exe/Main.hs
Outdated
| -- results <- runActionSync ide $ use TypeCheck $ toNormalizedFilePath' "src/Development/IDE/Core/Rules.hs" | ||
| -- results <- runActionSync ide $ use TypeCheck $ toNormalizedFilePath' "exe/Main.hs" |
There was a problem hiding this comment.
| -- results <- runActionSync ide $ use TypeCheck $ toNormalizedFilePath' "src/Development/IDE/Core/Rules.hs" | |
| -- results <- runActionSync ide $ use TypeCheck $ toNormalizedFilePath' "exe/Main.hs" |
| -- Parse DynFlags for the newly discovered component | ||
| hscEnv <- emptyHscEnv | ||
| (df, targets) <- evalGhcEnv hscEnv $ do | ||
| setOptions opts (hsc_dflags hscEnv) |
There was a problem hiding this comment.
The do seems to be redundant and the indentation is a bit weird.
exe/Main.hs
Outdated
| modifyVar_ hscEnvs (return . Map.adjust (\(h, _) -> (h, [])) hieYaml ) | ||
| Nothing -> return () | ||
| -- We sort so exact matches come first. | ||
| case HM.lookup (toNormalizedFilePath' cfp) v of |
There was a problem hiding this comment.
Yeah this doesn’t look right to me.
exe/Main.hs
Outdated
| finished <- poll as | ||
| case finished of | ||
| Just {} -> do | ||
| as <- async $ getOptions file |
There was a problem hiding this comment.
It looks like you can get interrupted between creating the async and updating the IORef. Should this be masked?
test/exe/Main.hs
Outdated
| getWatchedFilesSubscriptionsUntilProgressEnd :: Session [Maybe Value] | ||
| getWatchedFilesSubscriptionsUntilProgressEnd = do | ||
| msgs <- manyTill (Just <$> message @RegisterCapabilityRequest <|> Nothing <$ anyMessage) (message @WorkDoneProgressEndNotification) | ||
| getWatchedFilesSubscriptionsUntil :: forall end . (FromJSON end, Typeable end) => Session [Maybe Value] |
There was a problem hiding this comment.
How hard is it to setup a test that verifies that multi cradles at least work to some degree and we don’t break them completely? I guess direct-style cradles are a bit annoying to setup correctly but maybe we can make some simple example with two packages A, B and a cabal file and verify that we can first load a file from B check that go to definition to A does not work. Then load a file from A and check that it works?
|
Thanks for your comments Moritz. There is a rebased version on my fork which also has a few other fixes which are not obvious so I will get that pushed sometime next week. There is one known issue which I am not sure what to do about. If you have two packages in your project A and C and if A depends on B which depends on C then if you load A and C into a session but not B then you get confusing errors about the same type coming from different packages. My idea about how to fix this is to try using OneShot mode instead of --make mode, which seems to make sense to me anyway. I am predicting this will fix it as this will bypass the HPT which is why the fake_uid thing is necessary in the first place, to my understanding. Would you still merge the patch with this issue? It is still an improvement anyway over no multi-component support at all but I am not sure how to detect this situation and fail gracefully. |
Yes, that doesn’t seem too bad. Just add it to the troubleshooting section. |
|
ok then the plan is I will update the PR with the other changes from the fork and address your review comments. Then ping you again for a second review and after it is merged I can try the OneShot mode. |
mpickering
force-pushed
the
wip/multi-rebase
branch
from
256f8b5
to
48f51be
Compare
|
@cocreature I have pushed a new version which contains some bugs fixes.
I think it would benefit from another complete read. |
| @@ -166,16 +167,20 @@ moduleImportPath (takeDirectory . fromNormalizedFilePath -> pathDir) mn | |||
| -- if they are created with the same call to 'newHscEnvEq'. | |||
| data HscEnvEq | |||
| = HscEnvEq !Unique !HscEnv | |||
| [(InstalledUnitId, DynFlags)] -- In memory components for this HscEnv | |||
| -- This is only used at the moment for the import dirs in | |||
| -- the DynFlags | |||
| | GhcVersionMismatch { compileTime :: !Version | |||
There was a problem hiding this comment.
We can make this GhcVersionMismatch into a proper diagnostic now I think rather than this special constructor on HscEnvEq. Not sure whether to do that in this patch.
There was a problem hiding this comment.
I have another PR making changes to HscEnvEq which will need to be reviewed once this is merged, and I am happy to convert GhcVersionMismatch to a proper diagnostic if that is possible at all
There was a problem hiding this comment.
Would you prefer me to remove it in this patch? Or leave it until later?
There was a problem hiding this comment.
This PR is quite big already, I'm offering to take care of it myself once this is merged.
| @@ -472,7 +480,7 @@ instance Hashable GhcSessionIO | |||
| instance NFData GhcSessionIO | |||
| instance Binary GhcSessionIO | |||
|
|
|||
| newtype GhcSessionFun = GhcSessionFun (FilePath -> Action HscEnvEq) | |||
| newtype GhcSessionFun = GhcSessionFun (FilePath -> Action (IdeResult HscEnvEq)) | |||
There was a problem hiding this comment.
Notice I made GhcSessionFun return a proper IdeResult so that you can get proper diagnostics for failing cradle loads now.
| cradle: | ||
| cabal: | ||
| - path: "./src" | ||
| component: "ghcide:lib:ghcide" | ||
| - path: "./exe" | ||
| component: "ghcide:exe:ghcide" | ||
| - path: "./test" | ||
| component: "ghcide:test:ghcide-tests" | ||
| - path: "./test/preprocessor" | ||
| component: "ghcide:exe:ghcide-test-preprocessor" |
|
I think what is happening is this:
I think the way forward here is:
|
|
Hmm always generating hie files doesn't help the situation. |
|
I’m not quite following your explanation:
|
ghcide does not end up customizing the hieDir as it turns out that the flag always has a value, so the
I'm flipping A and B mentally in order to agree with what the test does. Why is the I can reproduce the problem using VSCode. And indeed, a dirty dist-newstyle folder makes the problem go away. Wiping the dist-newstyle folder and reloading brings the problem back. As an experiment I tried to wipe everything except for the |
|
I have worked out why this is happening now and it's nothing to do with hie files as pointed out by Moritz and Pepe. The bug is in some new code I added to make package imports work. |
You have to use the DynFlags for the file we are currently compiling to get the right packages in the package db so that lookupPackage doesn't always fail.
This reverts commit 820aa24.
|
Pushed some changes which I hope should fix the tests now, and a bonus test as well which loads the components in the other order. Who would have thought adding a test would actually show up a bug. |
|
It now appears that |
As usual, stack doesn’t understand Cabal properly and doesn’t seem to like ** wildcards so I’ve enumerated it manually.
|
Pushed a fix (hopefully) for that. |
|
Thanks Moritz, you beat me to a fix.. my push failed! Did you come up with that list of variablesthrough testing locally or from using your mind? |
|
Tested locally, I already had this change from when I was looking at the failing test on the weekend. I just forgot to push it since it failed with cabal as well so I forgot about it. |
|
Thanks, looks like the simple-multi tests pass on CI now. |
|
Crap that breaks the plugin tests, let me try to unset the env more locally. |
Fantastic work 
The type changed with multi component support: haskell/ghcide#522
* Multi component support In this commit we add support for loading multiple components into one ghcide session. The current behaviour is that each component is loaded lazily into the session. When a file from an unrecognised component is loaded, the cradle is consulted again to get a new set of options for the new component. This will cause all the currently loaded files to be reloaded into a new HscEnv which is shared by all the currently known components. The result of this is that functions such as go-to definition work between components if they have been loaded into the same session but you have to open at least one file from each component before it will work. Only minimal changes are needed to the internals to ghcide to make the file searching logic look in include directories for all currently loaded components. The main changes are in exe/Main.hs which has been heavily rewritten to avoid shake indirections. A global map is created which maps a filepath to the HscEnv which should be used to compile it. When a new component is created this map is completely refreshed so each path maps to a new Which paths belong to a componenent is determined by the targets listed by the cradle. Therefore it is important that each cradle also lists all the targets for the cradle. There are some other choices here as well which are less accurate such as mapping via include directories which is the aproach that I implemented in haskell-ide-engine. The commit has been tested so far with cabal and hadrian. Also deleted the .ghci file which was causing errors during testing and seemed broken anyway. Co-authored-by: Alan Zimmerman <alan.zimm@gmail.com> Co-authored-by: fendor <power.walross@gmail.com> * Final tweaks? * Fix 8.4 build * Add multi-component test * Fix hlint * Add cabal to CI images * Modify path * Set PATH in the right place (hopefully) * Always generate interface files and hie files * Use correct DynFlags in mkImportDirs You have to use the DynFlags for the file we are currently compiling to get the right packages in the package db so that lookupPackage doesn't always fail. * Revert "Always generate interface files and hie files" This reverts commit 820aa241890c4498c566e29b0823a803fb2fd297. * remove traces * Another test * lint * Unset env vars set my stack * Fix extra-source-files As usual, stack doesn’t understand Cabal properly and doesn’t seem to like ** wildcards so I’ve enumerated it manually. * Unset env locally Co-authored-by: Alan Zimmerman <alan.zimm@gmail.com> Co-authored-by: fendor <power.walross@gmail.com> Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
* Multi component support In this commit we add support for loading multiple components into one ghcide session. The current behaviour is that each component is loaded lazily into the session. When a file from an unrecognised component is loaded, the cradle is consulted again to get a new set of options for the new component. This will cause all the currently loaded files to be reloaded into a new HscEnv which is shared by all the currently known components. The result of this is that functions such as go-to definition work between components if they have been loaded into the same session but you have to open at least one file from each component before it will work. Only minimal changes are needed to the internals to ghcide to make the file searching logic look in include directories for all currently loaded components. The main changes are in exe/Main.hs which has been heavily rewritten to avoid shake indirections. A global map is created which maps a filepath to the HscEnv which should be used to compile it. When a new component is created this map is completely refreshed so each path maps to a new Which paths belong to a componenent is determined by the targets listed by the cradle. Therefore it is important that each cradle also lists all the targets for the cradle. There are some other choices here as well which are less accurate such as mapping via include directories which is the aproach that I implemented in haskell-ide-engine. The commit has been tested so far with cabal and hadrian. Also deleted the .ghci file which was causing errors during testing and seemed broken anyway. Co-authored-by: Alan Zimmerman <alan.zimm@gmail.com> Co-authored-by: fendor <power.walross@gmail.com> * Final tweaks? * Fix 8.4 build * Add multi-component test * Fix hlint * Add cabal to CI images * Modify path * Set PATH in the right place (hopefully) * Always generate interface files and hie files * Use correct DynFlags in mkImportDirs You have to use the DynFlags for the file we are currently compiling to get the right packages in the package db so that lookupPackage doesn't always fail. * Revert "Always generate interface files and hie files" This reverts commit 820aa241890c4498c566e29b0823a803fb2fd297. * remove traces * Another test * lint * Unset env vars set my stack * Fix extra-source-files As usual, stack doesn’t understand Cabal properly and doesn’t seem to like ** wildcards so I’ve enumerated it manually. * Unset env locally Co-authored-by: Alan Zimmerman <alan.zimm@gmail.com> Co-authored-by: fendor <power.walross@gmail.com> Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
* Multi component support In this commit we add support for loading multiple components into one ghcide session. The current behaviour is that each component is loaded lazily into the session. When a file from an unrecognised component is loaded, the cradle is consulted again to get a new set of options for the new component. This will cause all the currently loaded files to be reloaded into a new HscEnv which is shared by all the currently known components. The result of this is that functions such as go-to definition work between components if they have been loaded into the same session but you have to open at least one file from each component before it will work. Only minimal changes are needed to the internals to ghcide to make the file searching logic look in include directories for all currently loaded components. The main changes are in exe/Main.hs which has been heavily rewritten to avoid shake indirections. A global map is created which maps a filepath to the HscEnv which should be used to compile it. When a new component is created this map is completely refreshed so each path maps to a new Which paths belong to a componenent is determined by the targets listed by the cradle. Therefore it is important that each cradle also lists all the targets for the cradle. There are some other choices here as well which are less accurate such as mapping via include directories which is the aproach that I implemented in haskell-ide-engine. The commit has been tested so far with cabal and hadrian. Also deleted the .ghci file which was causing errors during testing and seemed broken anyway. Co-authored-by: Alan Zimmerman <alan.zimm@gmail.com> Co-authored-by: fendor <power.walross@gmail.com> * Final tweaks? * Fix 8.4 build * Add multi-component test * Fix hlint * Add cabal to CI images * Modify path * Set PATH in the right place (hopefully) * Always generate interface files and hie files * Use correct DynFlags in mkImportDirs You have to use the DynFlags for the file we are currently compiling to get the right packages in the package db so that lookupPackage doesn't always fail. * Revert "Always generate interface files and hie files" This reverts commit 820aa241890c4498c566e29b0823a803fb2fd297. * remove traces * Another test * lint * Unset env vars set my stack * Fix extra-source-files As usual, stack doesn’t understand Cabal properly and doesn’t seem to like ** wildcards so I’ve enumerated it manually. * Unset env locally Co-authored-by: Alan Zimmerman <alan.zimm@gmail.com> Co-authored-by: fendor <power.walross@gmail.com> Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
In this commit we add support for loading multiple components into one
ghcide session.
The current behaviour is that each component is loaded lazily into the
session. When a file from an unrecognised component is loaded, the
cradle is consulted again to get a new set of options for the new
component. This will cause all the currently loaded files to be
reloaded into a new HscEnv which is shared by all the currently known
components. The result of this is that functions such as go-to
definition work between components if they have been loaded into the
same session but you have to open at least one file from each component
before it will work.
Only minimal changes are needed to the internals to ghcide to make the
file searching logic look in include directories for all currently
loaded components. The main changes are in exe/Main.hs which has been
heavily rewritten to avoid shake indirections. A global map is created
which maps a filepath to the HscEnv which should be used to compile it.
When a new component is created this map is completely refreshed so each
path maps to a new
Which paths belong to a componenent is determined by the targets listed
by the cradle. Therefore it is important that each cradle also lists all
the targets for the cradle. There are some other choices here as well
which are less accurate such as mapping via include directories which
is the aproach that I implemented in haskell-ide-engine.
The commit has been tested so far with cabal and hadrian.
Also deleted the .ghci file which was causing errors during testing and
seemed broken anyway.