rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.79k stars 2.42k forks source link

Precedence of `cargo rustc -- <flags>` is too low to override flags set by Cargo #14346

Open konstin opened 3 months ago

konstin commented 3 months ago

Original issue title: -C strip=symbols doesn't work like strip = true

Problem

Reproducer: https://github.com/konstin/strip-reproducer/blob/main/compile.sh

With the default linker on linux, passing -C link-arg=-s, -C strip=symbols and -C strip=debuginfo to cargo rustc all don't strip the binary properly as setting strip = true does, even though the docs say "The strip option controls the -C strip flag".

In maturin we expose a --strip option, since small size is important for publishing. We pass this on to cargo rustc, historically with -C link-arg=-s, but i also tried with -C strip=symbols. This does strip some debuginfo, 40MB -> 37MB in the case of uv, but not down to 30MB for the fully stripped binary (https://github.com/astral-sh/uv/issues/5683). In the minimal reproducer, only strip = true shows any effect.

Note that this only happens with the default linker, mold strips properly in both cases.

Steps

https://github.com/konstin/strip-reproducer/blob/main/compile.sh

Create a new project with cargo new strip-reproducer, then run:

cargo build -q --release
echo "Release: $(stat --printf="%s" target/release/strip-reproducer)"

cargo rustc -q --release -- -C strip=symbols
echo "strip=symbols: $(stat --printf="%s" target/release/strip-reproducer)"

cargo rustc -q --release -- -C strip=debuginfo
echo "strip=debuginfo: $(stat --printf="%s" target/release/strip-reproducer)"

printf "[profile.release]\nstrip = true\n" >> Cargo.toml
cargo build -q --release
echo "strip = true: $(stat --printf="%s" target/release/strip-reproducer)"
Release: 398360
strip=symbols: 398360
strip=debuginfo: 398360
strip = true: 334304

Possible Solution(s)

There should ideally be a cli option to strip like strip = true does, or the docs should be adjusted on the differences between strip = true and the -C options.

Notes

No response

Version

cargo 1.80.0 (376290515 2024-07-16)
release: 1.80.0
commit-hash: 37629051518c3df9ac2c1744589362a02ecafa99
commit-date: 2024-07-16
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Ubuntu 24.4.0 (noble) [64-bit]
edmorley commented 3 months ago

I had a hunch that this might be related to #13257, which landed in Rust 1.77.0.

Trying with Rust 1.76.0 it doesn't reproduce:

$ ./compile.sh
Release: 400608
strip=symbols: 313936
strip=debuginfo: 368304
strip = true: 313936

But with 1.77.0 it does:

$ ./compile.sh
Release: 367968
strip=symbols: 367968
strip=debuginfo: 367968
strip = true: 313968

So it does seem like #13257 could be the cause?

I'm presuming the command passed to rustc ends up being -C strip=symbols -C strip=debuginfo (or similar) and thus the strip mode gets overridden?

weihanglo commented 3 months ago

I'm presuming the command passed to rustc ends up being -C strip=symbols -C strip=debuginfo (or similar) and thus the strip mode gets overridden?

Yeah your observation is correct. It's about precedence.

I have no idea why rustflags from cargo rustc got such a lower precendence. Every change in rustc invocation from Cargo may break the usage of it. Also, the precedence is not well-documented.

I am not sure how to document the behavior. Adding rustflags is usually a lower level thing, and the same regression might happen again for other kind of rustflags. Generally using [profile] is recommended if a rustflag is already control over profiles.

weihanglo commented 3 months ago

Another approach is we move cargo rustc -- <rustflags> to one line below:

https://github.com/rust-lang/cargo/blob/d3e84f01ff48d19892035b77db7e9c4eda87bf1f/src/cargo/core/compiler/mod.rs#L686

It has less impact to the precedence of other user-overridable rustflags (build.rustflags and RUSTFLAGS). I am not 100% sure about the implication and impact though.

Kobzol commented 2 months ago

Hmm, I didn't think of that. Yeah, this is tricky, but it also happens with other rustc flags set by Cargo, it just didn't happen for split specifically before 1.77.

If you use the CARGO_<PROFILE>_STRIP env. var. or just use RUSTFLAGS, it should work, however as @weihanglo said, cargo rustc flags have low priority. It seems to me like that is a very unexpected behavior, as cargo rustc is specifically designed to have control over how exactly is rustc invoked, and it is thus quite unintuitive that flags passed after cargo rustc have lower priority than flags passed by Cargo - that essentially makes these flags useless, and does not allow overriding the behavior of Cargo.

@weihanglo I think that it would make sense to raise the priority of the cargo rustc passed flags. What process should be used to achieve that? :) (RFC/PR/Zulip issue?)

weihanglo commented 2 months ago

I have already put this on the Cargo triage agenda, but I was out for family issues for the past two weeks. A zulip thread sounds good to me as a start

Kobzol commented 2 months ago

Awesome, thanks!

weihanglo commented 1 month ago

See https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/rustflags.20precendence.20of.20.60cargo.20rustc.60.

This is kinda accepted with some requirements:

weihanglo commented 1 month ago

Re-opened to track the stabilization of #14587.

In case of the new precedence start blocking your builds, please let us know and use __CARGO_RUSTC_ORIG_ARGS_PRIO=1 to restore to the original precedence.