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-modules/generic-builder.nix: set NIX_LDFLAGS at the top level #266878

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

reckenrode
Copy link
Contributor

Description of changes

This is a follow-up PR to #266172 to address the feedback left by @sternenseemann regarding env.

Rebuilds on staging-next #263535 should be limited if there are any.

Also pinging @vcunat.

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/)
  • 23.11 Release Notes (or backporting 23.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.

@vcunat vcunat marked this pull request as draft November 11, 2023 15:51
@reckenrode reckenrode marked this pull request as ready for review November 11, 2023 15:56
@reckenrode
Copy link
Contributor Author

reckenrode commented Nov 11, 2023

@vcunat It’s ready now. I confirmed the same rebuilds are happening with and without this PR using b8b231f as the base, so this PR isn’t the cause of the rebuilds I saw.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 11, 2023
env.NIX_LDFLAGS = "-l${stdenv.cc.libcxx.cxxabi.libName}";
env = (
let
args_env = args.env or { };
Copy link
Member

Choose a reason for hiding this comment

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

args never has the env attribute at the moment so this makes little sense.

NIX_LDFLAGS should be able to be set as a direct argument to stdenv.mkDerivation and be propagated to the environment which would avoid this whole dance.

Copy link
Contributor Author

@reckenrode reckenrode Nov 11, 2023

Choose a reason for hiding this comment

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

I’ll revise and push an update when I have some bandwidth later today.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can just leave it as is on staging-next, the change I'd like would mean a mass rebuild I think.

Copy link
Member

Choose a reason for hiding this comment

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

mass rebuilds are OK now. We have no binaries there (again).

Copy link
Contributor Author

@reckenrode reckenrode Nov 12, 2023

Choose a reason for hiding this comment

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

I’d wanted to rebuild Cachix to confirm rebuilds, but it’s taking a while to get there, so I fixed and force-pushed. I left it at the end since that keeps the diff small and lets it remain separated for the clang/libc++ case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell, moving it up to the top level doesn’t appear to have caused rebuilds locally.

This is a follow-up PR to NixOS#266172 to address the feedback left by
@sternenseemann regarding `env`.

Rebuilds on staging-next NixOS#263535 should be limited if there are any.
@reckenrode reckenrode changed the title haskell-modules/generic-builder.nix: preserve env if set haskell-modules/generic-builder.nix: set NIX_LDFLAGS at the top level Nov 12, 2023
@vcunat vcunat merged commit b68f7e4 into NixOS:staging-next Nov 14, 2023
6 checks passed
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: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants