Skip to content

Fix argument parsing in compiletest for quoted strings#154479

Open
JumpiiX wants to merge 1 commit into
rust-lang:mainfrom
JumpiiX:fix-132599-compiletest-arg-splitting
Open

Fix argument parsing in compiletest for quoted strings#154479
JumpiiX wants to merge 1 commit into
rust-lang:mainfrom
JumpiiX:fix-132599-compiletest-arg-splitting

Conversation

@JumpiiX

@JumpiiX JumpiiX commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

Fixes compiletest breaking when using quoted arguments like --cfg 'feature="my app"' in compile-flags or custom diff tools.

Uses proper shell argument parsing instead of naive whitespace splitting.

Closes #132599

@rustbot

rustbot commented Mar 27, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in src/tools/compiletest

cc @jieyouxu

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc labels Mar 27, 2026
@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 Mar 27, 2026
@rustbot

rustbot commented Mar 27, 2026

Copy link
Copy Markdown
Collaborator

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @jieyouxu, @oli-obk, @wesleywiser, bootstrap
  • @jieyouxu, @oli-obk, @wesleywiser, bootstrap expanded to 8 candidates
  • Random selection from Mark-Simulacrum, clubby789, jieyouxu, wesleywiser

@rust-log-analyzer

This comment has been minimized.

@JumpiiX JumpiiX force-pushed the fix-132599-compiletest-arg-splitting branch from 807fb0f to 7540640 Compare March 27, 2026 20:24
@rust-log-analyzer

This comment has been minimized.

@JumpiiX JumpiiX force-pushed the fix-132599-compiletest-arg-splitting branch from 7540640 to 08a1c22 Compare March 27, 2026 21:02
@rust-log-analyzer

This comment has been minimized.

@JumpiiX JumpiiX force-pushed the fix-132599-compiletest-arg-splitting branch from 08a1c22 to 756d483 Compare March 27, 2026 21:09
@rust-log-analyzer

This comment has been minimized.

  Only fix the diff command argument parsing to handle quoted paths.
  Keep compile-flags parsing unchanged to avoid breaking existing tests.
@JumpiiX JumpiiX force-pushed the fix-132599-compiletest-arg-splitting branch from 756d483 to 50ce1b4 Compare March 28, 2026 09:43
@JumpiiX

JumpiiX commented Mar 28, 2026

Copy link
Copy Markdown
Contributor Author

@wesleywiser I've narrowed this fix to only handle custom diff commands with quoted arguments (the original issue).

I initially tried using shlex for all argument parsing, but this breaks ~50-90 existing tests that use unquoted --cfg key=value syntax in their compile-flags directives.

Two options:

  1. Current approach: Only fix diff command parsing, leave compile-flags as-is for backward compatibility
  2. Alternative: Update all affected tests to use proper shell quoting (would touch 50+ test files)

Which approach would you prefer? Happy to go either way, just wanted to keep the PR focused initially.

Comment on lines 2837 to +2838
if let Some(diff_command) = self.config.diff_command.as_deref() {
let mut args = diff_command.split_whitespace();
let name = args.next().unwrap();
match Command::new(name).args(args).args([expected_path, actual_path]).output() {
Err(err) => {
self.fatal(&format!(
"failed to call custom diff command `{diff_command}`: {err}"
));
match shlex::split(diff_command) {

@jieyouxu jieyouxu Apr 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

View changes since the review

Remark: so actually, this case I care less about, properly splitting diff command stuff is "nice-to-have" for users on custom diff tools but this is not correctness-critical

@JumpiiX JumpiiX requested a review from jieyouxu April 20, 2026 09:20
@fmease

fmease commented Jun 8, 2026

Copy link
Copy Markdown
Member

I initially tried using shlex for all argument parsing, but this breaks ~50-90 existing tests that use unquoted --cfg key=value syntax in their compile-flags directives.

Two options:

  1. Current approach: Only fix diff command parsing, leave compile-flags as-is for backward compatibility
  2. Alternative: Update all affected tests to use proper shell quoting (would touch 50+ test files)

Which approach would you prefer? Happy to go either way, just wanted to keep the PR focused initially.

Personally speaking, I vote for option (2), shlex everywhere, update all tests and be done with it.

@fmease fmease assigned fmease and jieyouxu and unassigned wesleywiser Jun 8, 2026
@fmease fmease 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 Jun 8, 2026
@rust-bors

rust-bors Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #158212) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

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

Labels

A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

Fix argument splitting in compiletest

6 participants