From c9d443a063d75af96abbcd7c42f065ffc33a189b Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Fri, 8 Apr 2022 17:00:00 +0800 Subject: [PATCH 01/16] Separate command wrappers from command arguments --- crates/cargo-util/src/process_builder.rs | 52 +++++++++++++++--------- src/cargo/core/compiler/build_plan.rs | 2 +- src/cargo/core/compiler/mod.rs | 2 +- src/cargo/util/rustc.rs | 2 +- 4 files changed, 35 insertions(+), 23 deletions(-) diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index 9c43d3b2f03..730da0c3b0d 100644 --- a/crates/cargo-util/src/process_builder.rs +++ b/crates/cargo-util/src/process_builder.rs @@ -22,6 +22,9 @@ pub struct ProcessBuilder { env: BTreeMap<String, Option<OsString>>, /// The directory to run the program from. cwd: Option<OsString>, + /// A list of wrappers that wrap the original program when calling + /// [`ProcessBuilder::wrapped`]. The last one is the outermost one. + wrappers: Vec<OsString>, /// The `make` jobserver. See the [jobserver crate] for /// more information. /// @@ -48,9 +51,9 @@ impl fmt::Display for ProcessBuilder { } } - write!(f, "{}", self.program.to_string_lossy())?; + write!(f, "{}", self.get_program().to_string_lossy())?; - for arg in &self.args { + for arg in self.get_args() { write!(f, " {}", escape(arg.to_string_lossy()))?; } @@ -66,6 +69,7 @@ impl ProcessBuilder { args: Vec::new(), cwd: None, env: BTreeMap::new(), + wrappers: Vec::new(), jobserver: None, display_env_vars: false, } @@ -92,6 +96,13 @@ impl ProcessBuilder { /// (chainable) Replaces the args list with the given `args`. pub fn args_replace<T: AsRef<OsStr>>(&mut self, args: &[T]) -> &mut ProcessBuilder { + if let Some(program) = self.wrappers.pop() { + // User intend to replace all args, so we + // - use the outermost wrapper as the main program, and + // - cleanup other inner wrappers. + self.program = program; + self.wrappers = Vec::new(); + } self.args = args.iter().map(|t| t.as_ref().to_os_string()).collect(); self } @@ -117,12 +128,17 @@ impl ProcessBuilder { /// Gets the executable name. pub fn get_program(&self) -> &OsString { - &self.program + self.wrappers.last().unwrap_or(&self.program) } /// Gets the program arguments. - pub fn get_args(&self) -> &[OsString] { - &self.args + pub fn get_args(&self) -> impl Iterator<Item = &OsString> { + self.wrappers + .iter() + .rev() + .chain(once(&self.program)) + .chain(self.args.iter()) + .skip(1) // Skip the main `program } /// Gets the current working directory for the process. @@ -327,7 +343,12 @@ impl ProcessBuilder { /// Converts `ProcessBuilder` into a `std::process::Command`, and handles the jobserver, if /// present. pub fn build_command(&self) -> Command { - let mut command = Command::new(&self.program); + let mut command = { + let mut iter = self.wrappers.iter().rev().chain(once(&self.program)); + let mut cmd = Command::new(iter.next().expect("at least one `program` exists")); + cmd.args(iter); + cmd + }; if let Some(cwd) = self.get_cwd() { command.current_dir(cwd); } @@ -363,21 +384,12 @@ impl ProcessBuilder { /// let cmd = cmd.wrapped(Some("sccache")); /// ``` pub fn wrapped(mut self, wrapper: Option<impl AsRef<OsStr>>) -> Self { - let wrapper = if let Some(wrapper) = wrapper.as_ref() { - wrapper.as_ref() - } else { - return self; - }; - - if wrapper.is_empty() { - return self; + if let Some(wrapper) = wrapper.as_ref() { + let wrapper = wrapper.as_ref(); + if !wrapper.is_empty() { + self.wrappers.push(wrapper.to_os_string()); + } } - - let args = once(self.program).chain(self.args.into_iter()).collect(); - - self.program = wrapper.to_os_string(); - self.args = args; - self } } diff --git a/src/cargo/core/compiler/build_plan.rs b/src/cargo/core/compiler/build_plan.rs index 6ffe24a27e5..a823aa95232 100644 --- a/src/cargo/core/compiler/build_plan.rs +++ b/src/cargo/core/compiler/build_plan.rs @@ -78,7 +78,7 @@ impl Invocation { .ok_or_else(|| anyhow::format_err!("unicode program string required"))? .to_string(); self.cwd = Some(cmd.get_cwd().unwrap().to_path_buf()); - for arg in cmd.get_args().iter() { + for arg in cmd.get_args() { self.args.push( arg.to_str() .ok_or_else(|| anyhow::format_err!("unicode argument string required"))? diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 5abd06a75bd..b9982f41c0e 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -754,7 +754,7 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> { // The --crate-version flag could have already been passed in RUSTDOCFLAGS // or as an extra compiler argument for rustdoc fn crate_version_flag_already_present(rustdoc: &ProcessBuilder) -> bool { - rustdoc.get_args().iter().any(|flag| { + rustdoc.get_args().any(|flag| { flag.to_str() .map_or(false, |flag| flag.starts_with(RUSTDOC_CRATE_VERSION_FLAG)) }) diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 33f96c15532..25ecf336cc5 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -351,7 +351,7 @@ fn rustc_fingerprint( fn process_fingerprint(cmd: &ProcessBuilder, extra_fingerprint: u64) -> u64 { let mut hasher = StableHasher::new(); extra_fingerprint.hash(&mut hasher); - cmd.get_args().hash(&mut hasher); + cmd.get_args().for_each(|arg| arg.hash(&mut hasher)); let mut env = cmd.get_envs().iter().collect::<Vec<_>>(); env.sort_unstable(); env.hash(&mut hasher); From b788e5224646626eb52d9cede1852070791ab4ff Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Fri, 8 Apr 2022 17:00:00 +0800 Subject: [PATCH 02/16] Retry with argfile if hitting "command line too big" error - 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. --- crates/cargo-util/src/process_builder.rs | 186 ++++++++++++++++++++--- 1 file changed, 161 insertions(+), 25 deletions(-) diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index 730da0c3b0d..f792712b03d 100644 --- a/crates/cargo-util/src/process_builder.rs +++ b/crates/cargo-util/src/process_builder.rs @@ -1,15 +1,19 @@ use crate::process_error::ProcessError; use crate::read2; + use anyhow::{bail, Context, Result}; use jobserver::Client; use shell_escape::escape; +use tempfile::NamedTempFile; + use std::collections::BTreeMap; use std::env; use std::ffi::{OsStr, OsString}; use std::fmt; +use std::io; use std::iter::once; use std::path::Path; -use std::process::{Command, Output, Stdio}; +use std::process::{Child, Command, ExitStatus, Output, Stdio}; /// A builder object for an external process, similar to [`std::process::Command`]. #[derive(Clone, Debug)] @@ -32,6 +36,8 @@ pub struct ProcessBuilder { jobserver: Option<Client>, /// `true` to include environment variable in display. display_env_vars: bool, + /// `true` to retry with an argfile if hitting "command line too big" error. + retry_with_argfile: bool, } impl fmt::Display for ProcessBuilder { @@ -72,6 +78,7 @@ impl ProcessBuilder { wrappers: Vec::new(), jobserver: None, display_env_vars: false, + retry_with_argfile: false, } } @@ -177,13 +184,28 @@ impl ProcessBuilder { self } + /// Enables retrying with an argfile if hitting "command line too big" error + pub fn retry_with_argfile(&mut self, enabled: bool) -> &mut Self { + self.retry_with_argfile = enabled; + self + } + + fn should_retry_with_argfile(&self, err: &io::Error) -> bool { + self.retry_with_argfile && imp::command_line_too_big(err) + } + + /// Like [`Command::status`] but with a better error message. + pub fn status(&self) -> Result<ExitStatus> { + self.build_and_spawn(|_| {}) + .and_then(|mut child| child.wait()) + .with_context(|| { + ProcessError::new(&format!("could not execute process {self}"), None, None) + }) + } + /// Runs the process, waiting for completion, and mapping non-success exit codes to an error. pub fn exec(&self) -> Result<()> { - let mut command = self.build_command(); - let exit = command.status().with_context(|| { - ProcessError::new(&format!("could not execute process {}", self), None, None) - })?; - + let exit = self.status()?; if exit.success() { Ok(()) } else { @@ -215,14 +237,22 @@ impl ProcessBuilder { imp::exec_replace(self) } + /// Like [`Command::output`] but with a better error message. + pub fn output(&self) -> Result<Output> { + self.build_and_spawn(|cmd| { + cmd.stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(Stdio::null()); + }) + .and_then(|child| child.wait_with_output()) + .with_context(|| { + ProcessError::new(&format!("could not execute process {self}"), None, None) + }) + } + /// Executes the process, returning the stdio output, or an error if non-zero exit status. pub fn exec_with_output(&self) -> Result<Output> { - let mut command = self.build_command(); - - let output = command.output().with_context(|| { - ProcessError::new(&format!("could not execute process {}", self), None, None) - })?; - + let output = self.output()?; if output.status.success() { Ok(output) } else { @@ -253,16 +283,15 @@ impl ProcessBuilder { let mut stdout = Vec::new(); let mut stderr = Vec::new(); - let mut cmd = self.build_command(); - cmd.stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .stdin(Stdio::null()); - let mut callback_error = None; let mut stdout_pos = 0; let mut stderr_pos = 0; let status = (|| { - let mut child = cmd.spawn()?; + let mut child = self.build_and_spawn(|cmd| { + cmd.stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(Stdio::null()); + })?; let out = child.stdout.take().unwrap(); let err = child.stderr.take().unwrap(); read2(out, err, &mut |is_out, data, eof| { @@ -340,9 +369,68 @@ impl ProcessBuilder { Ok(output) } - /// Converts `ProcessBuilder` into a `std::process::Command`, and handles the jobserver, if - /// present. - pub fn build_command(&self) -> Command { + /// Builds a command from `ProcessBuilder` and spawn it. + /// + /// There is a risk when spawning a process, it might hit "command line + /// too big" OS error. To handle those kind of OS errors, this method try + /// to reinvoke the command with a `@<path>` argfile that contains all the + /// arguments. + /// + /// * `apply`: Modify the command before invoking. Useful for updating [`Stdio`]. + fn build_and_spawn(&self, apply: impl Fn(&mut Command)) -> io::Result<Child> { + let mut cmd = self.build_command(); + apply(&mut cmd); + + match cmd.spawn() { + Err(ref e) if self.should_retry_with_argfile(e) => { + let (mut cmd, _argfile) = self.build_command_with_argfile()?; + apply(&mut cmd); + cmd.spawn() + } + res => res, + } + } + + /// Builds the command with an `@<path>` argfile that contains all the + /// arguments. This is primarily served for rustc/rustdoc command family. + /// + /// Ref: + /// + /// - https://doc.rust-lang.org/rustdoc/command-line-arguments.html#path-load-command-line-flags-from-a-path + /// - https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path> + fn build_command_with_argfile(&self) -> io::Result<(Command, NamedTempFile)> { + use std::io::Write as _; + + let mut tmp = tempfile::Builder::new() + .prefix("cargo-argfile.") + .tempfile()?; + + let path = tmp.path().display(); + let mut cmd = self.build_command_without_args(); + cmd.arg(format!("@{path}")); + log::debug!("created argfile at {path} for `{self}`"); + + let cap = self.get_args().map(|arg| arg.len() + 1).sum::<usize>(); + let mut buf = String::with_capacity(cap); + for arg in &self.args { + let arg = arg + .to_str() + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::Other, + "argument contains invalid UTF-8 characters", + ) + })?; + // TODO: Shall we escape line feed? + buf.push_str(arg); + buf.push('\n'); + } + tmp.write_all(buf.as_bytes())?; + Ok((cmd, tmp)) + } + + /// Builds a command from `ProcessBuilder` for everythings but not `args`. + fn build_command_without_args(&self) -> Command { let mut command = { let mut iter = self.wrappers.iter().rev().chain(once(&self.program)); let mut cmd = Command::new(iter.next().expect("at least one `program` exists")); @@ -352,9 +440,6 @@ impl ProcessBuilder { if let Some(cwd) = self.get_cwd() { command.current_dir(cwd); } - for arg in &self.args { - command.arg(arg); - } for (k, v) in &self.env { match *v { Some(ref v) => { @@ -371,6 +456,19 @@ impl ProcessBuilder { command } + /// Converts `ProcessBuilder` into a `std::process::Command`, and handles + /// the jobserver, if present. + /// + /// Note that this method doesn't take argfile fallback into account. The + /// caller should handle it by themselves. + pub fn build_command(&self) -> Command { + let mut command = self.build_command_without_args(); + for arg in &self.args { + command.arg(arg); + } + command + } + /// Wraps an existing command with the provided wrapper, if it is present and valid. /// /// # Examples @@ -398,23 +496,35 @@ impl ProcessBuilder { mod imp { use super::{ProcessBuilder, ProcessError}; use anyhow::Result; + use std::io; use std::os::unix::process::CommandExt; pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> { let mut command = process_builder.build_command(); - let error = command.exec(); + + let mut error = command.exec(); + if process_builder.should_retry_with_argfile(&error) { + let (mut command, _argfile) = process_builder.build_command_with_argfile()?; + error = command.exec() + } + Err(anyhow::Error::from(error).context(ProcessError::new( &format!("could not execute process {}", process_builder), None, None, ))) } + + pub fn command_line_too_big(err: &io::Error) -> bool { + err.raw_os_error() == Some(libc::E2BIG) + } } #[cfg(windows)] mod imp { use super::{ProcessBuilder, ProcessError}; use anyhow::Result; + use std::io; use winapi::shared::minwindef::{BOOL, DWORD, FALSE, TRUE}; use winapi::um::consoleapi::SetConsoleCtrlHandler; @@ -433,4 +543,30 @@ mod imp { // Just execute the process as normal. process_builder.exec() } + + pub fn command_line_too_big(err: &io::Error) -> bool { + use winapi::shared::winerror::ERROR_FILENAME_EXCED_RANGE; + err.raw_os_error() == Some(ERROR_FILENAME_EXCED_RANGE as i32) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + #[test] + fn test_argfile() { + let mut cmd = ProcessBuilder::new("echo"); + cmd.args(["foo", "bar"].as_slice()); + let (cmd, argfile) = cmd.build_command_with_argfile().unwrap(); + + assert_eq!(cmd.get_program(), "echo"); + let cmd_args: Vec<_> = cmd.get_args().map(|s| s.to_str().unwrap()).collect(); + assert_eq!(cmd_args.len(), 1); + assert!(cmd_args[0].starts_with("@")); + assert!(cmd_args[0].contains("cargo-argfile.")); + + let buf = fs::read_to_string(argfile.path()).unwrap(); + assert_eq!(buf, "foo\nbar\n"); + } } From 4f6a2ec28b22d5197c91041178699982ff4a7801 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Fri, 8 Apr 2022 17:00:00 +0800 Subject: [PATCH 03/16] Bump cargo-util to 0.1.3 --- Cargo.toml | 2 +- crates/cargo-util/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f4e157cc64f..1d8a1b00ba7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,7 @@ path = "src/cargo/lib.rs" atty = "0.2" bytesize = "1.0" cargo-platform = { path = "crates/cargo-platform", version = "0.1.2" } -cargo-util = { path = "crates/cargo-util", version = "0.1.2" } +cargo-util = { path = "crates/cargo-util", version = "0.1.3" } crates-io = { path = "crates/crates-io", version = "0.34.0" } crossbeam-utils = "0.8" curl = { version = "0.4.41", features = ["http2"] } diff --git a/crates/cargo-util/Cargo.toml b/crates/cargo-util/Cargo.toml index 70c286a007f..86afbd0eeec 100644 --- a/crates/cargo-util/Cargo.toml +++ b/crates/cargo-util/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cargo-util" -version = "0.1.2" +version = "0.1.3" edition = "2021" license = "MIT OR Apache-2.0" homepage = "https://github.com/rust-lang/cargo" From 734569ce25153874690dd9c50cf6d7fdb0381697 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Fri, 8 Apr 2022 17:00:00 +0800 Subject: [PATCH 04/16] Avoid calling `build_command` in `fn cached_output` --- src/cargo/util/rustc.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 25ecf336cc5..5286180a457 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -231,10 +231,7 @@ impl Cache { } else { debug!("rustc info cache miss"); debug!("running {}", cmd); - let output = cmd - .build_command() - .output() - .with_context(|| format!("could not execute process {} (never executed)", cmd))?; + let output = cmd.output()?; let stdout = String::from_utf8(output.stdout) .map_err(|e| anyhow::anyhow!("{}: {:?}", e, e.as_bytes())) .with_context(|| format!("`{}` didn't return utf8 output", cmd))?; From c832a7a9a48137f7d9b1effb572222fc1876a7d3 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Fri, 8 Apr 2022 17:00:00 +0800 Subject: [PATCH 05/16] Avoid calling `build_command` in `cargo fix` --- src/cargo/ops/fix.rs | 45 +++++++++++++++++--------------------------- 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 75b2d011e4f..8c970ef171d 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -42,7 +42,7 @@ use std::collections::{BTreeSet, HashMap, HashSet}; use std::env; use std::ffi::OsString; use std::path::{Path, PathBuf}; -use std::process::{self, Command, ExitStatus}; +use std::process::{self, ExitStatus}; use std::str; use anyhow::{bail, Context, Error}; @@ -353,9 +353,15 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> { .ok(); let mut rustc = ProcessBuilder::new(&args.rustc).wrapped(workspace_rustc.as_ref()); rustc.env_remove(FIX_ENV); + args.apply(&mut rustc); trace!("start rustfixing {:?}", args.file); - let fixes = rustfix_crate(&lock_addr, &rustc, &args.file, &args, config)?; + let json_error_rustc = { + let mut cmd = rustc.clone(); + cmd.arg("--error-format=json"); + cmd + }; + let fixes = rustfix_crate(&lock_addr, &json_error_rustc, &args.file, &args, config)?; // Ok now we have our final goal of testing out the changes that we applied. // If these changes went awry and actually started to cause the crate to @@ -366,11 +372,8 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> { // new rustc, and otherwise we capture the output to hide it in the scenario // that we have to back it all out. if !fixes.files.is_empty() { - let mut cmd = rustc.build_command(); - args.apply(&mut cmd); - cmd.arg("--error-format=json"); - debug!("calling rustc for final verification: {:?}", cmd); - let output = cmd.output().context("failed to spawn rustc")?; + debug!("calling rustc for final verification: {json_error_rustc}"); + let output = json_error_rustc.output()?; if output.status.success() { for (path, file) in fixes.files.iter() { @@ -407,15 +410,13 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> { // - If the fix failed, show the original warnings and suggestions. // - If `--broken-code`, show the error messages. // - If the fix succeeded, show any remaining warnings. - let mut cmd = rustc.build_command(); - args.apply(&mut cmd); for arg in args.format_args { // Add any json/error format arguments that Cargo wants. This allows // things like colored output to work correctly. - cmd.arg(arg); + rustc.arg(arg); } - debug!("calling rustc to display remaining diagnostics: {:?}", cmd); - exit_with(cmd.status().context("failed to spawn rustc")?); + debug!("calling rustc to display remaining diagnostics: {rustc}"); + exit_with(rustc.status()?); } #[derive(Default)] @@ -502,7 +503,7 @@ fn rustfix_crate( // We'll generate new errors below. file.errors_applying_fixes.clear(); } - rustfix_and_fix(&mut fixes, rustc, filename, args, config)?; + rustfix_and_fix(&mut fixes, rustc, filename, config)?; let mut progress_yet_to_be_made = false; for (path, file) in fixes.files.iter_mut() { if file.errors_applying_fixes.is_empty() { @@ -543,26 +544,14 @@ fn rustfix_and_fix( fixes: &mut FixedCrate, rustc: &ProcessBuilder, filename: &Path, - args: &FixArgs, config: &Config, ) -> Result<(), Error> { // If not empty, filter by these lints. // TODO: implement a way to specify this. let only = HashSet::new(); - let mut cmd = rustc.build_command(); - cmd.arg("--error-format=json"); - args.apply(&mut cmd); - debug!( - "calling rustc to collect suggestions and validate previous fixes: {:?}", - cmd - ); - let output = cmd.output().with_context(|| { - format!( - "failed to execute `{}`", - rustc.get_program().to_string_lossy() - ) - })?; + debug!("calling rustc to collect suggestions and validate previous fixes: {rustc}"); + let output = rustc.output()?; // If rustc didn't succeed for whatever reasons then we're very likely to be // looking at otherwise broken code. Let's not make things accidentally @@ -834,7 +823,7 @@ impl FixArgs { }) } - fn apply(&self, cmd: &mut Command) { + fn apply(&self, cmd: &mut ProcessBuilder) { cmd.arg(&self.file); cmd.args(&self.other); if self.prepare_for_edition.is_some() { From 832274d3a21cf7dd6e6d87911c0012e499dbd193 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Fri, 8 Apr 2022 17:00:00 +0800 Subject: [PATCH 06/16] Retry with argfile for rustc/rustdoc --- src/cargo/core/compiler/compilation.rs | 1 + src/cargo/util/rustc.rs | 14 ++++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 38996222486..e8fc36d250f 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -194,6 +194,7 @@ impl<'cfg> Compilation<'cfg> { let rustdoc = ProcessBuilder::new(&*self.config.rustdoc()?); let cmd = fill_rustc_tool_env(rustdoc, unit); let mut cmd = self.fill_env(cmd, &unit.pkg, script_meta, unit.kind, true)?; + cmd.retry_with_argfile(true); unit.target.edition().cmd_edition_arg(&mut cmd); for crate_type in unit.target.rustc_crate_types() { diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 5286180a457..6073ffe0b60 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -93,18 +93,24 @@ impl Rustc { /// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`. pub fn process(&self) -> ProcessBuilder { - ProcessBuilder::new(self.path.as_path()).wrapped(self.wrapper.as_ref()) + 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 } /// Gets the output for the given command. From fc052776fc6d84519c11e259e1a4a3a474846ba4 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Fri, 8 Apr 2022 17:00:00 +0800 Subject: [PATCH 07/16] Retry with argfile for cargo in rustc fix-proxy-mode --- src/cargo/ops/fix.rs | 92 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 84 insertions(+), 8 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 8c970ef171d..67e742360f9 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -39,11 +39,10 @@ //! show them to the user. use std::collections::{BTreeSet, HashMap, HashSet}; -use std::env; use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::process::{self, ExitStatus}; -use std::str; +use std::{env, fs, str}; use anyhow::{bail, Context, Error}; use cargo_util::{exit_status_to_string, is_simple_exit_code, paths, ProcessBuilder}; @@ -122,6 +121,9 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions) -> CargoResult<()> { let rustc = ws.config().load_global_rustc(Some(ws))?; wrapper.arg(&rustc.path); + // This is calling rustc in cargo fix-proxy-mode, so it also need to retry. + // The argfile handling are located at `FixArgs::from_args`. + wrapper.retry_with_argfile(true); // primary crates are compiled using a cargo subprocess to do extra work of applying fixes and // repeating build until there are no more changes to be applied @@ -352,6 +354,7 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> { .map(PathBuf::from) .ok(); let mut rustc = ProcessBuilder::new(&args.rustc).wrapped(workspace_rustc.as_ref()); + rustc.retry_with_argfile(true); rustc.env_remove(FIX_ENV); args.apply(&mut rustc); @@ -774,35 +777,65 @@ struct FixArgs { impl FixArgs { fn get() -> Result<FixArgs, Error> { - let rustc = env::args_os() + Self::from_args(env::args_os()) + } + + // This is a separate function so that we can use it in tests. + fn from_args(argv: impl IntoIterator<Item = OsString>) -> Result<Self, Error> { + let mut argv = argv.into_iter(); + let mut rustc = argv .nth(1) .map(PathBuf::from) - .ok_or_else(|| anyhow::anyhow!("expected rustc as first argument"))?; + .ok_or_else(|| anyhow::anyhow!("expected rustc or `@path` as first argument"))?; let mut file = None; let mut enabled_edition = None; let mut other = Vec::new(); let mut format_args = Vec::new(); - for arg in env::args_os().skip(2) { + let mut handle_arg = |arg: OsString| -> Result<(), Error> { let path = PathBuf::from(arg); if path.extension().and_then(|s| s.to_str()) == Some("rs") && path.exists() { file = Some(path); - continue; + return Ok(()); } if let Some(s) = path.to_str() { if let Some(edition) = s.strip_prefix("--edition=") { enabled_edition = Some(edition.parse()?); - continue; + return Ok(()); } if s.starts_with("--error-format=") || s.starts_with("--json=") { // Cargo may add error-format in some cases, but `cargo // fix` wants to add its own. format_args.push(s.to_string()); - continue; + return Ok(()); } } other.push(path.into()); + Ok(()) + }; + + 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)?; + } } + let file = file.ok_or_else(|| anyhow::anyhow!("could not find .rs file in rustc args"))?; let idioms = env::var(IDIOMS_ENV).is_ok(); @@ -914,3 +947,46 @@ impl FixArgs { .and(Ok(true)) } } + +#[cfg(test)] +mod tests { + use super::FixArgs; + use std::ffi::OsString; + use std::io::Write as _; + use std::path::PathBuf; + + #[test] + fn get_fix_args_from_argfile() { + let mut temp = tempfile::Builder::new().tempfile().unwrap(); + let main_rs = tempfile::Builder::new().suffix(".rs").tempfile().unwrap(); + + let content = format!("/path/to/rustc\n{}\nfoobar\n", main_rs.path().display()); + temp.write_all(content.as_bytes()).unwrap(); + + let argfile = format!("@{}", temp.path().display()); + let args = ["cargo", &argfile]; + let fix_args = FixArgs::from_args(args.map(|x| x.into())).unwrap(); + assert_eq!(fix_args.rustc, PathBuf::from("/path/to/rustc")); + assert_eq!(fix_args.file, main_rs.path()); + assert_eq!(fix_args.other, vec![OsString::from("foobar")]); + } + + #[test] + fn get_fix_args_from_argfile_with_extra_arg() { + let mut temp = tempfile::Builder::new().tempfile().unwrap(); + let main_rs = tempfile::Builder::new().suffix(".rs").tempfile().unwrap(); + + let content = format!("/path/to/rustc\n{}\nfoobar\n", main_rs.path().display()); + temp.write_all(content.as_bytes()).unwrap(); + + let argfile = format!("@{}", temp.path().display()); + let args = ["cargo", &argfile, "boo!"]; + match FixArgs::from_args(args.map(|x| x.into())) { + Err(e) => assert_eq!( + e.to_string(), + "argfile `@path` cannot be combined with other arguments" + ), + Ok(_) => panic!("should fail"), + } + } +} From beaaefd90aae5951e6fe6f352ded879f32e4d309 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Fri, 8 Apr 2022 17:00:00 +0800 Subject: [PATCH 08/16] Use `CargoResult` pervasively --- src/cargo/ops/fix.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 67e742360f9..d8a85236062 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -44,7 +44,7 @@ use std::path::{Path, PathBuf}; use std::process::{self, ExitStatus}; use std::{env, fs, str}; -use anyhow::{bail, Context, Error}; +use anyhow::{bail, Context as _}; use cargo_util::{exit_status_to_string, is_simple_exit_code, paths, ProcessBuilder}; use log::{debug, trace, warn}; use rustfix::diagnostics::Diagnostic; @@ -443,7 +443,7 @@ fn rustfix_crate( filename: &Path, args: &FixArgs, config: &Config, -) -> Result<FixedCrate, Error> { +) -> CargoResult<FixedCrate> { if !args.can_run_rustfix(config)? { // This fix should not be run. Skipping... return Ok(FixedCrate::default()); @@ -548,7 +548,7 @@ fn rustfix_and_fix( rustc: &ProcessBuilder, filename: &Path, config: &Config, -) -> Result<(), Error> { +) -> CargoResult<()> { // If not empty, filter by these lints. // TODO: implement a way to specify this. let only = HashSet::new(); @@ -695,7 +695,7 @@ fn exit_with(status: ExitStatus) -> ! { process::exit(status.code().unwrap_or(3)); } -fn log_failed_fix(stderr: &[u8], status: ExitStatus) -> Result<(), Error> { +fn log_failed_fix(stderr: &[u8], status: ExitStatus) -> CargoResult<()> { let stderr = str::from_utf8(stderr).context("failed to parse rustc stderr as utf-8")?; let diagnostics = stderr @@ -776,12 +776,12 @@ struct FixArgs { } impl FixArgs { - fn get() -> Result<FixArgs, Error> { + fn get() -> CargoResult<FixArgs> { Self::from_args(env::args_os()) } // This is a separate function so that we can use it in tests. - fn from_args(argv: impl IntoIterator<Item = OsString>) -> Result<Self, Error> { + fn from_args(argv: impl IntoIterator<Item = OsString>) -> CargoResult<Self> { let mut argv = argv.into_iter(); let mut rustc = argv .nth(1) @@ -792,7 +792,7 @@ impl FixArgs { let mut other = Vec::new(); let mut format_args = Vec::new(); - let mut handle_arg = |arg: OsString| -> Result<(), Error> { + let mut handle_arg = |arg: OsString| -> CargoResult<()> { let path = PathBuf::from(arg); if path.extension().and_then(|s| s.to_str()) == Some("rs") && path.exists() { file = Some(path); From 877c0add35c76e5216b1fa3ea2ea75177a2c9aef Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Sun, 10 Apr 2022 12:57:30 +0800 Subject: [PATCH 09/16] State that arg in argfile must not contain newlines --- crates/cargo-util/src/process_builder.rs | 93 +++++++++++++++++++----- 1 file changed, 73 insertions(+), 20 deletions(-) diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index f792712b03d..0b4214498e3 100644 --- a/crates/cargo-util/src/process_builder.rs +++ b/crates/cargo-util/src/process_builder.rs @@ -37,6 +37,7 @@ pub struct ProcessBuilder { /// `true` to include environment variable in display. display_env_vars: bool, /// `true` to retry with an argfile if hitting "command line too big" error. + /// See [`ProcessBuilder::retry_with_argfile`] for more information. retry_with_argfile: bool, } @@ -185,6 +186,22 @@ impl ProcessBuilder { } /// Enables retrying with an argfile if hitting "command line too big" error + /// + /// This is primarily for the `@path` arg of rustc and rustdoc, which treat + /// each line as an command-line argument, so `LF` and `CRLF` bytes are not + /// valid as an argument for argfile at this moment. + /// For example, `RUSTDOCFLAGS="--crate-version foo\nbar" cargo doc` is + /// valid when invoking from command-line but not from argfile. + /// + /// To sum up, the limitations of the argfile are: + /// + /// - Must be valid UTF-8 encoded. + /// - Must not contain any newlines in each argument. + /// + /// Ref: + /// + /// - https://doc.rust-lang.org/rustdoc/command-line-arguments.html#path-load-command-line-flags-from-a-path + /// - https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path> pub fn retry_with_argfile(&mut self, enabled: bool) -> &mut Self { self.retry_with_argfile = enabled; self @@ -393,11 +410,6 @@ impl ProcessBuilder { /// Builds the command with an `@<path>` argfile that contains all the /// arguments. This is primarily served for rustc/rustdoc command family. - /// - /// Ref: - /// - /// - https://doc.rust-lang.org/rustdoc/command-line-arguments.html#path-load-command-line-flags-from-a-path - /// - https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path> fn build_command_with_argfile(&self) -> io::Result<(Command, NamedTempFile)> { use std::io::Write as _; @@ -411,21 +423,26 @@ impl ProcessBuilder { log::debug!("created argfile at {path} for `{self}`"); let cap = self.get_args().map(|arg| arg.len() + 1).sum::<usize>(); - let mut buf = String::with_capacity(cap); + let mut buf = Vec::with_capacity(cap); for arg in &self.args { - let arg = arg - .to_str() - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::Other, - "argument contains invalid UTF-8 characters", - ) - })?; - // TODO: Shall we escape line feed? - buf.push_str(arg); - buf.push('\n'); + let arg = arg.to_str().ok_or_else(|| { + io::Error::new( + io::ErrorKind::Other, + format!( + "argument for argfile contains invalid UTF-8 characters: `{}`", + arg.to_string_lossy() + ), + ) + })?; + if arg.contains('\n') { + return Err(io::Error::new( + io::ErrorKind::Other, + format!("argument for argfile contains newlines: `{arg}`"), + )); + } + writeln!(buf, "{arg}")?; } - tmp.write_all(buf.as_bytes())?; + tmp.write_all(&mut buf)?; Ok((cmd, tmp)) } @@ -552,10 +569,11 @@ mod imp { #[cfg(test)] mod tests { - use super::*; + use super::ProcessBuilder; use std::fs; + #[test] - fn test_argfile() { + fn argfile_build_succeeds() { let mut cmd = ProcessBuilder::new("echo"); cmd.args(["foo", "bar"].as_slice()); let (cmd, argfile) = cmd.build_command_with_argfile().unwrap(); @@ -569,4 +587,39 @@ mod tests { let buf = fs::read_to_string(argfile.path()).unwrap(); assert_eq!(buf, "foo\nbar\n"); } + + #[test] + fn argfile_build_fails_if_arg_contains_newline() { + let mut cmd = ProcessBuilder::new("echo"); + cmd.arg("foo\n"); + let err = cmd.build_command_with_argfile().unwrap_err(); + assert_eq!( + err.to_string(), + "argument for argfile contains newlines: `foo\n`" + ); + } + + #[test] + fn argfile_build_fails_if_arg_contains_invalid_utf8() { + let mut cmd = ProcessBuilder::new("echo"); + + #[cfg(windows)] + let invalid_arg = { + use std::os::windows::prelude::*; + std::ffi::OsString::from_wide(&[0x0066, 0x006f, 0xD800, 0x006f]) + }; + + #[cfg(unix)] + let invalid_arg = { + use std::os::unix::ffi::OsStrExt; + std::ffi::OsStr::from_bytes(&[0x66, 0x6f, 0x80, 0x6f]).to_os_string() + }; + + cmd.arg(invalid_arg); + let err = cmd.build_command_with_argfile().unwrap_err(); + assert_eq!( + err.to_string(), + "argument for argfile contains invalid UTF-8 characters: `fo�o`" + ); + } } From 8e0e68e647564ba5a1b778ccfd626b1cf4a673ab Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Sun, 10 Apr 2022 21:12:46 +0800 Subject: [PATCH 10/16] Avoid collect args from `env::args in proxy mode cargo in fix-proxy-mode might read `@path` argfile as what rustc does. Therefore, we should collect info from `ProcessBuilder::get_args()`. --- src/cargo/ops/fix.rs | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index d8a85236062..0b40a2942a6 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -405,7 +405,18 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> { paths::write(path, &file.original_code)?; } } - log_failed_fix(&output.stderr, output.status)?; + + let krate = { + let mut iter = json_error_rustc.get_args(); + let mut krate = None; + while let Some(arg) = iter.next() { + if arg == "--crate-name" { + krate = iter.next().and_then(|s| s.to_owned().into_string().ok()); + } + } + krate + }; + log_failed_fix(krate, &output.stderr, output.status)?; } } @@ -695,7 +706,7 @@ fn exit_with(status: ExitStatus) -> ! { process::exit(status.code().unwrap_or(3)); } -fn log_failed_fix(stderr: &[u8], status: ExitStatus) -> CargoResult<()> { +fn log_failed_fix(krate: Option<String>, stderr: &[u8], status: ExitStatus) -> CargoResult<()> { let stderr = str::from_utf8(stderr).context("failed to parse rustc stderr as utf-8")?; let diagnostics = stderr @@ -717,19 +728,6 @@ fn log_failed_fix(stderr: &[u8], status: ExitStatus) -> CargoResult<()> { .filter(|x| !x.starts_with('{')) .map(|x| x.to_string()), ); - let mut krate = None; - let mut prev_dash_dash_krate_name = false; - for arg in env::args() { - if prev_dash_dash_krate_name { - krate = Some(arg.clone()); - } - - if arg == "--crate-name" { - prev_dash_dash_krate_name = true; - } else { - prev_dash_dash_krate_name = false; - } - } let files = files.into_iter().collect(); let abnormal_exit = if status.code().map_or(false, is_simple_exit_code) { From 9155c0062bb6fcde2d8d727705841327dbe56e3f Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Sat, 9 Apr 2022 20:41:18 +0800 Subject: [PATCH 11/16] Ensure that temporary argfile lives longer them command execution --- crates/cargo-util/src/process_builder.rs | 93 ++++++++++++------------ crates/cargo-util/src/process_error.rs | 7 ++ 2 files changed, 55 insertions(+), 45 deletions(-) diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index 0b4214498e3..ebe0e7953e9 100644 --- a/crates/cargo-util/src/process_builder.rs +++ b/crates/cargo-util/src/process_builder.rs @@ -13,7 +13,7 @@ use std::fmt; use std::io; use std::iter::once; use std::path::Path; -use std::process::{Child, Command, ExitStatus, Output, Stdio}; +use std::process::{Command, ExitStatus, Output, Stdio}; /// A builder object for an external process, similar to [`std::process::Command`]. #[derive(Clone, Debug)] @@ -213,11 +213,19 @@ impl ProcessBuilder { /// Like [`Command::status`] but with a better error message. pub fn status(&self) -> Result<ExitStatus> { - self.build_and_spawn(|_| {}) - .and_then(|mut child| child.wait()) - .with_context(|| { - ProcessError::new(&format!("could not execute process {self}"), None, None) - }) + self._status() + .with_context(|| ProcessError::could_not_execute(self)) + } + + fn _status(&self) -> io::Result<ExitStatus> { + let mut cmd = self.build_command(); + match cmd.spawn() { + Err(ref e) if self.should_retry_with_argfile(e) => {} + Err(e) => return Err(e), + Ok(mut child) => return child.wait(), + } + let (mut cmd, _argfile) = self.build_command_with_argfile()?; + cmd.spawn()?.wait() } /// Runs the process, waiting for completion, and mapping non-success exit codes to an error. @@ -256,15 +264,19 @@ impl ProcessBuilder { /// Like [`Command::output`] but with a better error message. pub fn output(&self) -> Result<Output> { - self.build_and_spawn(|cmd| { - cmd.stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .stdin(Stdio::null()); - }) - .and_then(|child| child.wait_with_output()) - .with_context(|| { - ProcessError::new(&format!("could not execute process {self}"), None, None) - }) + self._output() + .with_context(|| ProcessError::could_not_execute(self)) + } + + fn _output(&self) -> io::Result<Output> { + let mut cmd = self.build_command(); + match piped(&mut cmd).spawn() { + Err(ref e) if self.should_retry_with_argfile(e) => {} + Err(e) => return Err(e), + Ok(child) => return child.wait_with_output(), + } + let (mut cmd, _argfile) = self.build_command_with_argfile()?; + piped(&mut cmd).spawn()?.wait_with_output() } /// Executes the process, returning the stdio output, or an error if non-zero exit status. @@ -303,12 +315,20 @@ impl ProcessBuilder { let mut callback_error = None; let mut stdout_pos = 0; let mut stderr_pos = 0; + + let spawn = |mut cmd| { + match piped(&mut cmd).spawn() { + Err(ref e) if self.should_retry_with_argfile(e) => {} + Err(e) => return Err(e), + Ok(child) => return Ok((child, None)), + }; + let (mut cmd, argfile) = self.build_command_with_argfile()?; + Ok((piped(&mut cmd).spawn()?, Some(argfile))) + }; + let status = (|| { - let mut child = self.build_and_spawn(|cmd| { - cmd.stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .stdin(Stdio::null()); - })?; + let cmd = self.build_command(); + let (mut child, _argfile) = spawn(cmd)?; let out = child.stdout.take().unwrap(); let err = child.stderr.take().unwrap(); read2(out, err, &mut |is_out, data, eof| { @@ -356,9 +376,7 @@ impl ProcessBuilder { })?; child.wait() })() - .with_context(|| { - ProcessError::new(&format!("could not execute process {}", self), None, None) - })?; + .with_context(|| ProcessError::could_not_execute(self))?; let output = Output { status, stdout, @@ -386,28 +404,6 @@ impl ProcessBuilder { Ok(output) } - /// Builds a command from `ProcessBuilder` and spawn it. - /// - /// There is a risk when spawning a process, it might hit "command line - /// too big" OS error. To handle those kind of OS errors, this method try - /// to reinvoke the command with a `@<path>` argfile that contains all the - /// arguments. - /// - /// * `apply`: Modify the command before invoking. Useful for updating [`Stdio`]. - fn build_and_spawn(&self, apply: impl Fn(&mut Command)) -> io::Result<Child> { - let mut cmd = self.build_command(); - apply(&mut cmd); - - match cmd.spawn() { - Err(ref e) if self.should_retry_with_argfile(e) => { - let (mut cmd, _argfile) = self.build_command_with_argfile()?; - apply(&mut cmd); - cmd.spawn() - } - res => res, - } - } - /// Builds the command with an `@<path>` argfile that contains all the /// arguments. This is primarily served for rustc/rustdoc command family. fn build_command_with_argfile(&self) -> io::Result<(Command, NamedTempFile)> { @@ -509,6 +505,13 @@ impl ProcessBuilder { } } +/// Creates new pipes for stderr and stdout. Ignores stdin. +fn piped(cmd: &mut Command) -> &mut Command { + cmd.stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(Stdio::null()) +} + #[cfg(unix)] mod imp { use super::{ProcessBuilder, ProcessError}; diff --git a/crates/cargo-util/src/process_error.rs b/crates/cargo-util/src/process_error.rs index 57feffbef67..e8607d6614d 100644 --- a/crates/cargo-util/src/process_error.rs +++ b/crates/cargo-util/src/process_error.rs @@ -95,6 +95,13 @@ impl ProcessError { stderr: stderr.map(|s| s.to_vec()), } } + + /// Creates a [`ProcessError`] with "could not execute process {cmd}". + /// + /// * `cmd` is usually but not limited to [`std::process::Command`]. + pub fn could_not_execute(cmd: impl fmt::Display) -> ProcessError { + ProcessError::new(&format!("could not execute process {cmd}"), None, None) + } } /// Converts an [`ExitStatus`] to a human-readable string suitable for From 3f749f615b6bd5afcb076fcb623f48c9f1c5701e Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Sun, 10 Apr 2022 00:46:24 +0800 Subject: [PATCH 12/16] Ensure non-UTF8 compatibility of argfile arg --- crates/cargo-util/src/process_builder.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index ebe0e7953e9..3fbad3e8995 100644 --- a/crates/cargo-util/src/process_builder.rs +++ b/crates/cargo-util/src/process_builder.rs @@ -413,10 +413,11 @@ impl ProcessBuilder { .prefix("cargo-argfile.") .tempfile()?; - let path = tmp.path().display(); + let mut arg = OsString::from("@"); + arg.push(tmp.path()); let mut cmd = self.build_command_without_args(); - cmd.arg(format!("@{path}")); - log::debug!("created argfile at {path} for `{self}`"); + cmd.arg(arg); + log::debug!("created argfile at {} for {self}", tmp.path().display()); let cap = self.get_args().map(|arg| arg.len() + 1).sum::<usize>(); let mut buf = Vec::with_capacity(cap); From 5e8827f26e599f249da01db8cf8ac6ae195f3bb8 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Sat, 9 Apr 2022 22:19:02 +0800 Subject: [PATCH 13/16] Debug assertion for rustc argfile invocations --- crates/cargo-util/src/process_builder.rs | 58 ++++++++++++++++-------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index 3fbad3e8995..66a59a2b571 100644 --- a/crates/cargo-util/src/process_builder.rs +++ b/crates/cargo-util/src/process_builder.rs @@ -218,11 +218,13 @@ impl ProcessBuilder { } fn _status(&self) -> io::Result<ExitStatus> { - let mut cmd = self.build_command(); - match cmd.spawn() { - Err(ref e) if self.should_retry_with_argfile(e) => {} - Err(e) => return Err(e), - Ok(mut child) => return child.wait(), + if !debug_force_argfile(self.retry_with_argfile) { + let mut cmd = self.build_command(); + match cmd.spawn() { + Err(ref e) if self.should_retry_with_argfile(e) => {} + Err(e) => return Err(e), + Ok(mut child) => return child.wait(), + } } let (mut cmd, _argfile) = self.build_command_with_argfile()?; cmd.spawn()?.wait() @@ -269,11 +271,13 @@ impl ProcessBuilder { } fn _output(&self) -> io::Result<Output> { - let mut cmd = self.build_command(); - match piped(&mut cmd).spawn() { - Err(ref e) if self.should_retry_with_argfile(e) => {} - Err(e) => return Err(e), - Ok(child) => return child.wait_with_output(), + if !debug_force_argfile(self.retry_with_argfile) { + let mut cmd = self.build_command(); + match piped(&mut cmd).spawn() { + Err(ref e) if self.should_retry_with_argfile(e) => {} + Err(e) => return Err(e), + Ok(child) => return child.wait_with_output(), + } } let (mut cmd, _argfile) = self.build_command_with_argfile()?; piped(&mut cmd).spawn()?.wait_with_output() @@ -317,11 +321,13 @@ impl ProcessBuilder { let mut stderr_pos = 0; let spawn = |mut cmd| { - match piped(&mut cmd).spawn() { - Err(ref e) if self.should_retry_with_argfile(e) => {} - Err(e) => return Err(e), - Ok(child) => return Ok((child, None)), - }; + if !debug_force_argfile(self.retry_with_argfile) { + match piped(&mut cmd).spawn() { + Err(ref e) if self.should_retry_with_argfile(e) => {} + Err(e) => return Err(e), + Ok(child) => return Ok((child, None)), + } + } let (mut cmd, argfile) = self.build_command_with_argfile()?; Ok((piped(&mut cmd).spawn()?, Some(argfile))) }; @@ -506,6 +512,13 @@ impl ProcessBuilder { } } +/// Forces the command to use `@path` argfile. +/// +/// You should set `__CARGO_TEST_FORCE_ARGFILE` to enable this. +fn debug_force_argfile(retry_enabled: bool) -> bool { + cfg!(debug_assertions) && env::var("__CARGO_TEST_FORCE_ARGFILE").is_ok() && retry_enabled +} + /// Creates new pipes for stderr and stdout. Ignores stdin. fn piped(cmd: &mut Command) -> &mut Command { cmd.stdout(Stdio::piped()) @@ -515,18 +528,23 @@ fn piped(cmd: &mut Command) -> &mut Command { #[cfg(unix)] mod imp { - use super::{ProcessBuilder, ProcessError}; + use super::{debug_force_argfile, ProcessBuilder, ProcessError}; use anyhow::Result; use std::io; use std::os::unix::process::CommandExt; pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> { - let mut command = process_builder.build_command(); - - let mut error = command.exec(); - if process_builder.should_retry_with_argfile(&error) { + let mut error; + if debug_force_argfile(process_builder.retry_with_argfile) { let (mut command, _argfile) = process_builder.build_command_with_argfile()?; error = command.exec() + } else { + let mut command = process_builder.build_command(); + error = command.exec(); + if process_builder.should_retry_with_argfile(&error) { + let (mut command, _argfile) = process_builder.build_command_with_argfile()?; + error = command.exec() + } } Err(anyhow::Error::from(error).context(ProcessError::new( From 7146111afecabc691daaad8070f6f084e44f6c1b Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Sun, 10 Apr 2022 01:44:40 +0800 Subject: [PATCH 14/16] Close tempfile explicitly after the command to exist --- crates/cargo-util/src/process_builder.rs | 38 ++++++++++++++++++------ 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index 66a59a2b571..ca4f4a7564a 100644 --- a/crates/cargo-util/src/process_builder.rs +++ b/crates/cargo-util/src/process_builder.rs @@ -226,8 +226,10 @@ impl ProcessBuilder { Ok(mut child) => return child.wait(), } } - let (mut cmd, _argfile) = self.build_command_with_argfile()?; - cmd.spawn()?.wait() + let (mut cmd, argfile) = self.build_command_with_argfile()?; + let status = cmd.spawn()?.wait(); + close_tempfile_and_log_error(argfile); + status } /// Runs the process, waiting for completion, and mapping non-success exit codes to an error. @@ -279,8 +281,10 @@ impl ProcessBuilder { Ok(child) => return child.wait_with_output(), } } - let (mut cmd, _argfile) = self.build_command_with_argfile()?; - piped(&mut cmd).spawn()?.wait_with_output() + let (mut cmd, argfile) = self.build_command_with_argfile()?; + let output = piped(&mut cmd).spawn()?.wait_with_output(); + close_tempfile_and_log_error(argfile); + output } /// Executes the process, returning the stdio output, or an error if non-zero exit status. @@ -334,7 +338,7 @@ impl ProcessBuilder { let status = (|| { let cmd = self.build_command(); - let (mut child, _argfile) = spawn(cmd)?; + let (mut child, argfile) = spawn(cmd)?; let out = child.stdout.take().unwrap(); let err = child.stderr.take().unwrap(); read2(out, err, &mut |is_out, data, eof| { @@ -380,7 +384,11 @@ impl ProcessBuilder { data.drain(..idx); *pos = 0; })?; - child.wait() + let status = child.wait(); + if let Some(argfile) = argfile { + close_tempfile_and_log_error(argfile); + } + status })() .with_context(|| ProcessError::could_not_execute(self))?; let output = Output { @@ -526,26 +534,38 @@ fn piped(cmd: &mut Command) -> &mut Command { .stdin(Stdio::null()) } +fn close_tempfile_and_log_error(file: NamedTempFile) { + file.close().unwrap_or_else(|e| { + log::warn!("failed to close temporary file: {e}"); + }); +} + #[cfg(unix)] mod imp { - use super::{debug_force_argfile, ProcessBuilder, ProcessError}; + use super::{close_tempfile_and_log_error, debug_force_argfile, ProcessBuilder, ProcessError}; use anyhow::Result; use std::io; use std::os::unix::process::CommandExt; pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> { let mut error; + let mut file = None; if debug_force_argfile(process_builder.retry_with_argfile) { - let (mut command, _argfile) = process_builder.build_command_with_argfile()?; + let (mut command, argfile) = process_builder.build_command_with_argfile()?; + file = Some(argfile); error = command.exec() } else { let mut command = process_builder.build_command(); error = command.exec(); if process_builder.should_retry_with_argfile(&error) { - let (mut command, _argfile) = process_builder.build_command_with_argfile()?; + let (mut command, argfile) = process_builder.build_command_with_argfile()?; + file = Some(argfile); error = command.exec() } } + if let Some(file) = file { + close_tempfile_and_log_error(file); + } Err(anyhow::Error::from(error).context(ProcessError::new( &format!("could not execute process {}", process_builder), From d813739c769a84e9d3aba82a840328eefb8ff607 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Sun, 10 Apr 2022 21:16:12 +0800 Subject: [PATCH 15/16] test: make tests compatible when force using argfile --- crates/cargo-test-support/src/tools.rs | 11 ++++++++++- tests/testsuite/fix.rs | 13 +++++++++++++ tests/testsuite/rustdocflags.rs | 1 + 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/crates/cargo-test-support/src/tools.rs b/crates/cargo-test-support/src/tools.rs index 9847af2b5b6..7c056b6fa99 100644 --- a/crates/cargo-test-support/src/tools.rs +++ b/crates/cargo-test-support/src/tools.rs @@ -24,8 +24,17 @@ pub fn echo_wrapper() -> PathBuf { .file( "src/main.rs", r#" + use std::fs::read_to_string; + use std::path::PathBuf; fn main() { - let args = std::env::args().collect::<Vec<_>>(); + // Handle args from `@path` argfile for rustc + let args = std::env::args() + .flat_map(|p| if let Some(p) = p.strip_prefix("@") { + read_to_string(p).unwrap().lines().map(String::from).collect() + } else { + vec![p] + }) + .collect::<Vec<_>>(); eprintln!("WRAPPER CALLED: {}", args[1..].join(" ")); let status = std::process::Command::new(&args[1]) .args(&args[2..]).status().unwrap(); diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index fce7ab843e5..04278b34415 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -89,8 +89,14 @@ fn broken_fixes_backed_out() { fn main() { // Ignore calls to things like --print=file-names and compiling build.rs. + // Also compatible for rustc invocations with `@path` argfile. let is_lib_rs = env::args_os() .map(PathBuf::from) + .flat_map(|p| if let Some(p) = p.to_str().unwrap_or_default().strip_prefix("@") { + fs::read_to_string(p).unwrap().lines().map(PathBuf::from).collect() + } else { + vec![p] + }) .any(|l| l == Path::new("src/lib.rs")); if is_lib_rs { let path = PathBuf::from(env::var_os("OUT_DIR").unwrap()); @@ -1319,8 +1325,15 @@ fn fix_to_broken_code() { use std::process::{self, Command}; fn main() { + // Ignore calls to things like --print=file-names and compiling build.rs. + // Also compatible for rustc invocations with `@path` argfile. let is_lib_rs = env::args_os() .map(PathBuf::from) + .flat_map(|p| if let Some(p) = p.to_str().unwrap_or_default().strip_prefix("@") { + fs::read_to_string(p).unwrap().lines().map(PathBuf::from).collect() + } else { + vec![p] + }) .any(|l| l == Path::new("src/lib.rs")); if is_lib_rs { let path = PathBuf::from(env::var_os("OUT_DIR").unwrap()); diff --git a/tests/testsuite/rustdocflags.rs b/tests/testsuite/rustdocflags.rs index b17f83c2fdf..44175de40c1 100644 --- a/tests/testsuite/rustdocflags.rs +++ b/tests/testsuite/rustdocflags.rs @@ -112,6 +112,7 @@ fn whitespace() { const SPACED_VERSION: &str = "a\nb\tc\u{00a0}d"; p.cargo("doc") + .env_remove("__CARGO_TEST_FORCE_ARGFILE") // Not applicable for argfile. .env( "RUSTDOCFLAGS", format!("--crate-version {}", SPACED_VERSION), From 275b0c68d4157045bc17341ff31422f1248c1dc0 Mon Sep 17 00:00:00 2001 From: Weihang Lo <me@weihanglo.tw> Date: Sun, 10 Apr 2022 22:21:48 +0800 Subject: [PATCH 16/16] CI: force using argfile to test againg `cargo fix` --- .github/workflows/main.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c009816a302..0e0c888854c 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -68,6 +68,13 @@ jobs: # Deny warnings on CI to avoid warnings getting into the codebase. - run: cargo test --features 'deny-warnings' + - name: Check operability of rustc invocation with argfile + env: + __CARGO_TEST_FORCE_ARGFILE: 1 + run: | + # This only tests `cargo fix` because fix-proxy-mode is one of the most + # complicated subprocess management in Cargo. + cargo test --test testsuite --features 'deny-warnings' -- fix:: - run: cargo test --features 'deny-warnings' --manifest-path crates/cargo-test-support/Cargo.toml env: CARGO_TARGET_DIR: target