Skip to content

Fix argument parsing in compiletest for quoted strings#154479

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

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

Conversation

@JumpiiX
Copy link
Copy Markdown
Contributor

@JumpiiX JumpiiX commented Mar 27, 2026 •
edited by rustbot
Loading

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
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 27, 2026

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
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 27, 2026

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.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@JumpiiX
Fix custom diff command parsing in compiletest
50ce1b4 
  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
Copy link
Copy Markdown
Contributor Author

JumpiiX commented Mar 28, 2026

@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.

@jieyouxu jieyouxu mentioned this pull request Apr 10, 2026
Closed
jieyouxu
jieyouxu reviewed Apr 10, 2026
View reviewed changes
Comment thread src/tools/compiletest/src/runtest.rs
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) {
Copy link
Copy Markdown
Member

@jieyouxu jieyouxu Apr 10, 2026 •
edited by rustbot
Loading

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jieyouxu jieyouxu Awaiting requested review from jieyouxu

At least 1 approving review is required to merge this pull request.

Assignees

@wesleywiser wesleywiser

Labels

A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc 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)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix argument splitting in compiletest

5 participants

@JumpiiX @rustbot @rust-log-analyzer @jieyouxu @wesleywiser