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.

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

Only dist llvm-objcopy if llvm tools are enabled #134240

Merged
merged 1 commit into from
Dec 14, 2024

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Dec 13, 2024

This uses the same condition that #132720 added in the compilation phase.

r? @jieyouxu

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 13, 2024
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

That makes sense, thanks!

@jieyouxu
Copy link
Member

jieyouxu commented Dec 13, 2024

Actually just one question. @cuviper in #131405 we had to

  • Includes llvm-strip (a symlink to llvm-objcopy) in the compiler dist artifact so that it can be used for -Cstrip instead of the system tooling.
  • Uses llvm-strip instead of /usr/bin/strip for macOS. macOS needs a specific linker and the system one is preferred, hence Fix up setting strip = true in Cargo.toml makes build scripts fail in… #130781 but that doesn't work when cross-compiling, so use the llvm-strip utility instead.

Will changing this for the dist flow prove problematic if the dist defaults + configurations we currently use combined regress that behavior?

I think we definitely should be guarding the dist flow here, like your change does, I just wanted to double-check what our default behavior is.

@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2024
@cuviper
Copy link
Member Author

cuviper commented Dec 13, 2024

The default still has llvm tools enabled, so you have to explicitly opt out -- I think that's fine for the issue you mention, although it doesn't look like we have any fallback on macOS if you built without it:

if sess.target.is_like_osx {
let stripcmd = "rust-objcopy";

For my part, I want to opt out in Linux distro builds, and it's not used there anyway.

@jieyouxu
Copy link
Member

Right, that sounds reasonable to me. Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 13, 2024

📌 Commit 4c6d793 has been approved by jieyouxu

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 13, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 13, 2024
Only dist `llvm-objcopy` if llvm tools are enabled

This uses the same condition that rust-lang#132720 added in the compilation phase.

r? `@jieyouxu`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2024
Only dist `llvm-objcopy` if llvm tools are enabled

This uses the same condition that rust-lang#132720 added in the compilation phase.

r? ```@jieyouxu```
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#134181 (Tweak multispan rendering to reduce output length)
 - rust-lang#134209 (validate `--skip` and `--exclude` paths)
 - rust-lang#134231 (rustdoc-search: fix mismatched path when parent re-exported twice)
 - rust-lang#134236 (crashes: more tests v2)
 - rust-lang#134240 (Only dist `llvm-objcopy` if llvm tools are enabled)
 - rust-lang#134244 (rustc_borrowck: Stop suggesting the invalid syntax `&mut raw const`)
 - rust-lang#134251 (A bunch of cleanups (part 2))
 - rust-lang#134256 (Use a more precise span in placeholder_type_error_diag)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a53a3cc into rust-lang:master Dec 14, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
Rollup merge of rust-lang#134240 - cuviper:dist-llvm-tools, r=jieyouxu

Only dist `llvm-objcopy` if llvm tools are enabled

This uses the same condition that rust-lang#132720 added in the compilation phase.

r? ``@jieyouxu``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants