cargo icon indicating copy to clipboard operation
cargo copied to clipboard

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

Open konstin opened this issue 1 year ago • 8 comments

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]

konstin avatar Aug 03 '24 10:08 konstin

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?

edmorley avatar Aug 03 '24 12:08 edmorley

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.

  • Trailing arguments from cargo rustc are appended here
  • -C strip from profile.strip is set later here.

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 avatar Aug 03 '24 15:08 weihanglo

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.

weihanglo avatar Aug 03 '24 15:08 weihanglo

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?)

Kobzol avatar Sep 07 '24 08:09 Kobzol

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

weihanglo avatar Sep 07 '24 08:09 weihanglo

Awesome, thanks!

Kobzol avatar Sep 07 '24 08:09 Kobzol

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:

  • Make the patch enabled on nightly only. And provide an opt-out mechanism while waiting for feedback.
  • Make it clear that interfering flags Cargo controls is not supported and use at your own risk.

weihanglo avatar Sep 20 '24 19:09 weihanglo

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.

weihanglo avatar Sep 25 '24 15:09 weihanglo