bootstrap: fix panic when repo path contains spaces by switching to CARGO_ENCODED_RUSTFLAGS#158073
bootstrap: fix panic when repo path contains spaces by switching to CARGO_ENCODED_RUSTFLAGS#158073Rohan-Singla wants to merge 5 commits into
Conversation
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
| let rustdocflags = &cargo.rustdocflags.0; | ||
| if !rustdocflags.is_empty() { | ||
| cargo.command.env("RUSTDOCFLAGS", rustdocflags); | ||
| cargo.command.env("RUSTDOCFLAGS", rustdocflags.join(" ")); |
There was a problem hiding this comment.
This can use CARGO_ENCODED_RUSTDOCFLAGS.
There was a problem hiding this comment.
Done! Updated to use CARGO_ENCODED_RUSTDOCFLAGS with the \x1f separator for RUSTDOCFLAGS as well, consistent with how RUSTFLAGS is now handeled.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
cc @rust-lang/miri |
|
Can you compare with a previous PR in this area (#156096) and evaluate whether there's anything from there that should also be done here? I'm also curious if we can get some kind of testing in place for this. Maybe one of the CI runners can adjust its build directory to have a space in it? |
Sure i will look into it ! |
|
Updated to use String with \x1f as the internal delimiter (matching #156096's approach) and added the \x1f assertion in arg(). For the CI test, I can add --set build.build-dir with a space in the path to one existing Linux job in jobs.yml would that be the right approach? cc : @Mark-Simulacrum |
|
This looks reasonable to me; at this point it is essentially the same as #156096. I don't think that we have to test this in CI, it will just create another testing hack in a bash script that we can't run locally through bootstrap. We don't test much more important things in bootstrap on our CI.. 😆 |
Thanks! Happy to skip the CI test then. Let me know if there's anything else needed before this can be merged. |
| let rustdocflags = &cargo.rustdocflags.0; | ||
| if !rustdocflags.is_empty() { | ||
| cargo.command.env("RUSTDOCFLAGS", rustdocflags); | ||
| cargo.command.env("CARGO_ENCODED_RUSTDOCFLAGS", rustdocflags); |
There was a problem hiding this comment.
Could we explicitly unset RUSTFLAGS/RUSTDOCFLAGS as well, just to avoid any confusion over those having different values? I assume Cargo will prioritize the encoded form but not sure if downstream things (e.g., build.rs scripts) will do so.
There was a problem hiding this comment.
Done, I now explicitly unset RUSTFLAGS and RUSTDOCFLAGS before setting the encoded forms, so cargo and any build.rs scripts only see CARGO_ENCODED_RUSTFLAGS/CARGO_ENCODED_RUSTDOCFLAGS.
Any flags from the caller's environment have already been folded into the Rustflags struct via propagate_cargo_env, so nothing is lost.
|
r? Kobzol |
|
|
Fixes #158052
Closes: #156096
Problem
./x buildpanics with a cryptic assertion error when the repository ischecked out under a directory path containing spaces (e.g.
/Users/foo/Open Source/rust):thread 'main' panicked at src/bootstrap/src/core/builder/cargo.rs:54:9:
assertion left == right failed
left: 2
right: 1
The root cause: when building tools in
ToolRustcPrivateorCodegenmode, bootstrap calls
llvm-config --libdirand passes the result as a-Clink-arg=-L<path>rustflag. TheRustflags::arg()method assertedthat arguments contain no spaces, but if the repo path has a space the
libdir path inherits it and the assertion fires. The error gives no hint
that the path is the problem.
A secondary bug:
llvm-config --libdiroutput has a trailing newlinethat was previously stripped accidentally by
RUSTFLAGSwhitespacesplitting. Nothing was trimming it explicitly.
Fix
Two changes in
src/bootstrap/src/core/builder/cargo.rs:Switch
RUSTFLAGS→CARGO_ENCODED_RUSTFLAGS: ChangeRustflagsto store args asVec<String>and join with\x1f(ASCII unit separator) when setting the env var. Cargo's
CARGO_ENCODED_RUSTFLAGS(stable since Cargo 1.55) uses\x1fasdelimiter, which never appears in filesystem paths, so paths with
spaces are handled correctly. The space-based assertion in
arg()isremoved.
Trim
llvm-config --libdiroutput: Explicitly.trim()thecaptured stdout so the trailing newline is not included in the linker
search path.
RUSTDOCFLAGSis left as-is (space-joined) since no llvm paths areadded to rustdocflags.
cc: @Kobzol