Retry command invocation with argfile #10546

weihanglo · 2022-04-08T12:06:42Z

What does this PR try to resolve?

The goal is to invoke rustc and rustdoc with @path arg file as a fallback.

Since the -Zcheck-cfg-features¹ has already been implemented, in the foreseeable future cargo will probably hit the “command line too big” more often on Windows. rustdoc and rustc both support @path argfile ²³, so we can leverage the feature for the fallback logic.

The idea behind this PR is that we put the retry logic in ProcessBuilder::exec* functions and reuse them as much as possible.

How should we test and review this PR?

Review it commit by commit is recommended.

Argfile fallback is hard to tested with integration tests, so I added some unit tests for cargo_util::ProcessBuilder and cargo::ops::fix::FixArgs.

If you do want to test the support of argfile manually, a new environment variable __CARGO_TEST_FORCE_ARGFILE is added for the sake of forcing cargo to use argfile for rustc invocations. For example, __CARGO_TEST_FORCE_ARGFILE cargo test --test testsuite -- fix:: is usually a good start to test this feature.

Additional information

Should we escape line feed when writing into argfile and unescape while reading? 82781929b8fffd65f363aab1549e1b8f6773920f implements that but I am not sure whether it is necessary.

- Add `ProcessBuilder::output` and ProcessBuilder::status`, which are unopinionated version of `exec_*` (won't error out when exitcode > 0) - Add `ProcessBuilder::retry_with_argfile` to enable trying with argfile when hitting the "command line too big" error.

rust-highfive · 2022-04-08T12:06:45Z

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

crates/cargo-util/src/process_builder.rs

epage · 2022-04-08T14:03:44Z

crates/cargo-util/src/process_builder.rs

+                let (mut cmd, _argfile) = self.build_command_with_argfile()?;
+                apply(&mut cmd);
+                cmd.spawn()


Aren't we closing / removing the tempfile before the process has completed, racing with the application that is accessing it?

And should we explicitly close the tempfile, identifying any errors from the clean up?

Fixed with 9155c00. It also removes the awkward build_and_spawn function as a side effect.

For detecting file cleanup error, in 7146111 we logs the errors, though I am not sure what's the elegant way to handle this.

crates/cargo-util/src/process_builder.rs

epage · 2022-04-08T14:11:25Z

src/cargo/util/rustc.rs

+        let mut cmd = ProcessBuilder::new(self.path.as_path()).wrapped(self.wrapper.as_ref());
+        cmd.retry_with_argfile(true);
+        cmd
    }

    /// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`.
    pub fn workspace_process(&self) -> ProcessBuilder {
-        ProcessBuilder::new(self.path.as_path())
+        let mut cmd = ProcessBuilder::new(self.path.as_path())
            .wrapped(self.workspace_wrapper.as_ref())
-            .wrapped(self.wrapper.as_ref())
+            .wrapped(self.wrapper.as_ref());
+        cmd.retry_with_argfile(true);
+        cmd
    }

    pub fn process_no_wrapper(&self) -> ProcessBuilder {
-        ProcessBuilder::new(&self.path)
+        let mut cmd = ProcessBuilder::new(&self.path);
+        cmd.retry_with_argfile(true);
+        cmd
    }


How comes retry_with_argfile isn't chained

I followed the existing pattern from both ProcessBuilder and std::process::Command. It is worth making it more chainable, but a dedicated PR should better IMO.

epage · 2022-04-08T14:13:55Z

src/cargo/ops/fix.rs

+        if let Some(argfile_path) = rustc.to_str().unwrap_or_default().strip_prefix("@") {
+            // Because cargo in fix-proxy-mode might hit the command line size limit,
+            // cargo fix need handle `@path` argfile for this special case.
+            if argv.next().is_some() {
+                bail!("argfile `@path` cannot be combined with other arguments");
+            }
+            let contents = fs::read_to_string(argfile_path)
+                .with_context(|| format!("failed to read argfile at `{argfile_path}`"))?;
+            let mut iter = contents.lines().map(OsString::from);
+            rustc = iter
+                .next()
+                .map(PathBuf::from)
+                .ok_or_else(|| anyhow::anyhow!("expected rustc as first argument"))?;
+            for arg in iter {
+                handle_arg(arg)?;
+            }
+        } else {
+            for arg in argv {
+                handle_arg(arg)?;
+            }
        }


Side note: While I'm assuming this is doing things like in other places, there is now the argfile crate

crates/cargo-util/src/process_builder.rs

src/cargo/ops/fix.rs

epage · 2022-04-08T14:20:43Z

crates/cargo-util/src/process_builder.rs

+                    "argument contains invalid UTF-8 characters",
+                )
+            })?;
+            writeln!(buf, "{}", ArgEscape::from(arg))?;


I guess the other question to escaping is what do rustc and similar tools support?

This is how rustc generates argfiles for ld. As I read it so far, they just follow the specification. rustc uses only newlines to determine so I guess escaping \r \n and \\ might be sufficient.

However, I found that rustc_driver just naively reads line by line from argfile, they won't do any unescaping. As long as rustc does nothing special and our primary use case is serving rustc, I decide to remove the escape part and error out if someone passes line feeds in, as you recommended earlier.

BTW there is a test case validating that behaviour, which unfortunately accept whitespaces in RUSTDOCFLAGS.

cargo in fix-proxy-mode might read `@path` argfile as what rustc does. Therefore, we should collect info from `ProcessBuilder::get_args()`.

weihanglo · 2022-04-10T15:07:48Z

Addressed issues in review comments. Ready to re-review :)

Drop escape/unescape (commit 82781929b8fffd65f363aab1549e1b8f6773920f) because rustc doesn't understand how to unescape escaped characters.
Add __CARGO_TEST_FORCE_ARGFILE env var to force using argfile for rustc invocation in debug mode. 5e8827f
Add a CI test to check argfile operability by force cargo to use @path argfile for rustc. Only test cargo fix because it is the only place reading from argfile. 275b0c6

crates/cargo-util/src/process_builder.rs

epage

Looks good!

epage · 2022-04-11T14:57:40Z

@bors r+

bors · 2022-04-11T14:57:42Z

📌 Commit 275b0c6 has been approved by epage

bors · 2022-04-11T14:57:49Z

⌛ Testing commit 275b0c6 with merge 1073915...

bors · 2022-04-11T16:16:50Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 1073915 to master...

Update cargo 11 commits in e2e2dddebe66dfc1403a312653557e332445308b..dba5baf4345858c591517b24801902a062c399f8 2022-04-05 17:04:53 +0000 to 2022-04-13 21:58:27 +0000 - Part 6 of RFC2906 - Switch the inheritance source from `workspace` to… (rust-lang/cargo#10564) - Part 5 of RFC2906 - Add support for inheriting `rust-version` (rust-lang/cargo#10563) - Add support for rustc --check-cfg well known names and values (rust-lang/cargo#10486) - Reserve filename `Cargo.toml.orig` in `cargo package` (rust-lang/cargo#10551) - Retry command invocation with argfile (rust-lang/cargo#10546) - Add a progress indicator for `cargo clean` (rust-lang/cargo#10236) - Ensure host units don't depend on Docscrape units, fixes rust-lang/cargo#10545 (rust-lang/cargo#10549) - Fix docs: Bindeps env vars are passed to build script at runtime (rust-lang/cargo#10550) - Part 4 of RFC2906 - Add support for inheriting `readme` (rust-lang/cargo#10548) - Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` (rust-lang/cargo#10538) - Bump to 0.63.0, update changelog (rust-lang/cargo#10544)

Bump cargo-util version. #10546 made a semver-incompatible change to the API of `ProcessBuilder::get_args`. Unfortunately we did not catch that until it was published. This bumps the version of cargo-util to 0.2.1 to accommodate that change. Stable will get version 0.2.0 so that the changes on beta can be released as 0.2.1 in their own time. cc #10803

weihanglo added 8 commits April 8, 2022 13:38

Separate command wrappers from command arguments

c9d443a

Bump cargo-util to 0.1.3

4f6a2ec

Avoid calling build_command in fn cached_output

734569c

Avoid calling build_command in cargo fix

c832a7a

Retry with argfile for rustc/rustdoc

832274d

Retry with argfile for cargo in rustc fix-proxy-mode

fc05277

Use CargoResult pervasively

beaaefd

rust-highfive assigned ehuss Apr 8, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 8, 2022

weihanglo force-pushed the issue-10381-rustc-argfile branch from a42104e to 8278192 Compare April 8, 2022 13:07

epage reviewed Apr 8, 2022

View reviewed changes

weihanglo added 8 commits April 10, 2022 21:46

State that arg in argfile must not contain newlines

877c0ad

Avoid collect args from `env::args in proxy mode

8e0e68e

cargo in fix-proxy-mode might read `@path` argfile as what rustc does. Therefore, we should collect info from `ProcessBuilder::get_args()`.

Ensure that temporary argfile lives longer them command execution

9155c00

Ensure non-UTF8 compatibility of argfile arg

3f749f6

Debug assertion for rustc argfile invocations

5e8827f

Close tempfile explicitly after the command to exist

7146111

test: make tests compatible when force using argfile

d813739

CI: force using argfile to test againg cargo fix

275b0c6

weihanglo force-pushed the issue-10381-rustc-argfile branch from 8278192 to 275b0c6 Compare April 10, 2022 15:00

epage reviewed Apr 11, 2022

View reviewed changes

crates/cargo-util/src/process_builder.rs Show resolved Hide resolved

epage approved these changes Apr 11, 2022

View reviewed changes

bors 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 Apr 11, 2022

bors merged commit 1073915 into rust-lang:master Apr 11, 2022

weihanglo deleted the issue-10381-rustc-argfile branch April 11, 2022 23:07

weihanglo added the relnotes Release-note worthy label Apr 12, 2022

ehuss mentioned this pull request Apr 14, 2022

Update cargo rust-lang/rust#96031

Merged

This was referenced Apr 20, 2022

Accept args via stdin/file (WAS: Too many dependencies leads to excessively long command that fails) #1241

Closed

Feature request: Use response files for rustc build/doc. #7759

Closed

ehuss added this to the 1.62.0 milestone Apr 22, 2022

ehuss mentioned this pull request Jun 30, 2022

Bump cargo-util version. #10804

Merged

Urgau mentioned this pull request Jul 21, 2022

Remove implicit names and values from --cfg in --check-cfg rust-lang/rust#99519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry command invocation with argfile #10546

Retry command invocation with argfile #10546

weihanglo commented Apr 8, 2022 •

edited

Loading

rust-highfive commented Apr 8, 2022

epage Apr 8, 2022

weihanglo Apr 10, 2022 •

edited

Loading

epage Apr 8, 2022

weihanglo Apr 10, 2022

epage Apr 8, 2022

epage Apr 8, 2022

weihanglo Apr 10, 2022

weihanglo commented Apr 10, 2022 •

edited

Loading

epage left a comment

epage commented Apr 11, 2022

bors commented Apr 11, 2022

bors commented Apr 11, 2022

bors commented Apr 11, 2022

Retry command invocation with argfile #10546

Retry command invocation with argfile #10546

Conversation

weihanglo commented Apr 8, 2022 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Footnotes

rust-highfive commented Apr 8, 2022

epage Apr 8, 2022

Choose a reason for hiding this comment

weihanglo Apr 10, 2022 • edited Loading

Choose a reason for hiding this comment

epage Apr 8, 2022

Choose a reason for hiding this comment

weihanglo Apr 10, 2022

Choose a reason for hiding this comment

epage Apr 8, 2022

Choose a reason for hiding this comment

epage Apr 8, 2022

Choose a reason for hiding this comment

weihanglo Apr 10, 2022

Choose a reason for hiding this comment

weihanglo commented Apr 10, 2022 • edited Loading

epage left a comment

Choose a reason for hiding this comment

epage commented Apr 11, 2022

bors commented Apr 11, 2022

bors commented Apr 11, 2022

bors commented Apr 11, 2022

weihanglo commented Apr 8, 2022 •

edited

Loading

weihanglo Apr 10, 2022 •

edited

Loading

weihanglo commented Apr 10, 2022 •

edited

Loading