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

ghc: hadrian patch fix for fully_static #317175

Conversation

NorfairKing
Copy link
Contributor

Description of changes

Fixes #208959 and #316850

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@NorfairKing
Copy link
Contributor Author

This will need a backport to nixos-24.05 as well.

@maralorn
Copy link
Member

maralorn commented Jun 4, 2024

We could theoretically only apply this patch for the exact configuration which we want to use it for. i.e. enableShared = false;. Not sure. @sternenseemann, what do you think?

@NorfairKing
Copy link
Contributor Author

NorfairKing commented Jun 4, 2024

We can also only turn on this patch when enableShared is false, to make it less intrusive.

Edit: Started typing that before the previous comment was posted.

Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Thanks for tracking this patch down.

The commit message should use haskell.compiler.ghc96 as prefix since it is more specific.

@@ -170,6 +171,11 @@
then ./docs-sphinx-7-ghc98.patch
else ./docs-sphinx-7.patch )
]
++ lib.optional (lib.versionAtLeast version "9.6" && lib.versionOlder version "9.8")
Copy link
Member

Choose a reason for hiding this comment

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

Please use lib.optionals for consistency with the rest of the logic.

@@ -170,6 +171,11 @@
then ./docs-sphinx-7-ghc98.patch
else ./docs-sphinx-7.patch )
]
++ lib.optional (lib.versionAtLeast version "9.6" && lib.versionOlder version "9.8")
(fetchpatch {
Copy link
Member

Choose a reason for hiding this comment

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

Please use the name argument to fetchpatch to give this patch a descriptive name which helps when debugging.

@sternenseemann
Copy link
Member

We could theoretically only apply this patch for the exact configuration which we want to use it for. i.e. enableShared = false;. Not sure. @sternenseemann, what do you think?

No, patches should be unconditional if possible to reduce complexity.

@NorfairKing
Copy link
Contributor Author

@sternenseemann Thanks for the quick review. I've fixed what you mentioned, I think.

@sternenseemann
Copy link
Member

Ah, one more thing: You need to remove the broken flag from meta in the expression since that is based on the problem we are fixing here.

@ofborg ofborg bot requested a review from sternenseemann June 4, 2024 16:47
@NorfairKing
Copy link
Contributor Author

@sternenseemann In order! What's the rest of the process? Is there any info about how to do a backport?

Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

I'll merge this for the next haskell-updates iteration so we don't block fixes in the current iteration much longer.

@sternenseemann
Copy link
Member

Is there any info about how to do a backport?

The backport will happen semi-automatically via github actions. The process is also described in CONTRIBUTING.md

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 5, 2024
@wegank wegank added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Jun 5, 2024
@sternenseemann sternenseemann merged commit 6457cf0 into NixOS:haskell-updates Jun 8, 2024
26 checks passed
Copy link
Contributor

github-actions bot commented Jun 8, 2024

Successfully created backport PR for staging-24.05:

@nh2
Copy link
Contributor

nh2 commented Jun 15, 2024

Note that e.g. on nixos-24.05 there's still this code that makes pkgsStatic use GHC 9.4 instead of the default 9.6:

  haskellPackages = dontRecurseIntoAttrs
    # Prefer native-bignum to avoid linking issues with gmp
    # GHC 9.6 rts can't be built statically with hadrian, so we need to use 9.4
    # until 9.8 is ready
    (if stdenv.hostPlatform.isStatic then haskell.packages.native-bignum.ghc94
    # JS backend can't use gmp
    else if stdenv.hostPlatform.isGhcjs then haskell.packages.native-bignum.ghc96
    else haskell.packages.ghc96)

https://github.com/NixOS/nixpkgs/blob/3a0e59b841eff01ba5535b1cc807a60eff543c66/pkgs/top-level/all-packages.nix#L15900-L15907

This PR fixes at least part of that; we should see if that can be changed now.

There's still this problem:

nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Jun 15, 2024
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Jun 15, 2024
elopez added a commit to elopez/hevm that referenced this pull request Sep 5, 2024
elopez added a commit to elopez/hevm that referenced this pull request Sep 18, 2024
elopez added a commit to elopez/hevm that referenced this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: haskell 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package backport staging-24.05 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants