Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distributable binaries #165

Merged
merged 58 commits into from
Jul 20, 2020
Merged

Distributable binaries #165

merged 58 commits into from
Jul 20, 2020

Conversation

lukel97
Copy link
Collaborator

@lukel97 lukel97 commented Jun 18, 2020

Ignore the commits for now, they should be squashed when merged!

This PR contains a lot of stuff, a lot of it coming from upstream components so it might not be visible in the diff:

  • Update ghcide to obtain the GHC lib dir at runtime, rather than at compile time with ghc-paths. This means that the binaries can be moved about since the lib dir is obtained on the fly
  • Share the exe/main.hs logic between ghcide and hls: the session setup logic which previously took up most of exe/main.hs now resides inside the ghcide library, and is used by both ghcide and hls's executables
  • Add a --project-ghc-version option to the wrapper which spits out the project's ghc version to stdout. This is useful for the vscode extension which can then use it to download the corresponding version of binary that the wrapper would have otherwise attempted to launch
  • Make the wrapper check to see if the correct tool is installed beforehand. For example, if it detects a stack project but stack isn't on the path, it will report an error then and there, rather than having hls/ghcide confusingly fail later on. The vscode extension uses this new error message as well to provide a pop up message linking the user to a website to install the missing tool
  • Remove cabal-helper from the wrapper, so that the implicit cradle logic is the same between ghcide/hls/hls-wrapper
  • And of course, add a GitHub action workflow that runs whenever a release is created on GitHub that builds static binaries on Linux, and distributable enough binaries on macOS and windows. This is documented a bit more in docs/releases.md

@fendor
Copy link
Collaborator

fendor commented Jun 18, 2020

Figure out how to implement runGhcLibDir for the cabal helper cradles

Practically, CabalHelper is only used in HLS-wrapper. In my opinion, we may remove it if it is not really used.

Also, there is the implementation of https://github.com/haskell/haskell-language-server/blob/master/src/Ide/Cradle.hs#L158 which obtained the libdir in Haskell IDE Engine. That should work, right?

@lukel97 lukel97 force-pushed the github-action-builds branch from 409ca9c to 773ec86 Compare July 16, 2020 16:30
@lukel97 lukel97 force-pushed the github-action-builds branch from 43fb062 to 53c3676 Compare July 16, 2020 16:48
@lukel97 lukel97 marked this pull request as ready for review July 16, 2020 17:04
@lukel97 lukel97 requested review from alanz, jneira, fendor and Ailrun and removed request for alanz and jneira July 16, 2020 17:05
@lukel97
Copy link
Collaborator Author

lukel97 commented Jul 16, 2020

The stuff that needs to go into ghcide has PRs open here, but has been cherry-picked into the ghcide submodule for now haskell/ghcide#697 haskell/ghcide#696

Copy link
Collaborator

@alanz alanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok provided @wz1000 and co are happy

@@ -13,4 +13,5 @@
# url = https://github.com/digital-asset/ghcide.git
# url = https://github.com/alanz/ghcide.git
# url = https://github.com/wz1000/ghcide.git
url = https://github.com/fendor/ghcide.git
# url = https://github.com/fendor/ghcide.git
url = https://github.com/bubba/ghcide.git
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the correct upstream repo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until those other PRs and @wz1000s PRs get merged into ghcide, I think so. This checkout is just @fendor's previous submodule + my patches cherry-picked on top

docs/releases.md Outdated
@@ -0,0 +1,72 @@
# Releases and distributable binaries

Starting with 0.X.0.0 haskell-language-server provides pre-built binaries on
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine 0.3.0.0, if we merge soon.

amesgen and others added 4 commits July 16, 2020 19:55
The cabal-helper cradle was only used by the wrapper for detecting the
project GHC version in the absence of an explicit hie.yaml file, whilst
ghcide itself used the hie-bios implicit cradle logic. This brings the
two in sync so the wrapper should behave more predictably now.
@lukel97 lukel97 changed the title GitHub Action static binaries and runtime libdir Distributable binaries Jul 17, 2020
@lukel97
Copy link
Collaborator Author

lukel97 commented Jul 17, 2020

Now that the tests appear to be passing, review/test away @fendor @jneira etc.

@jneira
Copy link
Member

jneira commented Jul 17, 2020

I hope we will not miss cabal-helper in the future (although for missing something you need remembering it 😝 )

@@ -1,862 +0,0 @@
{-# LANGUAGE ScopedTypeVariables #-}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here rests my biggest contribution to HIE.
RIP

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most beautifully documented module ⚰️

Comment on lines +104 to +123
-- | Find the cradle that the given File belongs to.
--
-- First looks for a "hie.yaml" file in the directory of the file
-- or one of its parents. If this file is found, the cradle
-- is read from the config. If this config does not comply to the "hie.yaml"
-- specification, an error is raised.
--
-- If no "hie.yaml" can be found, the implicit config is used.
-- The implicit config uses different heuristics to determine the type
-- of the project that may or may not be accurate.
findLocalCradle :: FilePath -> IO (Cradle Void)
findLocalCradle fp = do
cradleConf <- findCradle fp
crdl <- case cradleConf of
Just yaml -> do
hPutStrLn stderr $ "Found \"" ++ yaml ++ "\" for \"" ++ fp ++ "\""
loadCradle yaml
Nothing -> loadImplicitCradle fp
hPutStrLn stderr $ "Module \"" ++ fp ++ "\" is loaded by Cradle: " ++ show crdl
return crdl
Copy link
Collaborator

@fendor fendor Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont we need something similar for exe/Main.hs? So, should be put into the library, also for writing tests against it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The corresponding line in Ghcide's exe/Main.hs (src/Development/Ide/Session.hs now) is

cradle <- maybe (loadImplicitCradle $ addTrailingPathSeparator dir) loadCradle hieYaml

but it looks like the hieYaml is cached in some memo thingy

  cradleLoc <- liftIO $ memoIO $ \v -> do
      res <- findCradle v
      -- Sometimes we get C:, sometimes we get c:, and sometimes we get a relative path
      -- try and normalise that
      -- e.g. see https://github.com/digital-asset/ghcide/issues/126
      res' <- traverse makeAbsolute res
      return $ normalise <$> res'

I would have liked to have shared that bit but I'm not 100% sure how to abstract it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that still used? I can imagine that we do not need to cache this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fendor After looking at it a bit closer I think the memoisation part does make sense, since I presume that shake action can get called/executed (not sure what the terminology is) multiple times on an individual file. And since findCradle does a lot of directory traversing we probably don't want to do that every single time

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small question, other than that, just awesome!

import HIE.Bios
import Ide.Cradle (findLocalCradle)
import Ide.Logger (logm)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the logger gone here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logger never worked in the first place from what I could tell, since the wrapper didn't call the haskell-lsp setup function! So all those logm/debugm calls seemed to just go down the drain.

@@ -0,0 +1,72 @@
# Releases and distributable binaries
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome Documentation!

@lukel97 lukel97 merged commit 0c99ce0 into haskell:master Jul 20, 2020
@jneira
Copy link
Member

jneira commented Jul 20, 2020

@bubba sorry, due to windows build issues i had no time to test it again. 😟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants