rust: fix overriding rust flags on musl #221413
Conversation
alyssais
added
6.topic: rust
6.topic: musl
alyssais
requested review from
zowoq,
winterqt,
figsoda and
Mic92
as code owners
|
also see #220660, as they will run into merge conflicts |
If RUSTFLAGS is set in the environment, Cargo will ignore rustflags settings in its TOML configuration. So setting RUSTFLAGS=-g (like separateDebugInfo does) to generate debug info breaks dynamically-linked Rust packages on musl. This breakage is visible for any packages that call into C dynamic libraries. If the binary is linked directly to a C dynamic library, it will fail to build, and if it depends on a Rust library which links a C dynamic library, it will segfault at runtime when it tries to call a function from the C library. I noticed this because pkgsMusl.crosvm is broken for this reason, since it sets separateDebugInfo = true. It shouldn't be possible to end up with broken binaries just by using RUSTFLAGS to do something innocuous like enable debug info, so I think that, even though we liked the approach of modiyfing .cargo/config better at the time, it's become clear that it's too brittle, and we should bite the bullet and patch the compiler instead when targetting musl. It does not appear to be necessary to modify the compiler at all when cross-compiling /from/ dynamically-linked Musl to another target, so I'm only checking whether the target system is dynamically-linked Musl when deciding whether to make the modification to the compiler. This reverts commit c2eaaae ("cargoSetupHook: pass host config flags"), and implements the compiler patching approach instead.
|
In the future we might want to consider creating a proper wrapper for rustc, since this would avoid building two different rustcs to produce dynamically vs statically linked binaries in pkgsCross.musl64 vs pkgsStatic. Also see #211254 #176694 related to reducing the number of different rustc variants we have |
SGTM. I still think your PR is a good idea, even if it just ends up setting the linker. |
|
Alternatively, couldn't you just set |
I don't think so — "Environment variables will take precedence over TOML configuration files." |
|
Cargo tries its best to merge TOML lists. Here is a super dirty one-liner to check. CARGO_BUILD_RUSTFLAGS="--cfg foo" cargo +nightly -Zunstable-options config get build.rustflags --config 'build.rustflags=["--cfg", "bar"]'Or you can Look into the source code. During the deserialization of Thus, I think @lovesegfault is correct. |
|
Okay. I still think it shouldn't be possible for users to break their executables at runtime by setting innocuous RUSTFLAGS, so I still think we should make this change. |
|
I do think that long-term we should find a way to pass the correct crt-static and linker for both host and target (in cargo terms), when the target-applies-to-host feature in cargo is stabilized etc. |
ghost
mentioned this pull request
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi, it's awesome that this PR fixed the musl-is-always-static assumption. But...
It looks like this PR reverted c2eaaae on all platforms but only "implement(ed) the compiler patching approach instead" on Specifically, I'm having a hard time figuring out how to rebase #211868 past this commit. Where is For the time being I've put a partial revert of 470e613 into #211868 but hopefully there is a less radical way to get things done there. |
The issue is that Cargo does not have a proper way of specifying this, without breaking the RUSTFLAGS environment variable. |
|
Nevermind, I misread this as affecting things built by cargo rather than the build of cargo itself.
I might be interested in taking this up. Is there a reason why this is any more difficult than |
Description of changes
If
RUSTFLAGSis set in the environment, Cargo will ignorerustflagssettings in its TOML configuration. So settingRUSTFLAGS=-g(likeseparateDebugInfodoes) to generate debug info breaks dynamically-linked Rust packages on musl. This breakage is visible for any packages that call into C dynamic libraries. If the binary is linked directly to a C dynamic library, it will fail to build, and if it depends on a Rust library which links a C dynamic library, it will segfault at runtime when it tries to call a function from the C library. I noticed this becausepkgsMusl.crosvmis broken for this reason, since it setsseparateDebugInfo = true.It shouldn't be possible to end up with broken binaries just by using
RUSTFLAGSto do something innocuous like enable debug info, so I think that, even though we liked the approach of modifying.cargo/configbetter at the time, it's become clear that it's too brittle, and we should bite the bullet and patch the compiler instead when targeting musl. It does not appear to be necessary to modify the compiler at all when cross-compiling from dynamically-linked musl to another target, so I'm only checking whether the target system is dynamically-linked musl when deciding whether to make the modification to the compiler.This reverts commit c2eaaae ("cargoSetupHook: pass host config flags") (#198311), and implements the compiler patching approach instead, as originally proposed in #190796.
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/)