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

Support response files generated by ninja with @ninja:path syntax #684

Closed
1 of 3 tasks
djkoloski opened this issue Oct 26, 2023 · 6 comments · Fixed by rust-lang/rust#118680
Closed
1 of 3 tasks
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@djkoloski
Copy link

djkoloski commented Oct 26, 2023

Background

A response file (also called an argument file) is a special command-line argument that instructs a program to read additional arguments from a file. Response files are prefixed with @, and so invocations usually take the form rustc @args.txt. Response files exist to work around maximum line length limitations on various operating systems.

Motivation

This proposal is intended to provide a solution for this issue and allow rustc to integrate with ninja more easily. It also sets a convention for supporting response files with different formats in rustc.

Today in rustc, response files are parsed as newline-delimited files where each argument exists on a single line. Any spaces on a line are treated as part of that argument, and surrounding quotes are passed through verbatim. This makes it difficult to integrate rustc with ninja, which generates response files with space-delimited (possibly-)quoted arguments. This functionality is built into ninja itself with the rspfile option. Rustc's response file format is incompatible with ninja's generated format, and the maintainer of ninja is unlikely to make a special case for us.

Proposal

We propose changing the behavior of rustc and cargo to accept response file arguments of the form@ninja:path. Those arguments are treated as response files generated by ninja which follow its conventions for argument delimitation, quoting, and escaping.

Mentors or Reviewers

@tmandry is on the Fuchsia team and holds some stake in this MCP.

@Nilstrieb has expressed interest in reviewing an MCP for this behavior change.

@jsgf added response file handling to rustc in #63175 roughly four years ago. Argument quoting and character escaping was discussed but not implemented.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@djkoloski djkoloski added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Oct 26, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Oct 26, 2023
@djkoloski
Copy link
Author

djkoloski commented Oct 26, 2023

cc @fangism originally raised this in rust-lang/rust#116068.
cc @P1n3appl3 who provided an implementation in rust-lang/rust#116954.

@jsgf
Copy link

jsgf commented Oct 27, 2023

(moved to Zulip)

@djkoloski djkoloski changed the title Use shell parsing for response files Evaluate quoting in response files Oct 27, 2023
@djkoloski djkoloski changed the title Evaluate quoting in response files Allow and evaluate quoting in response files Oct 27, 2023
@djkoloski djkoloski changed the title Allow and evaluate quoting in response files Support response files generated by ninja with @ninja:path syntax Nov 2, 2023
@tmandry
Copy link
Member

tmandry commented Nov 7, 2023

This MCP has had several revisions in response to feedback, and at this point only bikeshed details are left.

@rust-lang/compiler would someone please kick off an FCP?

@tmandry
Copy link
Member

tmandry commented Nov 8, 2023

Actually, after some discussion with the compiler team it sounds like the right process is to start with an unstable-gated flag, in which case this does not need a full team sign-off and a second is all that is necessary.

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Nov 8, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 9, 2023
@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Dec 18, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 28, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 9, 2024
Rollup merge of rust-lang#118680 - djkoloski:shell_argfiles, r=compiler-errors

Add support for shell argfiles

Closes rust-lang/compiler-team#684
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants