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

stdenvBootstrapTools: inherit {cross,local}System #175484

Merged
merged 1 commit into from
May 31, 2022
Merged

stdenvBootstrapTools: inherit {cross,local}System #175484

merged 1 commit into from
May 31, 2022

Conversation

alyssais
Copy link
Member

Description of changes

It's expected that attributes in the top-level package set will all use that package set, but this wasn't the case for the bootstrap tools. This led some very confusing behaviour:

  • pkgsMusl.stdenvBootstrapTools would build glibc bootstrap tools

  • stdenvBootstrapTools was always cross compiled, even if Nixpkgs wasn't, because it always set crossSystem. This also didn't match the behaviour of using make-bootstrap-tools.nix as an entrypoint, where crossSystem would default to null.

For the Linux stdenv, I've made the ideal fix, which is to make pkgs an argument rather than taking the arguments for pkgs, and then re-importing it. This means it'll always use exactly the same package set that's calling it, and should also mean faster eval due to not importing Nixpkgs twice.

The Darwin stdenv is more complicated, and I'm not able to easily test it, so I wasn't confident in making the same fix there. Instead, I've just made sure crossSystem and localSystem are set to the correct values so they're not always cross compiled and match the parent package set's. It would still be preferable if somebody could make Darwin's make-bootstrap-tools.nix take pkgs as an argument, rather than all the arguments for pkgs.

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, 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/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label May 30, 2022
@alyssais alyssais added 6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: bootstrap Bootstrapping, avoiding pre-built binaries. Often overlaps with cross-compilation. 6.topic: musl Running or building packages with musl libc 6.topic: stdenv Standard environment and removed 6.topic: stdenv Standard environment labels May 30, 2022
@alyssais
stdenvBootstrapTools: inherit {cross,local}System
c0d69d3 
It's expected that attributes in the top-level package set will all use
that package set, but this wasn't the case for the bootstrap tools.
This led some very confusing behaviour:

- pkgsMusl.stdenvBootstrapTools would build glibc bootstrap tools
- stdenvBootstrapTools was _always_ cross compiled, even if
  Nixpkgs wasn't, because it always set crossSystem.  This also didn't
  match the behaviour of using make-bootstrap-tools.nix as an
  entrypoint, where crossSystem would default to null.

For the Linux stdenv, I've made the ideal fix, which is to make pkgs an
argument rather than taking the arguments for pkgs, and then
re-importing it.  This means it'll always use exactly the same package
set that's calling it, and should also mean faster eval due to not
importing Nixpkgs twice.

The Darwin stdenv is more complicated, and I'm not able to easily test
it, so I wasn't confident in making the same fix there.  Instead, I've
just made sure crossSystem and localSystem are set to the correct values
so they're not always cross compiled and match the parent package set's.
It would still be preferable if somebody could make Darwin's
make-bootstrap-tools.nix take pkgs as an argument, rather than all the
arguments for pkgs.
@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 May 30, 2022
@alyssais
Copy link
Member Author

@ofborg build stdenvBootstrapTools

@alyssais
Copy link
Member Author

Not sure what happened on aarch64-darwin, let's retry.

@ofborg build stdenvBootstrapTools

@bobby285271 bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 31, 2022
@thefloweringash
Copy link
Member

Not sure what happened on aarch64-darwin, let's retry.

@ofborg build stdenvBootstrapTools

The unpack and test attributes for the Darwin bootstrap tools seem to be out of date. For x86_64-darwin, there's some unnecessary but mostly harmless extra handling of rpaths in unpack, but for aarch64-darwin, it needs to be more careful to not break the signatures when rewriting the install names of the shipped .dylibs. It's easy enough to fix, but the test attribute also needs some attention before it will pass and I've ran out of time for now.

I don't think you need to worry about aarch64-darwin for this PR.

@alyssais alyssais merged commit 5643714 into NixOS:master May 31, 2022
@alyssais alyssais deleted the stdenvBootstrapTools-pkgs branch May 31, 2022 14:32
@alyssais
Copy link
Member Author

@thefloweringash thank you for your insight

@alyssais alyssais mentioned this pull request Jun 8, 2022
Merged
13 tasks
@trofi
Copy link
Contributor

trofi commented Jul 22, 2022

Looks like this PR caused hydra to silentry drop all the stdenvBootstrapTools jobs as it's an empty attrset now, Proposed possible fix: #182058

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

@thefloweringash thefloweringash thefloweringash approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@toonn toonn Awaiting requested review from toonn

Assignees
No one assigned
Labels
6.topic: bootstrap Bootstrapping, avoiding pre-built binaries. Often overlaps with cross-compilation. 6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: musl Running or building packages with musl libc 6.topic: stdenv Standard environment 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 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alyssais @thefloweringash @trofi @sternenseemann @bobby285271