haskell-language-server: make linking configureable in wrapper#158669
haskell-language-server: make linking configureable in wrapper#158669maralorn merged 4 commits intoNixOS:haskell-updatesfrom
Conversation
haskell-language-server will now default to building a shared
executable, as upstream does, complete with a huge closure. By passing
{ dynamic = false; } via override, it is still possible to build a
"statically linked" variant of HLS, as it used to be.
Note: Before this change HLS would fail to compile on aarch64.
github-actions
bot
added
the
6.topic: haskell
|
@ofborg build haskell-language-server |
ofborg
bot
added
10.rebuild-darwin: 1-10
|
@sternenseemann I am sorry I haven‘t followed the platform specific and the linking problems to closely. It was my impression that hls did (except for ghc 9.2) build on aarch64. Are there upsides for the other platforms as well from dynamic linking? I hadn‘t noticed any issues, even with template haskell. I am honestly not sure that this wrapper is even worth it or the recommended install method any more when it has a 5 GB closure. That’s a huge downside. (Seriously, that feels like something we loose Haskell adoption to … Some people are really scared of big closures.) Maybe we need to consider to changing the default here? Alternatively are there any downsides to enable dynamic linking in haskellPackages? |
|
This is not platform specific at all (there seems to be some platform specific component which hardly matters though). What happened is that starting with HLS 1.6.1.1 upstream will enable dynamic linking by default. I've so far just reflected the default in nixpkgs in order to keep builds working. The closure size is a concern of course, and I agree that we should probably default to |
I think this is relevant if you use e. g. TH and system libraries, as without dynamic linking the (partially buggy, especially on darwin) GHC internal linker is used instead of the system one. |
This means we don't use the system linker by default, but decreases closure size significantly, as we no longer reference every supported version GHC.
Also, this is already enable in |
Do I understand correctly that upstream changed the default in their cabal file and thus this changed automatically in haskellPackages? Does this mean that the wrapper package is currently broken on master because it uses justStaticExecutables on a dynamically linked build? |
|
I read a bit backlog in the hls channel. Apparently no one is really convinced that dynamic linking will solve an actual problem, but they do think so. Sadly, I don’t believe that users will know, that they need to open an issue, against nixpkgs, when they encounter an issue like this. My gut feeling is that the correct way forward would be to reduce the wrapper to the current default ghc, enable dynamic linking by default and document this accordingly. To get better caching in that case: I wonder: Do we need the remove references part if the closure size is screwed anyways? |
Probably at least the ghc one should be removed as it would probably cause some level of incorrectness. Not sure about the data output ones (why are they even there?). |
I just remember that those references happened in some kind of documentation and were therefore irrelevant for code correctness. |
If we are linking dynamically, it's practically no use removing references, as we depend on GHC either way via linking. I've also elected to keep the references to the data outputs in all cases — they are a bit arcane (there's no easy way to tell they definitely are not necessary) and don't contribute too much to the overall closure size.
|
See a386d54. |
|
I‘d be delighted to hear, if anyone is experiencing any TH+hls+linking issues. I am on the fence if we should enable dynamic linking by default. Still this PR is strict improvement. Thank you ver much! |
|
As it stands |
@maralorn I spent a while trying to figure out why TH wasn't working in VSCode, and ended up here after seeing TH mentioned in the HLS troubleshooting guide. Many thanks to @sternenseemann for the previous comment explaining how to install with dynamic linking, which seems to have fixed the problem. 🎉 |
|
@gloaming Thanks for the feedback. Then I guess we should enable this by default? But yeah, then we should restricted the wrapper to only the default version and document this accordingly. |
|
I don't think it's feasible to have it on by default as it'll necessarily pull in 4 GHCs (RTS is linked dynamically). |
|
Well then we should disable pulling in 4 GHCs by default? |
|
HLS actually displays a message about it on startup, in theory we could patch that to link to instructions for NixOS or check if they link anywhere and try getting upstream to amend the instructions. |
|
Hm, is that message intrusive enough? I am pretty sure I never saw it. But yeah, a documentation based fix would be nice, I guess. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Just wanted to add why I think this was a good change. I wasted many hours if not days because dynamic linking not being on messed up TH. It negatively affected my experiment/proposal to use Nix at work. Plus I think "it works for common cases like TH" is more common than "I don't want a 5GB closure". Haskell users, especially Haskell on Nix users are used to using 100s of gigabytes of space for better or worse. At least... I am given I have 20-40 direnv environments across various ghc versions, some also with different hls versions. |
5 participants
haskell-language-server will now default to building a shared
executable, as upstream does, complete with a huge closure. By passing
{ dynamic = false; } via override, it is still possible to build a
"statically linked" variant of HLS, as it used to be.
Note: Before this change HLS would fail to compile on aarch64.
Motivation for this change
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes