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

Add option to strip binaries #8246

Merged
merged 9 commits into from
May 19, 2020
Merged

Add option to strip binaries #8246

merged 9 commits into from
May 19, 2020

Conversation

GabrielMajeri
Copy link
Contributor

@GabrielMajeri GabrielMajeri commented May 16, 2020 •
edited
Loading

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

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

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 mentioned this pull request May 18, 2020
Merged
@alexcrichton
Copy link
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
Copy link
Contributor Author

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

alexcrichton
alexcrichton reviewed May 18, 2020
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looking good!

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/core/features.rs Outdated Show resolved Hide resolved
tests/testsuite/profiles.rs Show resolved Hide resolved
tests/testsuite/profiles.rs Show resolved Hide resolved
tests/testsuite/profiles.rs Show resolved Hide resolved
@GabrielMajeri
Copy link
Contributor Author

I've addressed the latest review comments.

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Collaborator

bors commented May 19, 2020

📌 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
Copy link
Collaborator

bors commented May 19, 2020

⌛ Testing commit 2cd41e1 with merge d18e4b3...

@bors
Copy link
Collaborator

bors commented May 19, 2020

☀️ 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
Copy link

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

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

@GabrielMajeri
Copy link
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
Copy link

yisibl commented Feb 23, 2022

How is this different from using the strip command directly?

@GabrielMajeri
Copy link
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
Copy link

yisibl commented Feb 26, 2022

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

pickfire commented Jul 5, 2022 •
edited
Loading

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

ehuss commented Jul 5, 2022

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

pickfire commented Jul 5, 2022 •
edited
Loading

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

@alexcrichton alexcrichton alexcrichton left review comments

@josephlr josephlr josephlr left review comments

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