Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
konstin opened this issue Aug 3, 2024 · 8 comments · Fixed by #14587 · May be fixed by #14900
Open

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

konstin opened this issue Aug 3, 2024 · 8 comments · Fixed by #14587 · May be fixed by #14900
Labels
A-rustflags Area: rustflags C-bug Category: bug Command-rustc S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns.

Comments

@konstin
Copy link
Contributor

konstin commented Aug 3, 2024 •
edited by weihanglo
Loading

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 (astral-sh/uv#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 konstin added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Aug 3, 2024
@konstin konstin mentioned this issue Aug 3, 2024
Closed
konstin added a commit to astral-sh/uv that referenced this issue Aug 3, 2024
@konstin
Always strip in release mode
f47f088 
See rust-lang/cargo#14346

Fixes #5683
@konstin konstin mentioned this issue Aug 3, 2024
Merged
konstin added a commit to astral-sh/uv that referenced this issue Aug 3, 2024
@konstin
Always strip in release mode (#5745)
0098173 
Decreases linux release wheel size by 2MB.

See rust-lang/cargo#14346

Fixes #5683
@edmorley
Copy link

edmorley commented Aug 3, 2024

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
Copy link
Member

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 weihanglo added Command-rustc A-rustflags Area: rustflags S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-triage Status: This issue is waiting on initial triage. labels Aug 3, 2024
@weihanglo
Copy link
Member

weihanglo commented Aug 3, 2024 •
edited
Loading

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

cargo/src/cargo/core/compiler/mod.rs

Line 686 in d3e84f0

base.args(&unit.rustflags);

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.

konstin added a commit to PyO3/maturin that referenced this issue Aug 4, 2024
@konstin
Use modern stripping option
68f297b 
Use `-C strip=symbols` instead of `-C link-arg=-s`. Still broken due to rust-lang/cargo#14346, but i think it's the right modernization anyway.
@konstin konstin mentioned this issue Aug 4, 2024
Merged
messense pushed a commit to PyO3/maturin that referenced this issue Aug 5, 2024
@konstin
Use modern stripping option (#2173)
ed8ba1e 
Use `-C strip=symbols` instead of `-C link-arg=-s`. Still broken due to rust-lang/cargo#14346, but i think it's the right modernization anyway.
@Kobzol
Copy link
Contributor

Kobzol commented Sep 7, 2024

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
Copy link
Member

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
Copy link
Contributor

Kobzol commented Sep 7, 2024

Awesome, thanks!

@weihanglo
Copy link
Member

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 weihanglo added E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. labels Sep 20, 2024
@weihanglo weihanglo mentioned this issue Sep 24, 2024
Merged
@bors bors closed this as completed in 01e1ab5 Sep 25, 2024
@weihanglo weihanglo reopened this Sep 25, 2024
@weihanglo
Copy link
Member

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 weihanglo changed the title -C strip=symbols doesn't work like strip = true Precedence of cargo rustc -- <flags> is too low to override flags set by Cargo Sep 25, 2024
@weihanglo weihanglo added the S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. label Sep 25, 2024
@weihanglo weihanglo removed E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review labels Oct 10, 2024
@weihanglo weihanglo linked a pull request Dec 6, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-rustflags Area: rustflags C-bug Category: bug Command-rustc S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns.
Projects
None yet
Milestone
No milestone
4 participants
@edmorley @Kobzol @konstin @weihanglo