Skip to content

Add option to strip binaries - #8246

Merged
bors merged 9 commits into
rust-lang:masterfrom
GabrielMajeri:add-strip-option
May 19, 2020
Merged

Add option to strip binaries#8246
bors merged 9 commits into
rust-lang:masterfrom
GabrielMajeri:add-strip-option

Conversation

@GabrielMajeri

@GabrielMajeri GabrielMajeri commented May 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR adds a Cargo option for stripping symbols from generated binaries.

This is based on the -Z strip flag for rustc, which has been recently implemented.

Notes for reviewers: I'm not entirely sure how we test this, I've created a crate locally and it seems to be working.

Supersedes my previous work in #8191.
Fixes #3483.

@rust-highfive

rust-highfive commented May 16, 2020

Copy link
Copy Markdown

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2020
@josephlr

josephlr commented May 18, 2020

Copy link
Copy Markdown

As this is an unstable rustc flag, shouldn't this functionality also be gated behind an unstable Cargo feature in src/cargo/core/features.rs?

josephlr
josephlr reviewed May 18, 2020
View reviewed changes
Comment thread src/cargo/core/profiles.rs Resolved
@josephlr josephlr mentioned this pull request May 18, 2020
Merged
@alexcrichton

alexcrichton commented May 18, 2020

Copy link
Copy Markdown
Member

Thanks for this! I agree yeah that this'll need an unstable opt-in in Cargo as well, but otherwise this is looking good! Mind adding some nightly-only tests for this as well?

@GabrielMajeri

GabrielMajeri commented May 18, 2020

Copy link
Copy Markdown
Contributor Author

I've gated strip behind -Z strip and added a test.

alexcrichton
alexcrichton reviewed May 18, 2020
View reviewed changes

@alexcrichton alexcrichton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

Comment thread src/cargo/util/toml/mod.rs Resolved
Comment thread src/cargo/core/features.rs Resolved
Comment thread tests/testsuite/profiles.rs Resolved
Comment thread tests/testsuite/profiles.rs Resolved
Comment thread tests/testsuite/profiles.rs Resolved
@GabrielMajeri

GabrielMajeri commented May 19, 2020

Copy link
Copy Markdown
Contributor Author

I've addressed the latest review comments.

@alexcrichton

alexcrichton commented May 19, 2020

Copy link
Copy Markdown
Member

@bors: r+

Thanks!

@bors

bors commented May 19, 2020

Copy link
Copy Markdown
Contributor

📌 Commit 2cd41e1 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2020
@bors

bors commented May 19, 2020

Copy link
Copy Markdown
Contributor

⌛ Testing commit 2cd41e1 with merge d18e4b3...

@bors

bors commented May 19, 2020

Copy link
Copy Markdown
Contributor

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing d18e4b3 to master...

@bors
bors merged commit d18e4b3 into rust-lang:master May 19, 2020
@GabrielMajeri
GabrielMajeri deleted the add-strip-option branch May 20, 2020 10:39
@ehuss ehuss mentioned this pull request May 26, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 27, 2020
@bors
Auto merge of rust-lang#72596 - ehuss:update-cargo, r=ehuss
783139b 
Update cargo

7 commits in 500b2bd01c958f5a33b6aa3f080bea015877b83c..9fcb8c1d20c17f51054f7aa4e08ff28d381fe096
2020-05-18 17:12:54 +0000 to 2020-05-25 16:25:36 +0000
- Bump to semver 0.10 for `VersionReq::is_exact` (rust-lang/cargo#8279)
- Fix panic with `cargo tree --target=all -Zfeatures=all` (rust-lang/cargo#8269)
- Fix nightly tests with llvm-tools. (rust-lang/cargo#8272)
- Provide better error messages for a bad `patch`. (rust-lang/cargo#8248)
- Try installing exact versions before updating (rust-lang/cargo#8022)
- Document unstable `strip` profile feature (rust-lang/cargo#8262)
- Add option to strip binaries (rust-lang/cargo#8246)
@joshuarli

joshuarli commented May 31, 2020

Copy link
Copy Markdown

This is great @GabrielMajeri, can't wait for it to land in stable cargo.

I just tried strip = "symbols" on nightly cargo with both x86_64 glibc and musl libc targets, output binaries are identical to those processed with strip --strip-all.

So this completely obviates the need to have strip installed.

@flyingmutant

flyingmutant commented Jun 1, 2020

Copy link
Copy Markdown

Strip::None => "abort", has to be "none" instead, right?

@GabrielMajeri

GabrielMajeri commented Jun 1, 2020

Copy link
Copy Markdown
Contributor Author

Strip::None => "abort", has to be "none" instead, right?

Yep, it's fixed in master

@eggyal eggyal mentioned this pull request Jun 8, 2020
Merged
@yoshuawuyts yoshuawuyts mentioned this pull request Jun 12, 2020
Closed
@Brooooooklyn Brooooooklyn mentioned this pull request Feb 20, 2021
Merged
@ehuss ehuss added this to the 1.45.0 milestone Feb 6, 2022
@yisibl

yisibl commented Feb 23, 2022

Copy link
Copy Markdown

How is this different from using the strip command directly?

@GabrielMajeri

GabrielMajeri commented Feb 23, 2022

Copy link
Copy Markdown
Contributor Author

@yisibl If you're talking about GNU strip, that command only exists/works on Linux or GCC-based toolchains. The -Z strip compiler flag automatically generates and passes down the corresponding flags to the linker, independent of the target platform.

@yisibl

yisibl commented Feb 26, 2022

Copy link
Copy Markdown

@GabrielMajeri Yes, I also use llvm-strip, see: https://github.com/yisibl/resvg-js/blob/main/.github/workflows/CI.yaml#L63-L68

I find that using the strip command directly makes the file size a bit smaller.

@MichalStrehovsky MichalStrehovsky mentioned this pull request May 30, 2022
Closed
@pickfire

pickfire commented Jul 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Will this be stabilized? On rust tracking issue it says cargo should have it by next release since april, but now it's quite some time since then. rust-lang/rust#72110 (comment)

@ehuss

ehuss commented Jul 5, 2022

Copy link
Copy Markdown
Contributor

The cargo strip profile option was stabilized in 1.59. More information is at https://doc.rust-lang.org/cargo/reference/profiles.html#strip. There's also a CHANGELOG if you need to find when changes land. If you're having problems, can you share more information in an issue?

@pickfire

pickfire commented Jul 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Ah, I did see that toml thing. But I thought there is something like -C strip=debuginfo, when I tried that it just errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@alexcrichton alexcrichton alexcrichton left review comments
@josephlr josephlr josephlr left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@Eh2406 Eh2406

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.45.0

Development

Successfully merging this pull request may close these issues.

Option for stripping binary