Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use shell parsing for response files #116954

Closed
wants to merge 1 commit into from
Closed

Conversation

P1n3appl3
Copy link
Contributor

It turns out that #116610 is inadequate, we actually need rustc to perform shell parsing on the lines in response files. This will require adding a dep to the compiler: https://crates.io/crates/shlex

Both clang and gcc do shlex parsing, so it seems reasonable to make rustc do the same. However this would technically be a breaking change if some build system was relying on being able to pass unquoted arguments with spaces in them in a response file and having rustc interpret them as a single arg. To me that scenario seems pretty unlikely, but I'd understand if people want to be more cautious.

How should we proceed? We could do any of:

  • Add some tests and land as-is assuming that no one relies on the current lack of shell parsing
  • Perform a crater run with this change and then do ^ with more confidence. Unfortunately crater doesn't do a good job of detecting breakages due to this change because cargo doesn't use response files. We'd expect ninja/make/bazel/buck/etc. build systems to be the ones affected by this. We could do a survey of build tools with rust support but I'd guess that because all of those tools interface with c compilers that do perform shlex parsing they'd be quoting their args correctly in the first place.
  • Write an MCP so that the compiler maintainers can sign off on the breaking change before we merge it.
  • Add a new unstable flag such as -Z response_file_shlex that enables this new behavior for all response files on the command line
    • If it's necessary to be able to mix both types of response file handling, it could work like -Bstatic/-Bdynamic where they toggle it for any response files after the flag
    • We could also have the flag apply to a specific file

@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 19, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2023

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.

Third-party dependency whitelist may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

@Noratrieb
Copy link
Member

I think crater would be useless. Given that clang and GCC do this, it seems unlikely that any build system would rely on it not being done. At least to me this seems reasonable, but you should create an MCP it.
Doing a quick survey of existing users of response files would be great if possible, just to make sure. Maybe git blame around the relevant code will reveal some?

@apiraino
Copy link
Contributor

Switching to S-waiting-on-author to take action after compiler-team#684 finishes (or advance a new MCP?)

@rustbot author

@rustbot rustbot 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 Dec 21, 2023
@bors
Copy link
Contributor

bors commented Jan 9, 2024

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

@Dylan-DPC
Copy link
Member

@P1n3appl3 any updates on this? thanks

@P1n3appl3
Copy link
Contributor Author

#118680 superseded this PR and was landed

@P1n3appl3 P1n3appl3 closed this Feb 23, 2024
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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.

7 participants