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 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-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::>(); + // 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::>(); eprintln!("WRAPPER CALLED: {}", args[1..].join(" ")); let status = std::process::Command::new(&args[1]) .args(&args[2..]).status().unwrap(); 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" diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index 9c43d3b2f03..ca4f4a7564a 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::{Command, ExitStatus, Output, Stdio}; /// A builder object for an external process, similar to [`std::process::Command`]. #[derive(Clone, Debug)] @@ -22,6 +26,9 @@ pub struct ProcessBuilder { env: BTreeMap>, /// The directory to run the program from. cwd: Option, + /// A list of wrappers that wrap the original program when calling + /// [`ProcessBuilder::wrapped`]. The last one is the outermost one. + wrappers: Vec, /// The `make` jobserver. See the [jobserver crate] for /// more information. /// @@ -29,6 +36,9 @@ pub struct ProcessBuilder { jobserver: Option, /// `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, } impl fmt::Display for ProcessBuilder { @@ -48,9 +58,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,8 +76,10 @@ impl ProcessBuilder { args: Vec::new(), cwd: None, env: BTreeMap::new(), + wrappers: Vec::new(), jobserver: None, display_env_vars: false, + retry_with_argfile: false, } } @@ -92,6 +104,13 @@ impl ProcessBuilder { /// (chainable) Replaces the args list with the given `args`. pub fn args_replace>(&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 +136,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 { + 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. @@ -161,13 +185,56 @@ impl ProcessBuilder { self } + /// 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 + } + + 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 { + self._status() + .with_context(|| ProcessError::could_not_execute(self)) + } + + fn _status(&self) -> io::Result { + 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()?; + 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. 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 { @@ -199,14 +266,30 @@ impl ProcessBuilder { imp::exec_replace(self) } - /// Executes the process, returning the stdio output, or an error if non-zero exit status. - pub fn exec_with_output(&self) -> Result { - let mut command = self.build_command(); + /// Like [`Command::output`] but with a better error message. + pub fn output(&self) -> Result { + self._output() + .with_context(|| ProcessError::could_not_execute(self)) + } - let output = command.output().with_context(|| { - ProcessError::new(&format!("could not execute process {}", self), None, None) - })?; + fn _output(&self) -> io::Result { + 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()?; + 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. + pub fn exec_with_output(&self) -> Result { + let output = self.output()?; if output.status.success() { Ok(output) } else { @@ -237,16 +320,25 @@ 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 spawn = |mut cmd| { + 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))) + }; + let status = (|| { - let mut child = cmd.spawn()?; + 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| { @@ -292,11 +384,13 @@ 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::new(&format!("could not execute process {}", self), None, None) - })?; + .with_context(|| ProcessError::could_not_execute(self))?; let output = Output { status, stdout, @@ -324,16 +418,56 @@ impl ProcessBuilder { Ok(output) } - /// 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); + /// Builds the command with an `@` 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)> { + use std::io::Write as _; + + let mut tmp = tempfile::Builder::new() + .prefix("cargo-argfile.") + .tempfile()?; + + let mut arg = OsString::from("@"); + arg.push(tmp.path()); + let mut cmd = self.build_command_without_args(); + cmd.arg(arg); + log::debug!("created argfile at {} for {self}", tmp.path().display()); + + let cap = self.get_args().map(|arg| arg.len() + 1).sum::(); + 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, + 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(&mut buf)?; + 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")); + cmd.args(iter); + cmd + }; 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) => { @@ -350,6 +484,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 @@ -363,46 +510,80 @@ impl ProcessBuilder { /// let cmd = cmd.wrapped(Some("sccache")); /// ``` pub fn wrapped(mut self, wrapper: Option>) -> 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()); + } } + self + } +} - let args = once(self.program).chain(self.args.into_iter()).collect(); +/// 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 +} - self.program = wrapper.to_os_string(); - self.args = args; +/// 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()) +} - self - } +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::{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 command = process_builder.build_command(); - let error = command.exec(); + 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()?; + 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()?; + 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), 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; @@ -421,4 +602,66 @@ 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::ProcessBuilder; + use std::fs; + + #[test] + 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(); + + 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"); + } + + #[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`" + ); + } } 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 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/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/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 { // 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/ops/fix.rs b/src/cargo/ops/fix.rs index 75b2d011e4f..0b40a2942a6 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -39,13 +39,12 @@ //! 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, Command, ExitStatus}; -use std::str; +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; @@ -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,10 +354,17 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult { .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); 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 +375,8 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult { // 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() { @@ -399,7 +405,18 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult { 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)?; } } @@ -407,15 +424,13 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult { // - 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)] @@ -439,7 +454,7 @@ fn rustfix_crate( filename: &Path, args: &FixArgs, config: &Config, -) -> Result { +) -> CargoResult { if !args.can_run_rustfix(config)? { // This fix should not be run. Skipping... return Ok(FixedCrate::default()); @@ -502,7 +517,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 +558,14 @@ fn rustfix_and_fix( fixes: &mut FixedCrate, rustc: &ProcessBuilder, filename: &Path, - args: &FixArgs, config: &Config, -) -> Result<(), Error> { +) -> CargoResult<()> { // 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 @@ -703,7 +706,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(krate: Option, stderr: &[u8], status: ExitStatus) -> CargoResult<()> { let stderr = str::from_utf8(stderr).context("failed to parse rustc stderr as utf-8")?; let diagnostics = stderr @@ -725,19 +728,6 @@ fn log_failed_fix(stderr: &[u8], status: ExitStatus) -> Result<(), Error> { .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) { @@ -784,36 +774,66 @@ struct FixArgs { } impl FixArgs { - fn get() -> Result { - let rustc = env::args_os() + fn get() -> CargoResult { + 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) -> CargoResult { + 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| -> CargoResult<()> { 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(); @@ -834,7 +854,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() { @@ -925,3 +945,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"), + } + } +} diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 33f96c15532..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. @@ -231,10 +237,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))?; @@ -351,7 +354,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::>(); env.sort_unstable(); env.hash(&mut hasher); 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),