Skip to content

Use shell-words to parse output from llvm-config#152712

Merged
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
eggyal:quote-lib-paths
Mar 3, 2026
Merged

Use shell-words to parse output from llvm-config#152712
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
eggyal:quote-lib-paths

Conversation

@eggyal
Copy link
Contributor

@eggyal eggyal commented Feb 16, 2026

View all comments

llvm-config might output paths that contain spaces, in which case the naive approach of splitting on whitespace breaks; instead we ask llvm-config to quote any paths and use the shell-words crate by @tmiasko (a new dependency) to parse the output.

r? ChrisDenton
Fixes #152707

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2026

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-tidy Area: The tidy tool 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 Feb 16, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2026

ChrisDenton is currently at their maximum review capacity.
They may take a while to respond.

@eggyal
Copy link
Contributor Author

eggyal commented Feb 16, 2026

I haven't added a test for this. Not entirely sure how we can do so, if it's desired?

@rust-log-analyzer

This comment has been minimized.

@ChrisDenton
Copy link
Member

Oh, it seems --quote-paths is too new for the oldest llvm version we use. Either we need to delay this or else add some logic to detect if it's supported.

@ChrisDenton
Copy link
Member

I haven't added a test for this. Not entirely sure how we can do so, if it's desired?

Properly checking would require checking out the source into a directory with a space in its name and trying to build from there. That may be kind of expensive in CI time though. Maybe it would be sufficient to have a wrapper we can unit test.

@eggyal

This comment was marked as outdated.

@ChrisDenton
Copy link
Member

If a backport is approved then that's definitely better!

@eggyal
Copy link
Contributor Author

eggyal commented Feb 16, 2026

Hm, no, that commit is already in the llvm-project submodule that we're using.

@ChrisDenton
Copy link
Member

Looking again, it seems to be using the system llvm so our backports won't help: /usr/lib/llvm-20/bin/llvm-config.

@eggyal
Copy link
Contributor Author

eggyal commented Feb 16, 2026

Now determining whether llvm-config supports --quote-paths by searching for "quote-paths" in the output of llvm-config --help.

llvm-config might output paths that contain spaces, in which case the
naive approach of splitting on whitespace breaks; instead we ask
llvm-config to quote any paths and use the shell-words crate to parse
the output.
@Zalathar
Copy link
Member

I haven't added a test for this. Not entirely sure how we can do so, if it's desired?

From looking at LLVM's sys::printArg (in src/llvm-project/llvm/lib/Support/Program.cpp), it seems that if a path contains any of " \ $, then LLVM will quote the whole thing even if it contains no spaces.

So even if we aren't specifically testing the quoted-spaces case, if this continues to work in Windows CI jobs then that should be a pretty decent proxy-test for quote-aware parsing.

(I'm assuming that on Windows, filesystem paths will naturally tend to contain \, though I haven't confirmed this.)

@eggyal
Copy link
Contributor Author

eggyal commented Feb 17, 2026

if this continues to work in Windows CI jobs then that should be a pretty decent proxy-test for quote-aware parsing.

Provided one or more of the Windows CI jobs are using LLVM 22?

@Zalathar
Copy link
Member

As I understand it, almost all CI jobs use the in-tree LLVM submodule (which is currently 22), except for the ones that specifically exist to test a numbered out-of-tree LLVM.

@ChrisDenton
Copy link
Member

Ok, the new dependency looks good to me (and david has checked the license) so I think we're fine on that front. This looks good to go, thanks!

@bors r+

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 3, 2026

📌 Commit c30e20a has been approved by ChrisDenton

It is now in the queue for this repository.

@rust-bors rust-bors bot 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 Mar 3, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Mar 3, 2026
Use shell-words to parse output from llvm-config

llvm-config might output paths that contain spaces, in which case the naive approach of splitting on whitespace breaks; instead we ask llvm-config to quote any paths and use the [shell-words](https://crates.io/crates/shell-words) crate by @tmiasko (a new dependency) to parse the output.

r? ChrisDenton
Fixes rust-lang#152707
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Mar 3, 2026
Use shell-words to parse output from llvm-config

llvm-config might output paths that contain spaces, in which case the naive approach of splitting on whitespace breaks; instead we ask llvm-config to quote any paths and use the [shell-words](https://crates.io/crates/shell-words) crate by @tmiasko (a new dependency) to parse the output.

r? ChrisDenton
Fixes rust-lang#152707
rust-bors bot pushed a commit that referenced this pull request Mar 3, 2026
…uwer

Rollup of 4 pull requests

Successful merges:

 - #152164 (Lint unused features)
 - #152712 (Use shell-words to parse output from llvm-config)
 - #153223 (Fix LegacyKeyValueFormat report from docker build: host-aarch64)
 - #153345 (MGCA: fix type error handling for const array and tuple lowering logic)
rust-bors bot pushed a commit that referenced this pull request Mar 3, 2026
…uwer

Rollup of 4 pull requests

Successful merges:

 - #152712 (Use shell-words to parse output from llvm-config)
 - #152911 (Stabilize `control_flow_ok`)
 - #153223 (Fix LegacyKeyValueFormat report from docker build: host-aarch64)
 - #153345 (MGCA: fix type error handling for const array and tuple lowering logic)
rust-bors bot pushed a commit that referenced this pull request Mar 3, 2026
Use shell-words to parse output from llvm-config

llvm-config might output paths that contain spaces, in which case the naive approach of splitting on whitespace breaks; instead we ask llvm-config to quote any paths and use the [shell-words](https://crates.io/crates/shell-words) crate by @tmiasko (a new dependency) to parse the output.

r? ChrisDenton
Fixes #152707
@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 3, 2026

⌛ Testing commit c30e20a with merge bf3d4c6...

@JonathanBrouwer
Copy link
Contributor

@bors yield
Yielding to enclosing rollup

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 3, 2026

Auto build cancelled. Cancelled workflows:

The next pull request likely to be tested is #153355.

rust-bors bot pushed a commit that referenced this pull request Mar 3, 2026
…uwer

Rollup of 4 pull requests

Successful merges:

 - #152712 (Use shell-words to parse output from llvm-config)
 - #152911 (Stabilize `control_flow_ok`)
 - #153223 (Fix LegacyKeyValueFormat report from docker build: host-aarch64)
 - #153345 (MGCA: fix type error handling for const array and tuple lowering logic)
@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 3, 2026

rust-bors bot pushed a commit that referenced this pull request Mar 3, 2026
Use shell-words to parse output from llvm-config

llvm-config might output paths that contain spaces, in which case the naive approach of splitting on whitespace breaks; instead we ask llvm-config to quote any paths and use the [shell-words](https://crates.io/crates/shell-words) crate by @tmiasko (a new dependency) to parse the output.

r? ChrisDenton
Fixes #152707
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@JonathanBrouwer
Copy link
Contributor

@bors yield
Yielding to enclosing rollup

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 3, 2026

Auto build cancelled. Cancelled workflows:

The next pull request likely to be tested is #153355.

rust-bors bot pushed a commit that referenced this pull request Mar 3, 2026
…uwer

Rollup of 4 pull requests

Successful merges:

 - #152712 (Use shell-words to parse output from llvm-config)
 - #152911 (Stabilize `control_flow_ok`)
 - #153223 (Fix LegacyKeyValueFormat report from docker build: host-aarch64)
 - #153345 (MGCA: fix type error handling for const array and tuple lowering logic)
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bjorn3
Copy link
Member

bjorn3 commented Mar 3, 2026

How does shell-words compare with shlex? The latter is already a dependency of the cc crate.

@rust-bors rust-bors bot merged commit 6a44bbd into rust-lang:main Mar 3, 2026
11 of 12 checks passed
@rustbot rustbot added this to the 1.96.0 milestone Mar 3, 2026
rust-timer added a commit that referenced this pull request Mar 3, 2026
Rollup merge of #152712 - eggyal:quote-lib-paths, r=ChrisDenton

Use shell-words to parse output from llvm-config

llvm-config might output paths that contain spaces, in which case the naive approach of splitting on whitespace breaks; instead we ask llvm-config to quote any paths and use the [shell-words](https://crates.io/crates/shell-words) crate by @tmiasko (a new dependency) to parse the output.

r? ChrisDenton
Fixes #152707
@eggyal
Copy link
Contributor Author

eggyal commented Mar 3, 2026

I wasn't aware of shlex, but from a cursory glance it actually looks slightly better suited as we can avoid parsing into a Vec<String> and instead use the Shlex iterator.

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Mar 5, 2026
…risDenton

Use shlex instead of shell-words

In rust-lang#152712, the [`shell-words`] crate was introduced as a new dependency of `rustc_llvm` in order for its build script to parse the output of `llvm-config --quote-paths` and thereby handle paths containing whitespace; however, as [noted by bjorn3], that build script already transitively depends upon the [`shlex`] crate (via the [`cc`] crate) which provides similar functionality.

This patch is based off (the already-approved) rust-lang#153419, which would otherwise conflict.

[`cc`]: https://crates.io/crates/cc
[noted by bjorn3]: rust-lang#152712 (comment)
[`shell-words`]: https://crates.io/crates/shell-words
[`shlex`]: https://crates.io/crates/shlex

r? ChrisDenton
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Mar 5, 2026
…risDenton

Use shlex instead of shell-words

In rust-lang#152712, the [`shell-words`] crate was introduced as a new dependency of `rustc_llvm` in order for its build script to parse the output of `llvm-config --quote-paths` and thereby handle paths containing whitespace; however, as [noted by bjorn3], that build script already transitively depends upon the [`shlex`] crate (via the [`cc`] crate) which provides similar functionality.

This patch is based off (the already-approved) rust-lang#153419, which would otherwise conflict.

[`cc`]: https://crates.io/crates/cc
[noted by bjorn3]: rust-lang#152712 (comment)
[`shell-words`]: https://crates.io/crates/shell-words
[`shlex`]: https://crates.io/crates/shlex

r? ChrisDenton
rust-timer added a commit that referenced this pull request Mar 5, 2026
Rollup merge of #153436 - eggyal:shlex-not-shell_words, r=ChrisDenton

Use shlex instead of shell-words

In #152712, the [`shell-words`] crate was introduced as a new dependency of `rustc_llvm` in order for its build script to parse the output of `llvm-config --quote-paths` and thereby handle paths containing whitespace; however, as [noted by bjorn3], that build script already transitively depends upon the [`shlex`] crate (via the [`cc`] crate) which provides similar functionality.

This patch is based off (the already-approved) #153419, which would otherwise conflict.

[`cc`]: https://crates.io/crates/cc
[noted by bjorn3]: #152712 (comment)
[`shell-words`]: https://crates.io/crates/shell-words
[`shlex`]: https://crates.io/crates/shlex

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

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-tidy Area: The tidy tool 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle lib paths with spaces in compiler/rustc_llvm/build.rs

7 participants