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

justStaticExecutables: Forbid references to GHC #304352

Merged
merged 2 commits into from
May 27, 2024
Merged

justStaticExecutables: Forbid references to GHC #304352

merged 2 commits into from
May 27, 2024

Conversation

9999years
Copy link
Contributor

Description of changes

This makes justStaticExecutables error if the produced store path contains references to GHC. This is almost always erroneous and due to the generated Paths_* module being imported. This helps prevent justStaticExecutables from producing binaries with closure sizes in the gigabytes.

See: #164630

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@github-actions github-actions bot added 6.topic: haskell 8.has: documentation This PR adds or changes documentation labels Apr 15, 2024
@maralorn
Copy link
Member

I like this.

A bit sad that this makes justStaticExecutables better than enableSeparateBinOutput. I forgot whether there is a per output option regarding requisites.

We will need to make sure that this doesn’t break any of the Haskell binaries that we ship in nixpkgs so this definitely needs to merge into haskell-updates.

@9999years
Copy link
Contributor Author

@maralorn Agreed, how do I do that? Just change the merge target?

@9999years 9999years changed the base branch from master to haskell-updates April 15, 2024 21:43
sternenseemann
sternenseemann requested changes Apr 16, 2024
View reviewed changes
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.

disallowedRequisites needs to become an argument to haskellPackages.mkDerivation and transparently passed on to stdenv.mkDerivation in there. Otherwise disallowedRequisites will only be set depending on override order, i.e. any overrideCabal-based function applied after justStaticExecutables would revert the change again, as it uses a different fix point from overrideAttrs.

@9999years
Copy link
Contributor Author

9999years commented Apr 16, 2024 •
edited
Loading

@sternenseemann I'm not sure how to make that work; I can add disallowedRequisites to the generic builder, but then I don't have access to the passthru.compiler attribute. I can add a ghc argument with a default value, but then I can't access that in the override unless it's changed from the default due to NixOS/nix#334

Maybe it could be made to work with some functor magic, but I wouldn't know how to wire it up robustly.

@sternenseemann
Copy link
Member

@9999years passthru.compiler is set in that function, so it is available as the ghc argument passed in.

@sternenseemann
Copy link
Member

Ah, I get what you mean. In principle you can literally add an disallowGhcReference argument tbh.

@sternenseemann
Copy link
Member

Or drv: let old = drv; in overrideCabal { disallowedRequisites = [ old.passthru.compiler ]; } old should also work?

@9999years 9999years force-pushed the just-static-executables-disallow-ghc branch from 49ce66f to dc0af03 Compare April 16, 2024 22:09
@9999years
Copy link
Contributor Author

OK, I took a shot at it, let me know what you think.

I also checked that the docs render correctly:

image

@9999years
Copy link
Contributor Author

@sternenseemann Still looking for a review on this!

sternenseemann
sternenseemann reviewed May 18, 2024
View reviewed changes
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.

Sorry for the wait!

pkgs/development/haskell-modules/generic-builder.nix Outdated Show resolved Hide resolved
pkgs/development/haskell-modules/generic-builder.nix Outdated Show resolved Hide resolved
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2024
@9999years 9999years force-pushed the just-static-executables-disallow-ghc branch from dc0af03 to e1bc2ba Compare May 22, 2024 22:01
@9999years
Copy link
Contributor Author

PTAL

@9999years 9999years requested a review from sternenseemann May 22, 2024 22:02
@sternenseemann sternenseemann force-pushed the just-static-executables-disallow-ghc branch from d83abc3 to 9e9a2f2 Compare May 27, 2024 14:33
@sternenseemann sternenseemann removed the 9.needs: port to stable A PR needs a backport to the stable release. label May 27, 2024
@sternenseemann
Copy link
Member

I've realized that we can't backport this anymore since this is technically a breaking change. I've moved the changelog entry to the 24.11 release notes as a result. We could backport a version of this which adds the new arguments to haskellPackages.mkDerivation, but doesn't change the behavior of justStaticExecutables.

I've also noticed that we haven't documented disallowedRequisites and disallowedGhcReference to the mkDerivation documentation in the manual, but maybe that's okay since disallowedRequisites is undocumented for stdenv.mkDerivation as well…

@sternenseemann sternenseemann merged commit 0454f7b into NixOS:haskell-updates May 27, 2024
22 checks passed
@sternenseemann
Copy link
Member

@9999years Evals newer than 1806586 of haskell-updates contain your change. There are already a few regressions I've seen on aarch64-darwin. This is not really surprising as GHC can't properly eliminate dead code on that platform. It may be prudent to ignore disallowGhcReference on that platform.

@9999years 9999years deleted the just-static-executables-disallow-ghc branch May 28, 2024 16:40
@sternenseemann sternenseemann mentioned this pull request May 28, 2024
Merged
6 tasks
@9999years
Copy link
Contributor Author

There are already a few regressions I've seen on aarch64-darwin

Which attributes are impacted?

@sternenseemann
Copy link
Member

@9999years They should accumulate here. Note that a lot of builds are still pending so the list will get longer as time goes on.

@9999years 9999years mentioned this pull request May 28, 2024
Closed
13 tasks
@9999years 9999years mentioned this pull request May 28, 2024
Merged
13 tasks
9999years added a commit to 9999years/nixpkgs that referenced this pull request May 29, 2024
@9999years
cabal-install: Fix for justStaticExecutables
2eef4ba 
Fixes `cabal-install` to remove references to GHC, mostly through other
libraries that are included in the binary.

See: NixOS#304352
9999years added a commit to 9999years/nixpkgs that referenced this pull request Jun 3, 2024
@9999years
cabal2nix: Fix for justStaticExecutables
67bb4e1 
Remove references to `hpack` and `distribution-nixpkgs` paths so that
`cabal2nix` can build on macOS.

See: NixOS#304352
sternenseemann pushed a commit that referenced this pull request Jun 3, 2024
@9999years @sternenseemann
cabal2nix: Fix for justStaticExecutables
2901eb8 
Remove references to `hpack` and `distribution-nixpkgs` paths so that
`cabal2nix` can build on macOS.

See: #304352
sternenseemann added a commit that referenced this pull request Jun 6, 2024
@9999years @sternenseemann
cabal-install: Fix for justStaticExecutables
caa1c96 
Fixes `cabal-install` to remove references to GHC, mostly through other
libraries that are included in the binary.

See: #304352

Co-authored-by: sternenseemann <[email protected]>
sternenseemann added a commit to sternenseemann/nixpkgs that referenced this pull request Jun 6, 2024
@sternenseemann
gitit: drop reference prevention code
9ae674e 
All these references would (indirectly) incur a GHC requisite which is
prevented by NixOS#304352 via justStaticExecutables. Consequently, we can
stop setting disallowedReferences.

As it turns out the references we were removing aren't currently
created, so our life gets even easier.
sternenseemann added a commit that referenced this pull request Jun 7, 2024
@sternenseemann
gitit: drop reference prevention code
1285457 
All these references would (indirectly) incur a GHC requisite which is
prevented by #304352 via justStaticExecutables. Consequently, we can
stop setting disallowedReferences.

As it turns out the references we were removing aren't currently
created, so our life gets even easier.
@sternenseemann sternenseemann mentioned this pull request Jun 7, 2024
Open
11 tasks
sternenseemann added a commit that referenced this pull request Jun 8, 2024
@sternenseemann
treewide: no justStaticExecutables on aarch64-darwin for bogus refs
d785f11 
This commit disables justStaticExecutables for packages on
aarch64-darwin that incur a (usually incorrect) reference on GHC.
justStaticExecutables now fails when a GHC reference remains in the
output (#304352). Due to this reference justStaticExecutables (before
these changes) would only marginally reduce closure size.

In the future, we'll be able to re-introduce justStaticExecutables
after (manually) removing the incorrect references stemming from the use
of Paths_* modules. Tracking Issue: #318013
This was referenced Jun 12, 2024
Merged
Merged
srid added a commit to srid/emanote that referenced this pull request Dec 24, 2024
@srid
Disable closure size reduction on macOS
e4d7a29 
- NixOS/nixpkgs#318013
- NixOS/nixpkgs#304352

Related: NixOS/nix#10877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sternenseemann sternenseemann sternenseemann approved these changes

@maralorn maralorn Awaiting requested review from maralorn

@ncfavier ncfavier Awaiting requested review from ncfavier

Assignees
No one assigned
Labels
6.topic: haskell 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 101-500 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@9999years @maralorn @sternenseemann @wegank