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

Jump to bottom

haskell-language-server: Init wrapper for multiple ghc versions at 0.5.0 #99519

Merged
merged 5 commits into from Oct 10, 2020
Merged

haskell-language-server: Init wrapper for multiple ghc versions at 0.5.0 #99519

merged 5 commits into from Oct 10, 2020

Conversation

maralorn
Copy link
Member

@maralorn maralorn commented Oct 4, 2020 •
edited

Motivation for this change

haskell-language-server needs a binary which is compiled with the ghc the user uses for their project. That's why hls normally get's distributed (e.g. via ghcup) with the haskell-language-server-wrapper that picks the right binary based on the current project and a binary for every supported ghc version.
I want hls support in nixpkgs to be as good as that, so we also need to provide all those binaries.

I am introducing pkgs.haskell-language-server here which has a overridable supportedGhcVersions list.

I have fixed the closure size with reference-to. A glance at the binary made a reasonable impression that those references aren‘t needed. A quick test of the binary seems to do just fine.

\cc @peti @GuillaumeDesforges @cdepillabout

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@maralorn maralorn marked this pull request as ready for review October 4, 2020 23:14
@maralorn
Copy link
Member Author

maralorn commented Oct 4, 2020

I have reduced the closure size to a reasonable ~330 MB. One hls binary is about 95 MB.

I am eager for feedback.

cdepillabout
cdepillabout approved these changes Oct 5, 2020
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

I don't use haskell-language-server, but this looks pretty good to me.

If I could make one suggestion, it would be to have some documentation in development/tools/haskell/haskell-language-server/withWrapper.nix that gave a short explanation of how this is supposed to be used.

For instance, if I was new to nixpkgs, my first thoughts would be:

  • How do I use haskell-language-server from nixpkgs?
  • Is it sufficient to do nix-shell -p cabal -p ghc -p haskell-language-server and just start using it? Or is there some other sort of setup I need?
  • It appears that haskell-language-server installs different versions, so will it basically just automatically work with different ghc versions? Or do I have to do something so this works? If I want haskell-language-server for a different ghc version, do I have to override the haskellPackages argument to development/tools/haskell/haskell-language-server/withWrapper.nix?

I guess it would be nice to have this in the official documentation as well, but personally I'd be fine with just documenting it here in the code.

@maralorn
Copy link
Member Author

maralorn commented Oct 5, 2020

Yeah, there is definitely documentation missing. I actually would go all in and add a section to nixpkgs manual. First I need to learn how to do that.

@Cmdv Cmdv mentioned this pull request Oct 6, 2020
Closed
10 tasks
@peti peti force-pushed the haskell-updates branch 3 times, most recently from be2e4fb to 1436509 Compare October 9, 2020 19:39
@maralorn maralorn force-pushed the hls-wrapper branch 2 times, most recently from 4a4d0cb to c26ce5e Compare October 9, 2020 22:17
@maralorn
Copy link
Member Author

maralorn commented Oct 9, 2020

I have added a documentation section. Would love some feedback if it's all clearly understandable.

@maralorn
Copy link
Member Author

I could have add 9.0.1 but I want to do that in a subsequent PR, because right know tons of packages are broken on 9.0.1.

cdepillabout
cdepillabout approved these changes Oct 10, 2020
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

Easy to understand! Looks good!

@maralorn maralorn merged commit 0756b8a into NixOS:haskell-updates Oct 10, 2020
@maralorn maralorn deleted the hls-wrapper branch October 10, 2020 14:02
@GuillaumeDesforges GuillaumeDesforges mentioned this pull request Oct 10, 2020
Closed
@roosemberth roosemberth mentioned this pull request Mar 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdepillabout cdepillabout cdepillabout approved these changes

Assignees
No one assigned
Labels
6.topic: haskell 8.has: documentation 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@maralorn @cdepillabout