pkgsMusl.cargo: fix build #190796
pkgsMusl.cargo: fix build #190796
Conversation
alyssais
added
6.topic: rust
6.topic: musl
|
Can you add code comments for these please? |
| @@ -136,6 +136,9 @@ in stdenv.mkDerivation rec { | |||
|
|
|||
| # Useful debugging parameter | |||
| # export VERBOSE=1 | |||
|
|
|||
| substituteInPlace compiler/rustc_target/src/spec/linux_musl_base.rs \ | |||
There was a problem hiding this comment.
Could make this conditional to stdenv.hostPlatform.isMusl too, since otherwise it changes the behavior for people using the pkgsStatic.buildPackages.rustc manually. They would now have to manually set +crt-static to get a static binary, which they might not expect since upstream is explicitly keeping the default.
There was a problem hiding this comment.
Actually I don't think it's necessary. Most people who do this kind of stuff manually will use rustup.
There was a problem hiding this comment.
I think it's a good idea not to change the upstream behaviour where a user hasn't explicitly requested it, so I'm going to make this change. But I think it should be stdenv.targetPlatform so it's correct when building a musl cross compiler.
There was a problem hiding this comment.
But I think it should be stdenv.targetPlatform so it's correct when building a musl cross compiler.
Not sure about this. When the compiler is a cross-compiler from glibc to musl, the build.rs scripts will also run on glibc and this setting does not matter.
If I remember correctly what hostPlatform and targetPlatform are...
For example, targetPlatform.isMusl would be true for pkgsStatic.buildPackages.rustc, even though the change is not needed there?
Or the opposite, if we are building a cross-compiler from musl to glibc, targetPlatform.isMusl will be false, but it should still apply the change because the build.rs will be run with musl.
There was a problem hiding this comment.
Not sure about this. When the compiler is a cross-compiler from glibc to musl, the build.rs scripts will also run on glibc and this setting does not matter.
If I remember correctly what hostPlatform and targetPlatform are...
But this is the change for the default value of crt-static for all Musl builds in rustc, not the one for build.rs scripts in Cargo.
For example, targetPlatform.isMusl would be true for pkgsStatic.buildPackages.rustc, even though the change is not needed there?
Yes, the actual check I would do here is stdenv.targetPlatform.isMusl && !stdenv.targetPlatform.isStatic.
Or the opposite, if we are building a cross-compiler from musl to glibc, targetPlatform.isMusl will be false, but it should still apply the change because the build.rs will be run with musl.
Not sure about this one. I'll have to test it.
There was a problem hiding this comment.
Or the opposite, if we are building a cross-compiler from musl to glibc, targetPlatform.isMusl will be false, but it should still apply the change because the build.rs will be run with musl.
Not sure about this one. I'll have to test it.
I tested this by building pkgsMusl.pkgsCross.aarch64-multiplatform.{fd,cargo}, and both built fine even without this change. I don't entirely understand why, but it seems like just enabling it when targetPlatform is non-static Musl works fine.
bobby285271
added
the
12.approvals: 1
|
Builds for me, nixos x86_64 |
There was a problem hiding this comment.
Great investigation and creative solution!
Could you replicate your rationale/reasoning as a comment in the postPatch of cargo.nix?
Then add a c.f. in the substituteInPlace that the reader should go look in the cargo drv for an explanation.
Just trying to make sure we don't add more Chesterton's fences :P
|
Update on this PR: I'm trying to fix cross-compilation of rustc first, so I can check I got the build/host/target in this PR right. Then I'll come back round to this. |
I will do, but IMO it's not really a Chesterton's fence if it's explained in this amount of detail in the commit message. :) |
alyssais
added
the
2.status: merge conflict
Co-authored-by: Alyssa Ross <hi@alyssa.is>
Unfortunately, Cargo does not support e.g. a [host.'cfg(target_env = "musl")'] key (it only supports it for "target"), so we have to patch this conditionally to avoid it messing with non-Musl targets. Fixes: NixOS#179242
ofborg
bot
removed
the
2.status: merge conflict
|
New version:
Builds I have tested (from x86_64) (with #196333 merged in):
|
|
Closing in favour of #198311. |
Description of changes
At long last, this finally fixes the build of even quite complex Rust packages with
pkgsMusl.Upstream rustc still assumes that musl = static. The fix for this is to disable crt-static by default for musl targets.
For every package apart from Cargo, we can fix this by just patching rustc to not have crt-static by default. But Cargo is built with the upstream bootstrap binary for rustc, which we can't easily patch. This means we need to find another way to make sure crt-static is not used during the build of pkgsMusl.cargo.
By default, Cargo doesn't apply Rust flags when building build.rs if
--targetis passed. The only good fix for this is to use the unstable-Zhost-configCargo feature, which allows us to specify flags that should be passed to rustc when building for the (in Nixpkgs parlance) build system (e.g. for build.rs scripts).Unfortunately, Cargo does not support e.g. a
[host.'cfg(target_env = "musl")']key (it only supports it for "target"), so we have to patch this conditionally to avoid it messing with non-Musl targets.I haven't taken most of @yu-re-ka's patches from #186466, just because they weren't strictly necessary to get them to work. They're probably still worth looking at.
I've tested
fd,pkgsMusl.fd, andpkgsStatic.fd. I checked that the static one is still static, and that thepkgsMuslone is dynamically linked.Things done
sandbox = trueset 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.shto update generated release notes