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

haskell-language-server, stylish-haskell: Fix build on GHC 8.10.* #97744

Merged
merged 6 commits into from Sep 11, 2020

Conversation

Galagora
Copy link
Contributor

@Galagora Galagora commented Sep 11, 2020

Motivation for this change

Haskell Language Server uses a fork of brittany that compiles on GHC 8.10.*. In addition, stylish-haskell forgot to update their Cabal version bounds in the test suite, causing a hard-to-debug build failure.

Closes #97743.
Closes #97745.

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.

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.

Could you change this to target the haskell-updates branch instead of master?

Other than the branch and the the one suggestion below, this looks good to me!

cc @maralorn and @GuillaumeDesforges

Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges left a comment

Choose a reason for hiding this comment

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

LGTM

}:
mkDerivation {
pname = "ghcide";
version = "0.2.0";
version = "0.3.0";
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if that's an issue in itself, but having a single update.sh makes fixing or extending with other libs (like this PR) into one non-atomic commit. While nixpkgs is a bit blurry about its "atomic changes" policy, I think it is still a good idea to split package update and build fixes.

@cdepillabout do you consider it to not be an issue?
Should this PR be split into more atomic commits manually? (my preference)
Do we change the workflow by splitting the update scripts?
Anything else?

Copy link
Contributor Author

@Galagora Galagora Sep 11, 2020

Choose a reason for hiding this comment

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

I split out the commits and added a note to update.sh to remind others to do the same. I don't think the script should be split out; you'll probably want to test the binary with all the automated updates applied, and you'll probably want to commit only after the binary works.

Copy link
Member

Choose a reason for hiding this comment

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

I don't particularly worry about it that much. I agree that it is a good idea to have atomic changes, but there is some trade-off with making it unreasonably challenging for people to contribute.

If you don't split out your commits, I guess you run the risk of having a whole bunch of unrelated changes reverted if something in one of your commits needs to be reverted.

I would be fine with the HLS stuff all going into one commit, since it is all basically related anyway.

@Galagora Galagora force-pushed the hls-ghc-8102 branch 2 times, most recently from c3e7f4b to fb06974 Compare September 11, 2020 11:33
@Galagora Galagora changed the base branch from master to haskell-updates September 11, 2020 11:46
@Galagora
Copy link
Contributor Author

Sorry, that was a mistake...

Copy link
Member

@maralorn maralorn left a comment

Choose a reason for hiding this comment

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

LGTM

If this builds, this should be fine. Thanks!

@maralorn
Copy link
Member

maralorn commented Sep 11, 2020

@GrahamcOfBorg build haskellPackages.haskell-language-server
@GrahamcOfBorg build haskellPackages.stylish-haskell
@GrahamcOfBorg build haskell.packages.ghc8102.haskell-language-server
@GrahamcOfBorg build haskell.packages.ghc8102.hls-brittany

1 similar comment
@cdepillabout
Copy link
Member

@GrahamcOfBorg build haskellPackages.haskell-language-server
@GrahamcOfBorg build haskellPackages.stylish-haskell
@GrahamcOfBorg build haskell.packages.ghc8102.haskell-language-server
@GrahamcOfBorg build haskell.packages.ghc8102.hls-brittany

@maralorn
Copy link
Member

Can you have another look at those tests? There seems to be a wrong hash error going on?

@Galagora
Copy link
Contributor Author

Galagora commented Sep 11, 2020

I can't reproduce the stylish-haskell patch hash error.

$ nix-prefetch-url https://github.com/jaspervdj/stylish-haskell/commit/9550aa1cd177aa6fe271d075177109d66a79e67f.patch
[0.0 MiB DL]
path is '/nix/store/k82rvk4aid72dvn72v2qqr5vdhv8amwd-9550aa1cd177aa6fe271d075177109d66a79e67f.patch'
159jr80k40hdq5gpqfjknqx6vj2licx1l0f57l5r3k4264lnxjdb

On aarch64 the hash fails to validate, but on x86_64-linux the build just... fails? I can't tell why (maybe the download failed?).

copying path '/nix/store/c9sk1y8gqfgkfjmn2rgldx6rff66a7x7-HsYAML-aeson-0.2.0.0' from 'https://cache.nixos.org'...
cannot build derivation '/nix/store/ir4yj9yk3j9a6s69742fj01j2bq278x1-stylish-haskell-0.11.0.3.drv': 1 dependencies couldn't be built
error: build of '/nix/store/ir4yj9yk3j9a6s69742fj01j2bq278x1-stylish-haskell-0.11.0.3.drv' failed

@Galagora
Copy link
Contributor Author

Galagora commented Sep 11, 2020

The failed tests are a problem with the build machines, not with the expressions. On aarch64, the wrong hash is computed for the stylish-haskell patch, presumably because that platform's fetchpatch returns a subtly different result. For the rest of the builds, at some point they just randomly fail, with an error message like this:

cannot build derivation '/nix/store/4x1mgipakbpj8b8qcyy5vx0njfvkq9hq-haskell-language-server-0.4.0.0.drv': 1 dependencies couldn't be built

Sometimes accompanied by this:

error: --- Error --- nix-build
error: --- Error --- nix-daemon

There's no indication as to which dependencies actually fail, however.

@Galagora
Copy link
Contributor Author

@GrahamcOfBorg build haskellPackages.haskell-language-server
@GrahamcOfBorg build haskellPackages.stylish-haskell
@GrahamcOfBorg build haskell.packages.ghc8102.haskell-language-server

@peti peti force-pushed the haskell-updates branch 2 times, most recently from bd1911c to 10ec99b Compare September 11, 2020 18:30
@Galagora
Copy link
Contributor Author

Ouch

@pbogdan
Copy link
Member

pbogdan commented Sep 11, 2020

@Galagora It looks like on aarch64 #97407 still applies which means there's no working GHC on that platform so you can probably ignore it.

For the hash-mismatch - a hash returned by nix-prefetch-url is generally not suitable for use in fetchpatch which does additional post-processing to normalise the contents. However if you ran it locally nix-prefetch-url would register a fixed-output derivation in your store. When you feed that hash to fetchpatch Nix will see it already has it so won't do any work fetchpatch would normally do and just use it as is. On the builders, which don't have it yet, the patch will be fetched, post-processed, which will produce different output with different hash.

You should be able to reproduce it locally if you delete the result of nix-prefetch-url from your store (nix-store --delete on the result).

@peti peti merged commit 2076624 into NixOS:haskell-updates Sep 11, 2020
@pbogdan
Copy link
Member

pbogdan commented Sep 11, 2020

There's still a hash mismatch in haskellPackages.stylish-haskell.patches

@cdepillabout
Copy link
Member

@pbogdan It looks like peti fixed the stylish-haskell patch in 5f7b5a3, which should be in both the haskell-updates branch and master.

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