Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

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

Open
fendor opened this issue Feb 3, 2020 · 18 comments
Open

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

fendor opened this issue Feb 3, 2020 · 18 comments
Labels
type: discussion type: refactor Refactor and tidy up internals.

Comments

@fendor
Copy link
Collaborator

fendor commented Feb 3, 2020

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
Copy link
Member

jneira commented Feb 4, 2020

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
Copy link
Member

jneira commented Feb 4, 2020

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
Copy link

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
Copy link
Collaborator Author

fendor commented Feb 4, 2020

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
Copy link

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
Copy link
Collaborator Author

fendor commented Feb 4, 2020

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
Copy link

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

@fendor
Copy link
Collaborator Author

fendor commented Feb 4, 2020

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: #1416 (comment)

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

@jneira
Copy link
Member

jneira commented Feb 4, 2020

@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:

  • discover another, more lightweight way to generate a reliable implicit cradle
  • make hie-bios use cabal-helper at runtime (i.e. calling c-h as a executable), creating another cradle type (implicit?). Not sure if it is possible/viable

@ndmitchell
Copy link

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
Copy link
Collaborator Author

fendor commented Feb 4, 2020

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
Copy link
Collaborator

alanz commented Feb 4, 2020

@alanz
Copy link
Collaborator

alanz commented Feb 4, 2020

Which should end up wherever we agree

@jneira
Copy link
Member

jneira commented Feb 27, 2020

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: haskell/haskell-language-server#38 (comment)
@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

@jneira jneira added type: discussion type: refactor Refactor and tidy up internals. labels Feb 27, 2020
@fendor
Copy link
Collaborator Author

fendor commented Feb 27, 2020

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
Copy link
Member

jneira commented Feb 27, 2020

@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 😉

@jneira
Copy link
Member

jneira commented Feb 27, 2020

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
Copy link
Collaborator Author

fendor commented Feb 27, 2020

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 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: discussion type: refactor Refactor and tidy up internals.
Projects
None yet
Development

No branches or pull requests

4 participants