-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Implement distribution of dynamic builds #2675
Implement distribution of dynamic builds #2675
Conversation
10ca101
to
245951f
Compare
b5ebb1e
to
056f22d
Compare
Can you teach |
056f22d
to
504494c
Compare
Hmm... that's less flexible though. E.g. people can relocate their ghcup installation to a different directory. The current wrapper also doesn't really rely on ghcup. Additionally, many systems won't have the |
But I agree that there's a chance that lots of configurations won't have a valid/matching ghc binary in PATH... what ghcup could probably do is to set E.g. GHC_LIBDIR="$(ghcup whereis -d ghc ${GHC_VERSION})/../lib/ghc-${GHC_VERSION}/" |
This relies on the HLS wrapper to find and call the right HLS binary for the GHC version in scope. So I think we want the HLS wrapper to be fully statically linked, right? |
Well... |
I suppose it doesn't - if the dyn wrapper doesn't work, the dyn binary won't work either |
Trying the binaries in CentOS 8:
|
I built those on Fedora 34 |
This is why we distribute statically linked artifacts... |
Huh? This isn't an issue at all. GHC distributes dynamically linked bindists as well, including for CentOS and some other distros. Yes, this will require building more than one linux bindist. GHC CI has the code and infrastructure to do that. |
We could probably achieve the same thing with a matrix of containers in Github CI as well. To deploy them, the VSCode extension should simply delegate to ghcup instead of doing a direct download as it does now. The only thing missing here is a volunteer to drive this change. @hasufell what do you think? |
Partly. Gitlab CI provides all architectures (including apple M1, FreeBSD 12/13 etc.). IMO, it is much easier to do this in gitlab. The emulation actions in github CI don't cover apple M1 and otherwise have massive memory constraints that we don't have in gitlab. FreeBSD action is lacking as well.
Yeah |
With the latest patch I improved the libdir detection (that's needed to point the binary to the correct GHC shipped dynamic libraries), so it will also work with stack.
If someone invokes
I believe this is robust enough and most people will never experience a fallback beyond |
This setup is impressive and useful to help users get a hls binary which works for TH However it will make more complex distribution channels, adding the Linux flavour to the actual combination. I still hoped a better solution could come from upstream so I considered dynamic builds somewhat a temporary workaround and not sure if make deep changes will worth it |
Want to note we are providing 37 artifacts in the last release |
It doesn't really help users, unless we distribute the resulting tarballs. The main Makefile is not meant to be run by users.
Sure. But the current distribution is broken, no?
Even if that happens, it will only work for very new GHC versions. Which means by the time a solution appears, it'll be 3+ years until it actually benefits most users. |
yeah, it will support such distribution, the motivation of the whole thing
broken for some envs and no for others, today an user asked for a way to disable the warning cause th works for them out of the box
sure, as always, and, as usual we could ask users upgrade if they want full th support in precompiled binaries (they always will be able to build from source, and with the ghcup compile helping) |
Well, if you guys are against this approach, then I'd rather not invest more time into it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's get this rolling!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot going on here. It would be good to have a section in the documentation that explains what this is all for and why.
Can you also add yourself to CODEOWNERS for /bindist
and any other new stuff.
@@ -0,0 +1,116 @@ | |||
UNAME := $(shell uname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this somewhere other than the top-level, given that it really only exists to deal with the new bindist workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to... otherwise the Makefile has to find the repository root or CI has to copy it back. It really belongs to the top level dir.
@@ -18,6 +18,7 @@ and it is being used in nix environments. | |||
|
|||
### prerelease sanity checks | |||
|
|||
- [ ] set the supported GHC versions and their corresponding cabal project-files in `bindist/ghcs` according to the [GHC version deprecation policy](../supported-versions.md#ghc-version-deprecation-policy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is in the wrong section.
Is this really the only new step? Isn't there something that needs to be done with all these new mysterious makefiles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These makefiles are used by gitlab CI to build the new bindists. They will replace the static bindists. I don't really understand the release documentation, so I'd appreciate someone with more knowledge can adjust it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, you understand the new stuff better than anyone else :) So if you don't want to integrate it into the existing docs, perhaps you could write a standalone explanation and then we can process that. But I think you've got to write us something :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require follow-up PRs anyway to integrate better with the github actions workflows. I've never touched them so far, only the gitlab ones.
The static release builds via github actions will be obsolete and instead release artifacts will only be pulled from gitlab pipelines. That's not automated yet and might be done manually for now.
Alright, I'll try to re-iterate some of the discussion and motivation around this PR. The problems
What this PR does
Drawbacks
Alternatives
|
We don't need to guess the bin/ dir. Instead we only need ghc libdir (which we already have) and the full path to the ghc binary to reconstruct the ghc wrapper.
1e5c716
to
ae442fb
Compare
And improve debug messages.
8d00d7f
to
db3bf2f
Compare
All green, let's go https://gitlab.haskell.org/maerwald/haskell-language-server/-/pipelines/47336 |
This PR seems to have broken hls installation via ghcup from source. I'm installing hls using Now when open my project in vscode, I'm getting this hls crash repeatedly:
The pattern match failure error points to this line haskell-language-server/exe/Wrapper.hs Line 117 in 54de5a7
Am I doing something wrong, or should I file new issue for this? |
I can't reproduce on my machine. Tried with both stack and cabal. I guess we need to fix the error reporting to figure out what's going on. |
@jhrcek I think we should open a new issue about it, hard to discover here |
This reverts commit a96b623.
A generated tarball can be tested here: https://downloads.haskell.org/ghcup/tmp/hls-dyn/
Building tarball
This needs
patchelf
and probably GNUmake
installed.This reads the file
bindist/ghcs
to determine what ghcs to build for.Assuming the
-dynamic
patch is already applied to the cabal file.Install
Install via e.g.
make PREFIX=$HOME/.ghcup/hls/ install
or so. then run~/.ghcup/hls/bin/haskell-language-server-8.10.7 --help
.How
*.so
libraries are copied out of that directory into alib/ghc-<ver>
subdirbin
subdirpatchelf
to set rpath to$ORIGIN/../lib/ghc-<ver>
, similar to what GHC bindists toghc --print-libdir
and adding all subdirs of it toLD_LIBRARY_PATH
in a wrapper scriptSo this should work with an arbitrary system-installed GHC that is in
PATH
. The ghc version isn't checked explicitly, but could.