Implement the HIE Bios #1126
Implement the HIE Bios #1126
Conversation
|
The relevant ticket is #1053 |
There was a problem hiding this comment.
Basically the idea here is nothing new. Calling cabal repl --with-compiler=some-wrapper-thing was literally one of the first designs I tried in ghc-mod before cabal-helper. I abandoned it for legacy reasons: namely low latency single shot command support for the vim people among other thing.
@mpickering definitely convinced me that now that these legacy factors are gone going back to that approach as a low effort, wide spectrum, base implementation for pretty much all Haskell build systems in existence is a very good idea and exactly the reason we need fresh eyes on these problems. Mine are obviously old and broken for completely missing this.
I'd definitely like to steal that but I think we shouldn't just throw everything away on a whim.
Especially with show-build-info back on the right track thank's to @hvr's students (haskell/cabal#5954) cabal-helper can be made even more reliable to the point where it's not even a dirty hack at all anymore. Removing the cabal-helper-wrapper and $libexec stuff already made a huge dent I think.
According to @mpickering starting from scratch instead of integrating into cabal-helper is partially a decision motivated by GPL aversion. I'm not sure if it's just FUD related or an actual real world problem. I'd really like to find out since it would be much more productive to work together on this rather than in different silos.
One more point I'd like to make is that this patch doesn't change the horribleness of the old ghc-mod Cradle approach at all. @mpickering do you have a plan on how to provide a better user experience than just randomly guessing one cradle type on overlap? Because I do :)
Overall I'm still not really convinced forking an ancient version of ghc-mod and patching it to hell is a good idea instead of just integrating this into cabal-helper and working together on moving that forward.
|
@mpickering @DanielG thanks to both of you |
|
@DanielG @mpickering Is the plan for this PR to fully-remove/replace the ghc-mod plugin? I'm currently wondering for the sake of wether or not this gets rid of cabal-helper-wrapper: it would be nice to get this rid of it so we can unblock static builds (cc @fendor this is related to #1143 #1068 #529 ) |
|
cabal-helper-wrapper is gone with cabal-helper-1.0 either way so this PR isn't strictly a prerequisite for that particular issue. FYI if someone feels inclined it would actually be downright trivial to remove it from the 0.8 branch too until the 1.0 release happens. As far as I understand from discussion with @mpickering his intention is to more or less replace the ghc-mod plugin with this. However things aren't so easy, this approach he's chosen has significantly different tradeoffs than what we're used to from ghc-mod/cabal-helper land so just replacing outright is a bit drastic. I've been discussing this with @mpickering and I think we've come to the understanding that both approaches are worthwhile and we'll will work towards having HIE allow both. It is unclear at this point how we handle the transition though. Obviously @mpickering wants us to adopt his branch and then integrate the cabal-helper story into that but I'm erring on the side of caution and proposing to first bring ghc-mod(-core) in line with cabal-helper-1.0 first and then after we relieve the time pressure to release something that works with new-build take the time to re-design the interface between HIE and GHC taking into account his approach. Incidentally I'm planning on submitting a GSoC proposal for just such an endeavor. IMO radically changing the software stack like this is too risky but ultimately I defer that decision to people more actively working on HIE than me. Oh and I should mention that since my last comment I actually did a bit more investigating and unfortunately @mpickering's approach will simply not fit into cabal-helper for technical reasons. So unfortunately folding both efforts together isn't easily possible. |
|
From my perspective replacing the ghc-mod backend with hie-bios has the quite significant advantage of working in many more situations. It also makes future maintenance easier by passing the responsibility for setting up the environment off to consumers. My idea here is that first a robust foundation needs to be created and then additional complicated There are some things which are not working yet on the branch
Vague comments like this are not very helpful. Please can you explain precisely what the problem is? |
Like I mentioned in private cabal-helper expects to be able to enumerate the project essentially, so I need at the very least a way to get a list of all Cabal packages related to a given super-project. Your repl trick however simply relies on having the path to the current Haskell source file and let the build tool figure out which package/component it belongs too. So there is no channel to get that information. Come to mention it I still don't really see how that is going to work properly with cabal or stack? Their repl commands don't support that at the moment (AFAIK). Can you explain how you plan to make that work? |
|
The stack repl command supports querying by a file path already. Cabal doesn't but which component to use can be configured a la ghcid. Once |
|
Please put git modules in the |
I might give this a shot on @alanz's fork, although I'm not sure if its been merged in with the latest upstream changes. |
| # url = https://github.com/bubba/ghc-mod.git | ||
| url = https://github.com/alanz/ghc-mod.git | ||
|
|
||
| url = https://github.com/fendor/ghc-mod.git |
There was a problem hiding this comment.
Need an issue which details what to do to remove this?
There was a problem hiding this comment.
Good idea, we want to subsume ghc-project-types to make us independent of ghc-mod.
There was a problem hiding this comment.
Moreover, if we take ghc-project-types and turn it into its own project, we are almost ready to go to hackage.
There was a problem hiding this comment.
I did discuss that with @DanielG , he is happy for us to move it elsewhere, possibly as a sub-project inside haskell-ide-engine.
| cradle: | ||
| cabal: | ||
| component: "lib:haskell-ide-engine" | ||
| ``` |
There was a problem hiding this comment.
Use the lightweight-cabal syntax here I think.
There was a problem hiding this comment.
Here it should show how to define multi-cradles. Mutli-Cabal version is a bit above
| Left err -> error (show err) | ||
| Right cr -> return $ GM.cradleRootDir cr | ||
| -- Get the cabal directory from the cradle | ||
| cradle <- findLocalCradle (d </> "File.hs") |
There was a problem hiding this comment.
Unsure this will work well with multi-cradle projects.
There was a problem hiding this comment.
It wont, we have to tackle it in hie-bios
There was a problem hiding this comment.
I manually tested and documented how I intend it to work in https://github.com/mpickering/haskell-ide-engine/blob/hie-bios/hie-plugin-api/Haskell/Ide/Engine/Cradle.hs#L256
There was a problem hiding this comment.
So, this will work in a best effort manner, imo.
| in makeRequest $ IReq tn reqId reqCallback x | ||
| go acc [] _ _ _ callback = callback acc | ||
| go acc (x : xs) d tn reqId callback = | ||
| let reqCallback result = go (acc ++ [result]) xs d tn reqId callback |
There was a problem hiding this comment.
I don't think that changed on this PR? Is that a general point?
| errorHandler lid J.InternalError msg | ||
| errorHandler (Just lid) J.InternalError msg | ||
|
|
||
| liftIO $ traceEventIO $ "STOP " ++ show tn ++ "ide:" ++ d |
There was a problem hiding this comment.
We should make a new module for these trace events to abstract over this pattern.
There was a problem hiding this comment.
We could move a fitting function, traceEvent :: MonadIO m => String -> m a -> m a into PluginUtils? Or into a module hie-plugin-api/Haskell/Ide/Engine/TraceEvent.hs?
| @@ -0,0 +1 @@ | |||
| cabal-helper-helper . $1 | |||
There was a problem hiding this comment.
Yeah, we need to remove them.
| when (optGhcModVomit opts) $ | ||
| logm "Enabling --vomit for ghc-mod. Output will be on stderr" | ||
| when (optBiosVerbose opts) $ | ||
| logm "Enabling verbose mode for hie-bios. This option currently doesn't do anything." |
There was a problem hiding this comment.
I would be in favour of removing these manual command line arguments in favour for LSP's InitialiseParams.trace
| @@ -1,5 +1,5 @@ | |||
| name: haskell-ide-engine | |||
| version: 0.14.0.0 | |||
| version: 1.0.0.0 | |||
There was a problem hiding this comment.
I would leave the version bump till after the merge. I think it is a definitely a worthwhile change but there's probably still some ironing out to do once it lands in master
| -- If the virtual file associated to the given uri does not exist, Nothing | ||
| -- is returned. | ||
| persistVirtualFile' :: Core.LspFuncs Config -> Uri -> IO (Maybe FilePath) | ||
| persistVirtualFile' lf uri = Core.persistVirtualFileFunc lf (toNormalizedUri uri) |
There was a problem hiding this comment.
These smell as if they should be in haskell-lsp
| @@ -116,6 +117,11 @@ updateDocumentRequest | |||
| :: (MonadIO m, MonadReader REnv m) => Uri -> Int -> PluginRequest R -> m () | |||
| updateDocumentRequest = Scheduler.updateDocumentRequest | |||
|
|
|||
| updateDocument :: (MonadIO m, MonadReader REnv m) => Uri -> Int -> m () | |||
There was a problem hiding this comment.
Can this be documented to clarify what exactly its doing again? @mpickering
| @@ -46,7 +48,7 @@ spec = describe "code actions" $ do | |||
| contents <- getDocumentEdit doc | |||
| liftIO $ contents `shouldBe` "main = undefined\nfoo x = x\n" | |||
|
|
|||
| noDiagnostics | |||
| -- noDiagnostics | |||
There was a problem hiding this comment.
Why have these been commented out?
There was a problem hiding this comment.
No clue. @alanz ? We can try to enable them again.
There was a problem hiding this comment.
I found the tests were failing on my local machine with them in. I have no strong feeling, so long as CI is happy.
There was a problem hiding this comment.
lemme try reenabling them
Co-Authored-By: Luke Lau <[email protected]>
Co-Authored-By: Luke Lau <[email protected]>
| @@ -118,7 +125,7 @@ newScheduler plugins biosOpts = do | |||
| } | |||
|
|
|||
| -- | A handler for any errors that the dispatcher may encounter. | |||
| type ErrorHandler = J.LspId -> J.ErrorCode -> T.Text -> IO () | |||
| type ErrorHandler = Maybe J.LspId -> J.ErrorCode -> T.Text -> IO () | |||
There was a problem hiding this comment.
This was an ad-hoc change that somebody should think about again. Intention was that in LspStdio.hs we can differentiate between errors caused by requests and stuff such as runActionWithContext
Clarify difference between lsp and hie-bios configuration
|
Congrats to everyone thats worked on this, this is a huge milestone 🥳 |
This branch implements the HIE bios which is a simplified way of setting up a GHC session.
The library,
hie-bios, is responsible for one thing and only one thing. That is to set up the correct GHC session.The guiding principle of
hie-biosis:This means that it is easy to implement support for any build tool. I already implemented support for stack/cabal/hadrian/rules_haskell/obelisk. These are the main build tools I think. More information can be read in the README
I need help in finishing this PR which is why I am putting it up here.
ghc-mod. In theory it could still useghc-modI think but I didn't give it much thought yet.ghc-modwhich are notghc-modspecific. They need to be moved intohaskell-ide-engineor upstreamed.DynFlagswhen changing between sessions? This is not something the GHC API is really designed for, I would think you would have to make a new GHC session for each component or spawn another process. If the approach of replacing DynFlags really does work then we just need to create a map from arguments toDynFlags. This also deals with creating a new session if the project configuration changes quite naturally as the cache will miss and a new session created.