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

pkgsMusl.haskell.compiler.ghc8107Binary: Remove now-incorrect gmp dependency #138523

Merged

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Sep 19, 2021

Motivation for this change

GHC HQ switched the musl bindists from gmp to integer-simple
with GHC >= 8.10.6, but this was not reflected in the nixpkgs update:

See also #130441.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@domenkozar
Copy link
Member

I had no idea they switched.

@cdepillabout
Copy link
Member

cdepillabout commented Sep 19, 2021

@nh2 How do you recommend this be tested?

Would building the following things fully test this?

binary ghcs

  • haskell.packages.ghc8102Binary.lens: This would test that the gmp-linked non-musl ghc8102Binary can actually be used to build packages.
  • pkgsMusl.haskell.packages.ghc8102Binary.lens: This would test that the gmp-linked musl ghc8102Binary can be used to build packages.
  • haskell.packages.ghc8107Binary.lens: This would test that the gmp-linked non-musl ghc8107Binary can actually be used to build packages.
  • pkgsMusl.haskell.packages.ghc8107Binary.lens: This would test that the non-gmp-linked musl ghc8107Binary can be used to build packages.

ghc8107

  • haskell.packages.ghc8107.lens: This would test that a gmp-linked non-musl ghc8107 (which is built with the gmp-linked non-musl ghc865Binary) can be used to build packages. edit: I guess this shouldn't actually be affected by this PR at all
  • pkgsMusl.haskell.packages.ghc8107.lens: This would test that a gmp-linked musl ghc8107 (which is built with gmp-linked musl ghc8102BinaryMinimal) can be used to build packages.
  • haskell.packages.integer-simple.ghc8107.lens: This would test that a non-gmp-linked non-musl ghc8107 (which is built with the gmp-linked non-musl ghc865Binary) can be used to build packages. edit: I guess this shouldn't actually be affected by this PR at all
  • pkgsMusl.haskell.packages.integer-simple.ghc8107.lens: This would test that a non-gmp-linked musl ghc8107 (which is built with gmp-linked musl ghc8102BinaryMinimal) can be used to build packages.

ghc901

  • haskell.packages.ghc901.lens: This would test that a gmp-linked non-musl ghc901 (which is built with the gmp-linked non-musl ghc8102Binary) can be used to build packages.
  • pkgsMusl.haskell.packages.ghc901.lens: This would test that a gmp-linked musl ghc901 (which is built with gmp-linked musl ghc8102Binary) can be used to build packages.
  • haskell.packages.integer-simple.ghc901.lens: This would test that a non-gmp-linked non-musl ghc901 (which is built with the gmp-linked non-musl ghc8102Binary) can be used to build packages.
  • pkgsMusl.haskell.packages.integer-simple.ghc901.lens: This would test that a non-gmp-linked musl ghc901 (which is built with gmp-linked musl ghc8102Binary) can be used to build packages.

Is this about right? Do the success of these derivations give a good indication as to whether this PR is correct? Or is this overkill?


The one thing the above derivation list is missing is something like the above ghc901 section, but where haskell.compiler.ghc901, pkgsMusl.haskell.compiler.ghc901, haskell.compiler.integer-simple.ghc901, pkgsMusl.haskell.compiler.integer-simple.ghc901 are built using ghc8107Binary.

Also, in pkgs/top-level/haskell-packages.nix, there is a note that starting from GHC 9, integer-simple and integer-gmp have been replaced by ghc-bignum with native and gmp backends. Should these be tested as well??

@nh2
Copy link
Contributor Author

nh2 commented Sep 19, 2021

@cdepillabout Personally I'd just build all Haskell packages Hydra builds, and check if any of them regressed, if that's easy (I fail to remember how to do that though). However, all the packages you mentioned build fine for me (see below).

That said, I think it'd be better to test application builds than library ones, given that we were dealing with linker errors.

Recording here some failures and successes:

  • Failure: pkgsMusl.haskell.compiler.integer-simple.ghc884
    /build/ghc27986_0/ghc_4.c:7:15: error:
         error: variable ‘stg_exports_FastString’ has initializer but incomplete type
            7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
              |               ^~~~~~~~~~~~~~~~~~
      |
    7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
      |               ^
    
    /build/ghc27986_0/ghc_4.c:7:61: error:
         error: ‘struct ForeignExportsList’ has no member named ‘exports’
            7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
              |                                                             ^~~~~~~
      |
    7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
      |                                                             ^
    
    /build/ghc27986_0/ghc_4.c:7:72: error:
         error: extra brace group at end of initializer
            7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
              |                                                                        ^
      |
    7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
      |                                                                        ^
    
  • Success: pkgsMusl.haskellPackages.hello
  • Success: pkgsMusl.haskell.compiler.integer-simple.ghc8107
  • Success: haskell.packages.ghc8102Binary.lens
  • Success: pkgsMusl.haskell.packages.ghc8102Binary.lens
  • Success: haskell.packages.ghc8107Binary.lens
  • Success: pkgsMusl.haskell.packages.ghc8107Binary.lens
  • Success: haskell.packages.ghc8107.lens
  • Success: pkgsMusl.haskell.packages.ghc8107.lens
  • Success: haskell.packages.integer-simple.ghc8107.lens
  • Success: pkgsMusl.haskell.packages.integer-simple.ghc8107.lens
  • Success: haskell.packages.ghc901.lens
  • Success: pkgsMusl.haskell.packages.ghc901.lens
  • Success: haskell.packages.integer-simple.ghc901.lens
  • Success: pkgsMusl.haskell.packages.integer-simple.ghc901.lens

@@ -140,20 +143,35 @@ stdenv.mkDerivation rec {

src = fetchurl binDistUsed.src;

# Note that for GHC 8.10 versions <= 8.10.5, the GHC HQ musl bindist
Copy link
Member

Choose a reason for hiding this comment

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

I think this sentence is missing a word.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

nativeBuildInputs = [ perl ];
propagatedBuildInputs =
lib.optionals useLLVM [ llvmPackages.llvm ]
# Because musl bindists currently provide no way to tell where
# libgmp is (see not [musl bindists have no .buildinfo]), we need
Copy link
Member

Choose a reason for hiding this comment

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

Can the comment on musl bindists have no .buildinfo further down removed here as well (or clarified wrt musl)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have fixed the dangling reference.

@sternenseemann
Copy link
Member

sternenseemann commented Sep 20, 2021

Maybe let's close this in favor of #138524 for simplicity's sake? Would like to get all changes in this rotation.

…endency.

GHC HQ switched the musl bindists from gmp to `integer-simple`
with GHC >= 8.10.6, but this was not reflected in the nixpkgs update:

* commit 6f12424: ghc: 8.10.5-binary -> 8.10.7-binary
  From PR NixOS#135453

See also NixOS#130441.
@nh2 nh2 force-pushed the ghc-binary-musl-propagated-comment branch from fb79cf9 to a51af7d Compare September 20, 2021 17:02
@nh2
Copy link
Contributor Author

nh2 commented Sep 20, 2021

Maybe let's close this in favor of #138524 for simplicity's sake? Would like to get all changes in this rotation.

Sure, can do either.

I made this one separately because it is much easier, as it touches only musl; the other one touches all compilers.

@sternenseemann
Copy link
Member

I'd just do the set rebuild all in one go, actually.

@nh2
Copy link
Contributor Author

nh2 commented Sep 26, 2021

  • Failure: pkgsMusl.haskell.compiler.integer-simple.ghc884
    /build/ghc27986_0/ghc_4.c:7:15: error:
         error: variable ‘stg_exports_FastString’ has initializer but incomplete type
            7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
              |               ^~~~~~~~~~~~~~~~~~
      |
    7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
      |               ^
    
    /build/ghc27986_0/ghc_4.c:7:61: error:
         error: ‘struct ForeignExportsList’ has no member named ‘exports’
            7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
              |                                                             ^~~~~~~
      |
    7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
      |                                                             ^
    
    /build/ghc27986_0/ghc_4.c:7:72: error:
         error: extra brace group at end of initializer
            7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
              |                                                                        ^
      |
    7 | static struct ForeignExportsList stg_exports_FastString = {.exports =  {}, .n_entries =  0};
      |                                                                        ^
    

@sternenseemann Investigating this deeper, I found some odd results:

let mc = pkgsMusl.haskell.compiler in:

  1. ghc8102BinaryMinimal succeeds at mc.ghc884 fine.
  2. ghc8107BinaryMinimal fails at mc.ghc884, with the above C error,
    so the .2 -> .7 bindist regressed that
  3. ghc8102BinaryMinimal fails at mc.integer-simple.ghc884, with the above C error
  4. ghc8102BinaryMinimal fails at mc.integer-simple.ghc884 with the old expected error
    ld: cannot find -lgmp

sternenseemann added a commit that referenced this pull request Sep 26, 2021
Reverse bootstrapping is not supported by GHC upstream. In the case of
8.8.4 it just happens to work using 8.10.2, with later versions,
specifically 8.10.7 there seems to be some digressions in the generated /
used C code which cause 8.8.4 to fail to compile [1].

Thus we revert to using 8.10.2 for aarch64 and Musl which means: Still
no integer-simple and musl at the same time (however all other GHCs have
it, so it's probably not a problem) and no aarch64-darwin (GHC 8.8.4
can't target that architecture anyways). In short, the situation stays
the same.

[1]: #138523 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants