-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
stdenvBootstrapTools: native aarch64-darwin build #202347
Conversation
bfac032
to
ea1f4b6
Compare
e65de42
to
969c929
Compare
969c929
to
283ca25
Compare
# Copy coreutils, bash, etc. | ||
cp ${coreutils_}/bin/* $out/bin | ||
(cd $out/bin && rm vdir dir sha*sum pinky factor pathchk runcon shuf who whoami shred users) | ||
|
||
cp ${bash}/bin/bash $out/bin | ||
ln -s bash $out/bin/sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bash
and bzip2
links were previously created by the unpack script, but I moved them here. I feel like we should avoid altering the package at unpack-time if we can, but the remainder (wrappers, install names) seem unavoidable.
''} | ||
# copy sigtool | ||
cp -d ${pkgs.darwin.sigtool}/bin/sigtool $out/bin | ||
cp -d ${pkgs.darwin.sigtool}/bin/codesign $out/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the conditions around codesigning to keep both archs more in sync. They don't harm intel, and only affect the bootstrap.
cp -rd ${pkgs.darwin.CF}/Library $out | ||
cp -rd ${pkgs.darwin.CF}/Library $out | ||
${lib.optionalString stdenv.targetPlatform.isAarch64 '' | ||
cp -rd ${pkgs.darwin.libobjc}/lib/* $out/lib/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both CoreFoundation and libobjc are tbds only on aarch64, but having these allows the CF test to pass.
sed -i -e 's|/nix/store/.*/libobjc.A.dylib|@executable_path/../libobjc.A.dylib|g' \ | ||
$out/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation.tbd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a hack. I'm not sure how to properly implement the rpath logic here for tbds. But it seems to work fine, and only affects the bootstrap.
echo patching $i | ||
install_name_tool -add_rpath $out/lib $i || true | ||
fi | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step seems unnecessary, because we already add @executable_path/../lib
during the build. Regardless, I based the new script on the ARM variant, which doesn't have it, and that appears to work nicely for Intel too.
|
||
export CPP="clang -E $flags" | ||
export CC="clang $flags -Wl,-rpath,${unpack}/lib -Wl,-v -Wl,-sdk_version,10.10" | ||
export CXX="clang++ $flags --stdlib=libc++ -lc++abi -isystem${unpack}/include/c++/v1 -Wl,-rpath,${unpack}/lib -Wl,-v -Wl,-sdk_version,10.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -sdk_version
appears unnecessary, and conflicts with -platform_version
that the Clang driver already passes to ld. (It warns about -sdk_version
being ignored.)
The -rpath
is something Clang also accepts, no -Wl
necessary.
meta = with lib; { | ||
homepage = "https://github.com/stephank/dumpnar"; | ||
description = "Minimal tool to produce a Nix NAR archive."; | ||
license = licenses.lgpl2Plus; | ||
platforms = platforms.all; | ||
maintainers = [ maintainers.stephank ]; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is code extracted from Nix itself. Trying to use Nix itself here was trouble. It significantly increases the built closure, plus it apparently looks for config causing errors even for simple things like creating a NAR. I didn't investigate beyond that point and simply extracted the code. (I had another use for it myself.)
inherit (bootstrap) dist test; | ||
# Test a full stdenv bootstrap from the bootstrap tools definition | ||
# TODO: Re-enable once the new bootstrap-tools are in place. | ||
#inherit (bootstrap.test-pkgs) stdenv; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be re-enabled in the follow-up PR that swaps in the new tools.
The change in bootstrapFiles
means it's unfortunately not possible to test a stdenv
build in this PR via this attribute. Instead, pull in the follow-up commit from the PR description, then just do nix-build -A stdenv
(optionally setting system
).
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/1802 |
inherit | ||
(import ../stdenv/linux/make-bootstrap-tools.nix { | ||
genAttrs systemsWithAnySupport (system: | ||
if hasSuffix "-linux" system then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use stdenv.isLinux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is iterating over systemsWithAnySupport
.
283ca25
to
c3693fb
Compare
Thanks for the review. I updated the PR and believe I addressed all comments. |
''; | ||
}; | ||
|
||
bootstrapLlvmVersion = llvmPackages.llvm.version; | ||
|
||
bootstrapFiles = { | ||
sh = "${build}/on-server/sh"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nix-repl> :p legacyPackages.x86_64-darwin.freshBootstrapTools.test-pkgs.CoinMP
error: attribute 'sh' missing
at /nix/store/8ycgcgzaskmw3wcw0kn7v0v1ggwnvbzz-source/pkgs/stdenv/darwin/default.nix:72:15:
71| name = "bootstrap-tools";
72| builder = bootstrapFiles.sh; # Not a filename! Attribute 'sh' on bootstrapFiles
| ^
73| args = if localSystem.isAarch64 then [ ./unpack-bootstrap-tools-aarch64.sh ] else [ ./unpack-bootstrap-tools.sh ];
«derivation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverting this line show bzip2
is also required. So this PR might be incomplete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: https://github.com/NixOS/nixpkgs/pull/202347/files#r1060701648
There's a tracking PR for new bootstrap tools: #222717
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok. I just track eval errors. I would have appreciated a throw or assert (in that case the :p can continue)
# Set the ELF interpreter / RPATH in the bootstrap binaries. | ||
echo Patching the tools... | ||
bootstrapTools = derivation { | ||
inherit system; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks nix build -f pkgs/top-level/release.nix unstable
error: undefined variable 'system'
at /home/artturin/nixgits/my-nixpkgs/.worktree/1/pkgs/stdenv/darwin/make-bootstrap-tools.nix:213:32:
212|
213| bootstrapTools = derivation {
| ^
214| inherit system;
it should probably be
diff --git a/pkgs/stdenv/darwin/make-bootstrap-tools.nix b/pkgs/stdenv/darwin/make-bootstrap-tools.nix
index 94c61e396b65..46ba25f8603b 100644
--- a/pkgs/stdenv/darwin/make-bootstrap-tools.nix
+++ b/pkgs/stdenv/darwin/make-bootstrap-tools.nix
@@ -211,7 +211,7 @@ in rec {
};
bootstrapTools = derivation {
- inherit system;
+ inherit (localSystem) system;
name = "bootstrap-tools";
builder = "${bootstrapFiles.tools}/bin/bash";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error can also be replicated in darwin repl with freshBootstrapTools.bootstrapTools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description of changes
Following discussion in:
This removes cross compilation from the
aarch64-darwin
bootstrap tools, and instead builds them with a nativeaarch64-darwin
toolchain.This should fix the currently failing Hydra job. The last successful run was in april 2022, but it was disabled temporarily. Since re-enabling it, some fixes were done, but it now fails on Python, which does not support cross-compiling on Darwin at all. (Perhaps Python was added by accident, but cross-compiling from Intel to ARM will be a problem down the road any way.)
The following builds succeed for me:
And I also have a follow-up commit prepared that actually swaps in the new bootstrap-tools: stephank@7be022c. This successfully builds
stdenv
on bothx86_64-darwin
andaarch64-darwin
. This will eventually be a second PR once Hydra has completed a build, because the URLs currently point to manual builds hosted on my server.Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes