Add getRuntimeGhcLibDir and getRuntimeGhcVersion, via runGhc #207

lukel97 · 2020-06-14T14:50:44Z

This PR moves the logic in ghcide/hls of getting the runtime ghc library directory and version into hie-bios, and in doing so moves the responsibility to the cradle

lukel97 · 2020-06-14T14:53:26Z

Closes #151

fendor

I like the design!
Since this is a big breaking change that is very important to get right, we will wait for some feedback before merging it.

tests/BiosTests.hs

src/HIE/Bios/Cradle.hs

fendor · 2020-06-14T19:51:55Z

src/HIE/Bios/Cradle.hs

+        , getGhcPath =
+            -- We're being lazy here and just returning the ghc path for the
+            -- first non-none cradle. This shouldn't matter in practice: all
+            -- sub cradles should be using the same ghc version!


This should probably be "must"

Not sure about that, the multi combinations could give us as first ghc a wrong one. F.e. if you use a direct cradle it can give us the ghc on path but stack or cabal the ghc configured for the project.
However it is probable that the direct cradle could work with the build tool one but not the other way around.
Would be not feasible return a list of unique ghc's and let the client code pick the first one or handle the list in other way? i can imagine you could choose the newer ghc, f.e.

The ghc libdir should be the same ghc version for each sub-cradle though. This would mean it doesn't matter which one is used to load the ghc session
The only case where it would make a difference would be if two sub-cradles actually had different ghc versions, and then returned two different ghc libdirs of different ghc versions.
But then in that case ghcide/hls wouldn't be able to load them both anyway!

I guess we could return a list anyway for ghcide/hls and they could just called head. But I think it would be nice to handle as much of this stuff in hie-bios as possible

Uh i completly missed that comment, sorry
Actually i am not sure if return a list without knowing which cradle is the source of each ghc version is a good idea.
I dont have an alternative that it is simple, maybe return an error or warning if not all ghcs are the same version? 🤔

If you think that this could be merged as is, it will be fine for me too

fendor · 2020-06-14T19:53:28Z

src/HIE/Bios/Environment.hs

+-- hie-bios compiled on, so we avoid using it unless we have to!
+getGhcLibDir :: Cradle a -> IO FilePath
+getGhcLibDir cradle =
+  fmap (fromMaybe Paths.libdir) $ runMaybeT $ fromNix <|> fromCradle


Do we know what should take precedence?
In my experience with nix, ghc is wrapped to give the correct libdir.
The order should be in my opinion: Cradle Ghc -> Nix env var -> Paths.libdir

As ghc-paths will be wrong in the general case maybe we should remove it and returns IO (Maybe Filepath) to make clear we cant return it for all cases in a reliable way and force the client code handle that possibility (maybe using ghc-paths at the end!)

My aim here was to mirror what currently happens inside ghcide/hls, and I presume this nix env var might be used for the direct cradles where it's calling ghc without cabal or stack: https://github.com/digital-asset/ghcide/blob/9e984b56cfb62a83829975210ce3f14c668912f6/docs/Setup.md#issues-with-nix

If the client code is just going to end up calling ghc-paths then shouldn't we just do it here as well? We might want to rename getGhcLibDir to something more honest though like mostlyAccurateGuessGhcLibDir, since the only scenario where ghc-paths doesn't work will be when moving binaries across machines. On the other hand, if we are using static binaries then we would really want to error here instead of silently giving the wrong path. But then we would end up splitting the libdir logic even further back into the clients

Well i suppose client code could do a final sanity check executing the ghc returned by the cradle (--numeric-version ?) and catch the error. Not elegant but doable

@jneira getGhcOnPathLibDir should now do a check of the ghc version beforehand now

fendor · 2020-06-14T19:55:41Z

src/HIE/Bios/Ghc/Api.hs

@@ -32,29 +31,25 @@ import HIE.Bios.Flags
 ----------------------------------------------------------------

 -- | Converting the 'Ghc' monad to the 'IO' monad. All exceptions are ignored and logged.
-withGHC :: FilePath  -- ^ A target file displayed in an error message.
+withGHC :: Cradle c  -- ^ The cradle to use for resolving the lib dir


Not entirely sure I like this API. The ghc session is not dependent on the cradle, only on the libdir.
To me, this usage sends a signal that the cradle is somehow used in the session and needs to be known before launching the session. (my opinion is debatable here)
(btw, neither ghcide/hls nor haskell-ide-engine uses it)

Ok, I'm happy enough to change this to the libdir, and then the users of this API can just run the cradle themselves to extract it

Come to think of it, if we make the user pass in the libdir then there's no point really in having withGhcT, since all it did was handle the libdir part

I'd also advocate for just removing withGHC as well, since it's not used by ghcide/hls and is a carryover from ghc-mod

Yeah, I agree, let's remove it.

src/HIE/Bios/Ghc/Check.hs

jneira · 2020-06-14T21:25:27Z

src/HIE/Bios/Cradle.hs

+        , getGhcPath =
+            -- We're being lazy here and just returning the ghc path for the
+            -- first non-none cradle. This shouldn't matter in practice: all
+            -- sub cradles should be using the same ghc version!


Not sure about that, the multi combinations could give us as first ghc a wrong one. F.e. if you use a direct cradle it can give us the ghc on path but stack or cabal the ghc configured for the project.
However it is probable that the direct cradle could work with the build tool one but not the other way around.
Would be not feasible return a list of unique ghc's and let the client code pick the first one or handle the list in other way? i can imagine you could choose the newer ghc, f.e.

src/HIE/Bios/Cradle.hs

jneira · 2020-06-14T21:45:54Z

src/HIE/Bios/Cradle.hs

@@ -326,6 +340,7 @@ biosCradle wdir biosCall biosDepsCall =
    , cradleOptsProg   = CradleAction
        { actionName = Types.Bios
        , runCradle = biosAction wdir biosCall biosDepsCall
+        , getGhcPath = findExecutable "ghc"


Afaik the dependencies of the bios cradle is a responsability of the bios program (see biosDeps): should not be the ghc used by that bios program returned by itself? Or at least being able to take in account it and fallback to the ghc on path.

Is there a way for a bios program to return the path to the GHC it intends to use? From my understanding it just returns newline separated arguments in stdout

It may still want define the libdir, right?

I'm not sure if there will be a good way to have the bios program pass the library directory back, seeing as programs are only really aware of filepaths being passed to them, either through the shell arguments or by the HIE_BIOS_ARG environment variable. Also I can't really imagine how the bios programs are going to return the library directory in the first place.
With that said, the current way haskell-language-server works out the project version is by calling ghc on the path anyway, and this should fall back to the hardcoded ghc-paths should that fail. So I don't think there would be much of a difference.

Well, an option could be add another optional config option like the existing one for dependencies:

cradle: bios: program: ./flags.sh dependency-program: ./dependency.sh ghc-path-program: ./ghc-path.sh

But i am not aware of a bios program that needs it so i would merge this without it and add it on concrete user request afterwards

jneira · 2020-06-14T21:50:49Z

@bubba thanks a lot for this, not only it is necessary for the demanded precompiled ide binaries but i think it is a great feature for hie-bios standalone.
As @fendor noted we have to be specially careful, sorry if i have been too fuzzy 😅

src/HIE/Bios/Cradle.hs

jneira

I've tested in windows with some cabal and stack projects and it has worked in all cases.
Really a great job.

EDIT: ~~Well, that was difficult but the result for ghc and its bios cradle is strange:~~

GHC library directory: Just "D:\\bin\\stack\\x86_64-windows\\ghc-8.6.5\\lib"

(ghc on path is 8.8.3 and hie-bios was installed with 8.10.1)

EDIT2: I've jut run it again in ghc and i've ot another result so surely i did something wrong

PS D:\dev\ws\haskell\ghc> hie-bios debug .\ghc\Main.hs
Root directory:        D:\dev\ws\haskell\ghc
Component directory:   D:\dev\ws\haskell\ghc
GHC options:           "-v "
GHC library directory: Nothing
Config Location:       D:\dev\ws\haskell\ghc\hie.yaml
Cradle:                Cradle {cradleRootDir = "D:\\dev\\ws\\haskell\\ghc", cradleOptsProg = CradleAction: Bios}
Dependencies:

lukel97 · 2020-06-25T21:11:06Z

For posterity, the issue with hie-bios failing on the ghc source tree itself was due to the sanity check that the ghc versions matched when fetching the library directory for GHC on the path. We just removed this in the end, since we now have a separate function for checking the project version.

And upon further reflection, I think hie-bios shouldn't try to act smart here – the runtime ghc version for a bios or plain/default cradle should always just consult ghc on the PATH. Not ghc-8.x.y with whatever version hie-bios was compiled with.
This will only get confusing, and the version of what hie-bios was compiled with really shouldn't determine the version of ghc selected since we will be using this logic in haskell-langugage-server-wrapper, which is compiled with an arbitrary ghc version!

jneira · 2020-06-25T21:17:43Z

Not ghc-8.x.y with whatever version hie-bios was compiled with.

Yeah, moreover now it is named getRuntimeGhcLibDir, i would remove the fromGhcPaths alternative

This is part of the work towards getting static binaries properly working in ghcide and haskell-language-server. To do this we need to be able to fetch the ghc library directory on the fly, as it will change with each ghc installation. getRuntimeGhcLibDir is what client code should use to obtain the libdir, and searches three places, falling back in this order: 1. the NIX_GHC_LIBDIR environment variable 2. ghc --print-libdir for whatever ghc the cradle is using 3. the libdir baked into ghc-paths We want to avoid using ghc-paths if possible since it bakes the path into the binary, which means it isn't portable in the static binary sense. So if True is passed for the second argument, it will avoid falling back on ghc-paths, which can be useful when distributing static binaries etc. From the library directory we can also get the path to the ghc binary used, which we can then use to get the cradle ghc version via getRuntimeGhcVersion

lukel97 · 2020-06-26T20:40:38Z

Was trying to figure out why appveyor was failing with stack and ghc 8.10.1 so I set up GitHub CI to see if would make a difference but it didn't. I think there's something funny going on with that combination anyway so I've just commented out those tests again. I've left in the GitHub CI anyway, should be handy

fendor

Looking good!

Needs a follow-up PR so that bios cradle can also specify its libdir somehow.

tests/BiosTests.hs

Now we can just call `cabal exec ghc -- --numeric-version` directly without the need for guessing where the ghc binary lies.

Since the functions are now "getRuntimeGhc*", it doesn't make sense to try and use anything compiletime related

This avoids clashes with runGhc exported from GHC

lukel97 force-pushed the ghc-path-libdir branch from 3bd352f to 0383ff1 Compare June 14, 2020 14:52

This was referenced Jun 14, 2020

Expose location of the ghc libdir in cradle. #151

Closed

Distributable binaries and install-less VS Code extension haskell/haskell-language-server#158

Closed

lukel97 force-pushed the ghc-path-libdir branch from 0383ff1 to 33d2144 Compare June 14, 2020 17:31

fendor requested review from mpickering and ndmitchell June 14, 2020 19:46

fendor reviewed Jun 14, 2020

View reviewed changes

jneira requested changes Jun 14, 2020

View reviewed changes

jneira reviewed Jun 14, 2020

View reviewed changes

lukel97 force-pushed the ghc-path-libdir branch 5 times, most recently from 6df34cb to 12f4f2f Compare June 16, 2020 12:39

lukel97 commented Jun 16, 2020

View reviewed changes

src/HIE/Bios/Cradle.hs Outdated Show resolved Hide resolved

lukel97 force-pushed the ghc-path-libdir branch from 12f4f2f to 6e45b12 Compare June 18, 2020 19:54

lukel97 changed the title ~~Add getGhcPath and getGhcLibDir~~ Add getRuntimeGhcLibDir and getRuntimeGhcVersion Jun 21, 2020

jneira self-requested a review June 23, 2020 22:22

jneira approved these changes Jun 24, 2020

View reviewed changes

lukel97 force-pushed the ghc-path-libdir branch from 6e45b12 to 64b02c9 Compare June 25, 2020 19:44

lukel97 force-pushed the ghc-path-libdir branch from 64b02c9 to 2628fe7 Compare June 25, 2020 21:11

lukel97 force-pushed the ghc-path-libdir branch from 4b6d9a1 to fdea533 Compare June 25, 2020 22:21

lukel97 changed the title ~~Add getRuntimeGhcLibDir and getRuntimeGhcVersion~~ Add getRuntimeGhcLibDir and getRuntimeGhcVersion, via runGhc Jun 25, 2020

lukel97 force-pushed the ghc-path-libdir branch 2 times, most recently from 29d2d3a to 126bfd8 Compare June 26, 2020 12:39

lukel97 force-pushed the ghc-path-libdir branch 6 times, most recently from 45577a8 to 482d8af Compare June 26, 2020 20:26

lukel97 force-pushed the ghc-path-libdir branch from 482d8af to 97f0bea Compare June 26, 2020 21:12

fendor approved these changes Jun 28, 2020

View reviewed changes

tests/BiosTests.hs Outdated Show resolved Hide resolved

tests/BiosTests.hs Outdated Show resolved Hide resolved

lukel97 mentioned this pull request Jun 28, 2020

How should bios cradles run ghc? #215

Closed

lukel97 added 3 commits June 28, 2020 16:06

Abstract out runGhcLibDir into runGhc

36fae3e

Now we can just call `cabal exec ghc -- --numeric-version` directly without the need for guessing where the ghc binary lies.

Remove ghc-paths

4b695f2

Since the functions are now "getRuntimeGhc*", it doesn't make sense to try and use anything compiletime related

Add GitHub Actions CI

a187409

lukel97 force-pushed the ghc-path-libdir branch from 97f0bea to a187409 Compare June 28, 2020 15:12

fendor mentioned this pull request Jun 29, 2020

Add subcradle for Setup.hs haskell/haskell-language-server#179

Closed

Rename runGhc to runGhcCmd

a013b2a

This avoids clashes with runGhc exported from GHC

fendor merged commit c786a73 into haskell:master Jul 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add getRuntimeGhcLibDir and getRuntimeGhcVersion, via runGhc #207

Add getRuntimeGhcLibDir and getRuntimeGhcVersion, via runGhc #207

lukel97 commented Jun 14, 2020 •

edited

Loading

lukel97 commented Jun 14, 2020

fendor left a comment

fendor Jun 14, 2020

jneira Jun 14, 2020

lukel97 Jun 16, 2020

jneira Jun 23, 2020 •

edited

Loading

fendor Jun 14, 2020

jneira Jun 14, 2020

lukel97 Jun 15, 2020

lukel97 Jun 15, 2020

jneira Jun 15, 2020

lukel97 Jun 18, 2020

fendor Jun 14, 2020

lukel97 Jun 15, 2020

lukel97 Jun 15, 2020 •

edited

Loading

lukel97 Jun 15, 2020

fendor Jun 15, 2020 •

edited

Loading

jneira Jun 14, 2020

jneira Jun 14, 2020

lukel97 Jun 15, 2020

fendor Jun 15, 2020

lukel97 Jun 23, 2020

jneira Jun 23, 2020

jneira commented Jun 14, 2020 •

edited

Loading

jneira left a comment •

edited

Loading

lukel97 commented Jun 25, 2020 •

edited

Loading

jneira commented Jun 25, 2020

lukel97 commented Jun 26, 2020

fendor left a comment

Add getRuntimeGhcLibDir and getRuntimeGhcVersion, via runGhc #207

Add getRuntimeGhcLibDir and getRuntimeGhcVersion, via runGhc #207

Conversation

lukel97 commented Jun 14, 2020 • edited Loading

lukel97 commented Jun 14, 2020

fendor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneira Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukel97 Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fendor Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneira commented Jun 14, 2020 • edited Loading

jneira left a comment • edited Loading

Choose a reason for hiding this comment

lukel97 commented Jun 25, 2020 • edited Loading

jneira commented Jun 25, 2020

lukel97 commented Jun 26, 2020

fendor left a comment

Choose a reason for hiding this comment

lukel97 commented Jun 14, 2020 •

edited

Loading

jneira Jun 23, 2020 •

edited

Loading

lukel97 Jun 15, 2020 •

edited

Loading

fendor Jun 15, 2020 •

edited

Loading

jneira commented Jun 14, 2020 •

edited

Loading

jneira left a comment •

edited

Loading

lukel97 commented Jun 25, 2020 •

edited

Loading