rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation34 Commits2 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Fixes #131206.

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.

cc #123151

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

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)

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Oct 8, 2024

We could make this same change to the illumos and AIX targets which also hardcode a path to a strip binary. Pinging relevant people for those targets to see if this would be desirable.

@ecnelises @bzEq (listed as target maintainers for powerpc64-ibm-aix
@jclulow (we have no target maintainers for the illumos targets in the documentation, but I've seen @jclulow in the issue tracker and I know is involved in illumos)

This comment was marked as resolved.

@@ -1151,6 +1151,11 @@ impl<'a> Builder<'a> {
self.ensure(compile::Sysroot::new(compiler))
}

/// Returns the bindir for a compiler's sysroot.

Choose a reason for hiding this comment

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

Probably t-bootstrap will have opinions here, because there are 2 "bin" directories 😓

Thank you for posting this. I tried twice and couldn't even figure out where to start.

illumos

AFAIK some change is needed for illumos too because using the default strip binary of Linux host causes a problem (oxidecomputer/helios#147).
However, I have not actually tested to see if using LLVM strip would solve the problem. (This comment may indicate that anything other than native strip utility would not work.)

cc @sunshowers

Comment on lines +1152 to +1160

let mut new_path = sess.get_tools_search_paths(false);
if let Some(path) = env::var_os("PATH") {
new_path.extend(env::split_paths(&path));
}
cmd.env("PATH", env::join_paths(new_path).unwrap());

Choose a reason for hiding this comment

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

also: it's probably safer to look for rust-objcopy in the tools search path and launch w/ the absolute path cmd rather than overriding the $PATH?

Choose a reason for hiding this comment

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

It probably is but we override the PATH for rust-lld so I'll keep it this way and we can change both together later.

Choose a reason for hiding this comment

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

Ah, do we? I remember we set the linker search path to the lld wrappers in the sysroot without overriding the PATH (so they should execute ../rust-lld without overrides), and that rustup ensures the PATH contains the bin and rustlib/bin folders, but didn’t remember rustc overrode it for rust-lld.

Choose a reason for hiding this comment

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

cmd.env("PATH", env::join_paths(new_path).unwrap());

I believe this is where it happens

Choose a reason for hiding this comment

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

Yeah that's indeed likely it for the targets involving the linker directly. We don't need/make use of that on the common linux/mac targets.

@jclulow (we have no target maintainers for the illumos targets in the documentation, but I've seen @jclulow in the issue tracker and I know is involved in illumos)

Hey, with my apologies, someone pointed this out to me recently and I owe a PR to fix the docs and list some maintainers there.

We could make this same change to the illumos and AIX targets which also hardcode a path to a strip binary. Pinging relevant people for those targets to see if this would be desirable.

When we're running on an illumos host and compiling for an illumos target, we definitely want to use the system strip binary explicitly. If we're cross compiling, either on an illumos host for a non-illumos target, or on a non-illumos host, I don't have a particular preference and we should do whatever is tested to work there.

Does that make sense?

Thanks for headsup. On AIX, we are still using /usr/bin/strip. llvm-strip is not compatible with AIX's system strip utility.

This was referenced

Oct 10, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Oct 10, 2024

…nnethercote

Fix hardcoded strip path when cross-compiling from Linux to Darwin

Fixes rust-lang#131206.

I fear that rust-lang#131405 might end up taking some time, so opening this PR to resolve the regression.

@rustbot label O-apple

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Oct 10, 2024

Rollup merge of rust-lang#131480 - madsmtm:macos-fix-strip-binary, r=nnethercote

Fix hardcoded strip path when cross-compiling from Linux to Darwin

Fixes rust-lang#131206.

I fear that rust-lang#131405 might end up taking some time, so opening this PR to resolve the regression.

@rustbot label O-apple

This comment was marked as resolved.

Thanks for headsup. On AIX, we are still using /usr/bin/strip. llvm-strip is not compatible with AIX's system strip utility.

Okay, I've kept AIX's logic as it is, but I have added a warning when cross-compiling that whatever is in /usr/bin/strip may not work because it isn't AIX's system strip.

When we're running on an illumos host and compiling for an illumos target, we definitely want to use the system strip binary explicitly. If we're cross compiling, either on an illumos host for a non-illumos target, or on a non-illumos host, I don't have a particular preference and we should do whatever is tested to work there.

Does that make sense?

That makes sense. I've kept /usr/bin/strip if not cross-compiling, and we'll now use llvm-strip instead of /usr/bin/strip when cross-compiling (that seems more likely to work reliably than relying on whatever /usr/bin/strip could be). I also changed the condition we used here from target.os == "illumos" to target.is_like_solaris, since that's why we have is_like_solaris in the first place. Let me know if that's not correct and there is some difference with stripping between illumos and solaris in general.

Choose a reason for hiding this comment

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

LGTM for the technical side in bootstrap

r? t-compiler for the compiler side of things

This comment was marked as resolved.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Nov 6, 2024

…iaskrgr

Rollup of 5 pull requests

Successful merges:

rust-lang#131261 (Stabilize UnsafeCell::from_mut)
rust-lang#131405 (bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip)
rust-lang#132077 (Add a new wide-arithmetic feature for WebAssembly)
rust-lang#132562 (Remove the wasm32-wasi target from rustc)
rust-lang#132660 (Remove unused errs.rs file)

Failed merges:

rust-lang#131721 (Add new unstable feature const_eq_ignore_ascii_case)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Nov 6, 2024

Rollup merge of rust-lang#131405 - davidtwco:hardcoded-strip-macos, r=jieyouxu,albertlarsan68

bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip

Fixes rust-lang#131206.

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 rust-lang#130781 but that doesn't work when cross-compiling, so use the llvm-strip utility instead.

cc rust-lang#123151

This was referenced

Nov 7, 2024

ehuss mentioned this pull request

Nov 11, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Nov 11, 2024

Only copy, rename and link llvm-objcopy if llvm tools are enabled

Fixes rust-lang#132719.

cc @bjorn3 who reported the bootstrapping problem for cg_clif. cc @davidtwco in case this might be problematic for linux -> macOS cross-compile builds, but seems very unlikely. cc @albertlarsan68 (co-reviewed rust-lang#131405)

r? bootstrap

mati865 pushed a commit to mati865/rust that referenced this pull request

Nov 12, 2024

Only copy, rename and link llvm-objcopy if llvm tools are enabled

Fixes rust-lang#132719.

r? bootstrap

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Dec 3, 2024

…ution, r=onur-ozkan

Unify sysroot_target_{bin,lib}dir handling

Follow-up to rust-lang#131405 (comment) where sysroot_target_bindir had to do some dancing because the sysroot ensure logic embedded in sysroot_target_libdir returned $sysroot/$relative_lib/rustlib/$target/lib and not the rustlib parent $sysroot/$relative_lib/rustlib/.

This PR pulls out the sysroot ensure logic into a helper, and return $sysroot/$relative_lib/rustlib/ instead so sysroot_target_bindir doesn't have to do parent traversal from the path returned from sysroot_target_libdir, and also make them easier to follow in that they are now clearly closely related based on the common target sysroot ensure logic.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Dec 3, 2024

Rollup merge of rust-lang#132723 - jieyouxu:sysroot-dance-dance-revolution, r=onur-ozkan

Unify sysroot_target_{bin,lib}dir handling

orlp mentioned this pull request

Jan 2, 2025

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 2, 2025

Pass objcopy args for stripping on OSX

When -Cstrip was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

But strip and objcopy are, while being the same binary, different, they have different defaults depending on which binary they are. Notably, strip strips everything by default, and objcopy doesn't strip anything by default.

Additionally, -S actually means --strip-all, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass --strip-debug and --strip-all.

fixes rust-lang#135028

try-jobs: aarch64-apple

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 2, 2025

Pass objcopy args for stripping on OSX

When -Cstrip was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

Additionally, -S actually means --strip-all, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass --strip-debug and --strip-all.

fixes rust-lang#135028

try-jobs: aarch64-apple try-jobs: dist-aarch64-apple

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 2, 2025

Pass objcopy args for stripping on OSX

When -Cstrip was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

Additionally, -S actually means --strip-all, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass --strip-debug and --strip-all.

fixes rust-lang#135028

try-job: aarch64-apple try-job: dist-aarch64-apple

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 2, 2025

Pass objcopy args for stripping on OSX

When -Cstrip was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

Additionally, -S actually means --strip-all, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass --strip-debug and --strip-all.

fixes rust-lang#135028

try-job: aarch64-apple try-job: dist-aarch64-apple

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 3, 2025

Pass objcopy args for stripping on OSX

When -Cstrip was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

Additionally, -S actually means --strip-all, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass --strip-debug and --strip-all.

fixes rust-lang#135028

try-job: aarch64-apple try-job: dist-aarch64-apple

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 3, 2025

Pass objcopy args for stripping on OSX

When -Cstrip was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

Additionally, -S actually means --strip-all, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass --strip-debug and --strip-all.

fixes rust-lang#135028

try-job: aarch64-apple try-job: dist-aarch64-apple

lqd mentioned this pull request

Apr 17, 2025

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)

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.