haskell / haskell-ide-engine

The engine for haskell ide-integration. Not an IDE
BSD 3-Clause "New" or "Revised" License
2.38k stars 211 forks source link

Extract Cabal-Helper specific code into its own library #1631

Open fendor opened 4 years ago

fendor commented 4 years ago

Talking with @jneira on irc, we realized this makes it easier to share code between the projects hie, hls and, potentially, ghcide! Code would be entirely taken from Cradle.hs in hie-plugin-api and the entire logic of discovering cradles with cabal-helper would be moved into its own library. Opinions?

jneira commented 4 years ago

I've just discovered that Haskell.IDe.Engine.Cradle only imports Haskell.Ide.Engine.Logger from hie own code! Almost it is asking to live in its own home. 😄

jneira commented 4 years ago

In case we decided to create the library, what about implicit-hie-bios (or hie-bios-implicit or something alike), to not bind it to cabal-helper. Maybe some day both build tools stack and cabal gives us enough info to use them directly? or we could add support for other tools implicit cradle?

ndmitchell commented 4 years ago

I really like the abstraction boundary that hie-bios figures out what the flags for the session should be. If it became a plugin API that I could then configure I'd be much less happy, as it's something I'm super grateful to not have to care about. Whether it incorporates or includes cabal-helper like functionality under the hood is a detail, so if this is transparent, I'm fine with it.

fendor commented 4 years ago

I would not have considered it a plugin API, but I think I see what you mean. The intention would have been to extract code from Haskell.IDe.Engine.Cradle since only roughly six functions are used elsewhere in haskell-ide-engine, none of them specific to cabal-helper. I think one can argue that this micro-package would be nothing else but moved code for now. Do you dislike the idea to have an independent package for cabal-helper to hie-bios cradle conversion? Does that already count as a plugin API?

ndmitchell commented 4 years ago

Is the cabal-helper to hie-bios cradle something ghcide has to mix in? I don't like that idea. Or something that hie-bios always links in? If so, I don't see the advantage of separating the package.

fendor commented 4 years ago

No, currently only hie uses it and hls copied the code to https://github.com/haskell/haskell-language-server/blob/master/src/Ide/Cradle.hs It is an alternative implementation to using the implicitCradle function. It is optional and would not affect ghcide at all, unless ghcide decides to use cabal-helper as well.

ndmitchell commented 4 years ago

I think hls and ghcide should converge on this as much as possible.

fendor commented 4 years ago

Absolutely agreed. I think, somewhere it was mentioned that you agreed in bristol to use cabal-helper? Should there be an attempt to use cabal-helper in ghcide? EDIT: https://github.com/haskell/haskell-ide-engine/issues/1416#issuecomment-578409687

... that hie-bios has good implicit-project support, probably making use of cabal-helper.

jneira commented 4 years ago

@ndmitchell just in case it would be solvable, what are your concerns about link statically cabal-helper in ghcide? is it that it depends on the Cabal library or another problematic lib? or the runtime compilation it triggers?

If hls uses cabal helper directly it will need to pass the cradle generated by c-h to ghcide (the component that actually is using it directly). Not sure if ithat is already possible in ghcide or it should be changed to accept "external" cradles.

Obviously if we add support in ghcide both would have the implicit cradle available. Imo it would be the desirable option over the former one.

Other options i can think of:

ndmitchell commented 4 years ago

I've heard cabal-helper takes a lot of work to support. At the moment, if there's an issue with the GHC flags, I go to hie-bios because the contract is hie-bios provides the flags to start a GHC session. I like the abstract interface "give me the flags" and the support model "ping Matthew et al". It's as much lines of communication and separation of responsibilities as it is code. If you guys want to shove cabal-helper behind the same interface and support model, provided there is no GPL impact, I'm happy (and moreover, consider it none of my business).

fendor commented 4 years ago

That is exactly how we integrated cabal-helper. It is hidden within a Cradle and you just query it, like any other cradle. Granted, the mechanism is not as light-weight as the implicit cradle discovery in hie-bios, but we have no extra logic in hie to support cabal-helper. That's why usage of cabal-helper is completely isolated in this one module.

alanz commented 4 years ago

FYI https://github.com/haskell/haskell-language-server/blob/master/src/Ide/Cradle.hs

alanz commented 4 years ago

Which should end up wherever we agree

jneira commented 4 years ago

Well i think the consesus had been using the cabal-helper cradle in haskell-language-server, cause cradle setup is in the main exe module owned by h-l-s: https://github.com/haskell/haskell-language-server/issues/38#issuecomment-582789371 @fendor could we close this or you have some concerns about?

EDIT: well, a concern is we have to port manually each change in the cabal-helper cradle between hie and hls

fendor commented 4 years ago

I still think outsourcing it into different package does no harm and makes it easier to support both hls and hie. As I read it, there was no argument against it?

jneira commented 4 years ago

@fendor no other than the usual overhead of maintain a separate lib: github repo, maybe upload to hackage/stackage, etc and the fact hie will be deprecated at some point. Otoh it could be used in another projects. If you are fine setting up the new package i am too :wink:

jneira commented 4 years ago

Other option could be integrate the cabal-helper cradle in cabal-helper itself, if @DanielG agree it is a good idea add hie-bios as dependency.

fendor commented 4 years ago

I think, for now I would prefer it as a separate library. I feel like it does not really belong into cabal-helper. However, lets wait for @DanielG opinion :)