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.

Sign up for GitHub

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

ghc: Add variantSuffix, fix musl+integer-simple #138524

Closed
wants to merge 3 commits into from
Closed

ghc: Add variantSuffix, fix musl+integer-simple #138524

wants to merge 3 commits into from

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Sep 19, 2021 •
edited
Loading

Motivation for this change
  • Makes musl+integer-simple combination unbroken (was marked as broken in PR ghc: mark broken for musl + integer-simple builds #130441).
    • This is because the GHC HQ 8.10.7 bindist, in contrast to the 8.10.2 one, does not depend on gmp, but was built against integer-simple itself.
  • Obviates the workaround found in PR ghc: mark broken for musl + integer-simple builds #130441.
  • PR also includes a technically separate change: Changing name for all GHCs so that you can tell whether it's a -musl and/or -integer-simple variant, making inspection with nix-store -q --tree much easier, and copy-pasted nix error messages more obvious (you know which GHC variant failed to build).
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.

TODO

This was referenced Sep 19, 2021
Closed
Merged
@nh2 nh2 force-pushed the ghc-use-ghc8107-for-bootstrapping branch from d7d99e4 to bc5a969 Compare September 19, 2021 13:38
nomeata added a commit to nomeata/ic-hs that referenced this pull request Sep 19, 2021
@nomeata
Try NixOS/nixpkgs#138524
cb1d991
@nomeata
Copy link
Contributor

nomeata commented Sep 19, 2021

I can confirm that this works in my project (ic-hs, including stuff like Template Haskell etc.)

marcin-dziadus pushed a commit to dfinity/ic-hs that referenced this pull request Sep 20, 2021
@nomeata
Bump nixpkgs to latest master (#30)
fb558eb 
* Experiment: Bump nixpkgs to latest master

This is to see if there are problems coming up. Some of the things in
nix/haskell-packages.nix can probably be cleaned up now.

Originally https://github.com/dfinity-lab/ic-ref/commit/dce1286a900525607fcc21171b82940e3ec3385f

* Bump again

Originally https://github.com/dfinity-lab/ic-ref/commit/b43f83609fd3a99e0a8ecd8700f4baee0c53b863

* Bump once more

Originally https://github.com/dfinity-lab/ic-ref/commit/a0746ed7be49c8f1619e5b75ba84d5c832730cf8

* Fix merge issue

* Fix path

* Back to master

* Try winter master

* Remove many haskell package changes

* More simplification and upstream cache use

* Update freeze file

* Less versionoverrides needed

* Remove nix/haskell-packages, more DNRY

* Fix patch handling

* Add .gitattributes to mark generated files as such

* s/	/        /

* Run github actions with ghc-8.10.7

* Fix docs jobs

* Fix docs jobs some more

* Update cabal.project

* Bump candid some more

* Checkout before setting up the cache

* Fix cabal keys

* Don’t set active-repositories in freeze file

* Rename workflow

* naersk changed it seems

* Try pkgsStatic, not pkgsMusl

as the latter doesn’t seem to work any more since
NixOS/nixpkgs#130441

* Fix static cborg build

* Try static-haskell-nix

* Revert "Try static-haskell-nix"

This reverts commit b375ad4.

* Revert "Try pkgsStatic, not pkgsMusl"

This reverts commit d1f64f1.

* Try pkgsMusl again

pulling in NixOS/nixpkgs#138429

* Update freeze file

* Right GHC version

* Try NixOS/nixpkgs#138524
@sternenseemann sternenseemann mentioned this pull request Sep 20, 2021
Merged
12 tasks
sternenseemann
sternenseemann reviewed Sep 20, 2021
View reviewed changes
pkgs/development/compilers/ghc/8.10.2-binary.nix
@@ -139,7 +145,7 @@ in
stdenv.mkDerivation rec {
inherit version;

name = "ghc-${version}-binary";
name = "ghc-${version}-binary${binDistUsed.variantSuffix}";
Copy link
Member

Choose a reason for hiding this comment

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

Appending something to a derivation name is usually a bad idea since builtins.parseDrvName interprets everything after the first number after a dash as the version.

We are already doing this for the binary ones, but we shouldn't expand this. So let's take this opportunity to redo the naming of the binary derivations as well and move the -binary suffix before the version number (maybe in a separate commit?):

Suggested change
name = "ghc-${version}-binary${binDistUsed.variantSuffix}";
name = "ghc-binary${binDistUsed.variantSuffix}-${version}";

Of course would need to be reflected in the other derivations as well. If you don't feel like doing it, I can also take care of that for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point.

I think we should do it the way you suggest.

Copy link
Member

Choose a reason for hiding this comment

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

This was done in #139164 which I've already merged into haskell-updates (with the variant suffix changes as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done in PR #139164.

pkgs/top-level/haskell-packages.nix
@@ -77,7 +77,7 @@ in {
# aarch64 ghc865Binary gets SEGVs due to haskell#15449 or similar
# Musl bindists do not exist for ghc 8.6.5, so we use 8.10.* for them
else if stdenv.isAarch64 || stdenv.targetPlatform.isMusl then
Copy link
Member

Choose a reason for hiding this comment

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

You can drop the aarch64-darwin condition above, since stdenv.isAarch64 will also catch that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense.

pkgs/top-level/haskell-packages.nix
@@ -91,7 +91,7 @@ in {
# aarch64 ghc865Binary gets SEGVs due to haskell#15449 or similar
Copy link
Member

Choose a reason for hiding this comment

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

You can drop the aarch64-darwin condition above, since stdenv.isAarch64 will also catch that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense.

pkgs/top-level/haskell-packages.nix
@@ -108,19 +108,19 @@ in {
packages.ghc8107BinaryMinimal
Copy link
Member

Choose a reason for hiding this comment

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

You can drop the aarch64-darwin condition above, since stdenv.isAarch64 will also catch that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense.

@nh2
pkgsMusl.haskell.compiler.ghc8107Binary: Remove now-incorrect gmp dep…
a51af7d 
…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
ghc: Add variantSuffix.
b266503 
When debugging musl builds, I often have to sift through thousands of lines
of `nix-store -q --tree` or `nix-store -qR` output.
Until now, `pkgsMusl` and normal `pkgs` GHCs looked exactly the same in
there, making that task tough.

Same for `integer-simple`, which makes debugging `gmp` issues easier.

This commit introduces a suffix to tell them apart easily.

Note that this is different from `targetPrefix` which is for
cross-compilation, which `pkgsMusl` does not do.
@nh2
ghc: Use 8.10.7 bindist for bootstrapping. Fixes musl+integer-simple
8ec92c5 
See NixOS#130441.
@nh2 nh2 force-pushed the ghc-use-ghc8107-for-bootstrapping branch from bc5a969 to 8ec92c5 Compare September 20, 2021 17:02
@sternenseemann sternenseemann mentioned this pull request Sep 23, 2021
Merged
12 tasks
@nh2
Copy link
Contributor Author

nh2 commented Sep 23, 2021

Closing in favour of these PRs that include all these changes, rebased:

@nh2 nh2 closed this Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sternenseemann sternenseemann sternenseemann left review comments

@cdepillabout cdepillabout Awaiting requested review from cdepillabout

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@maralorn maralorn Awaiting requested review from maralorn

@expipiplus1 expipiplus1 Awaiting requested review from expipiplus1

Assignees
No one assigned
Labels
6.topic: haskell
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nh2 @nomeata @sternenseemann