From 9161b3a9e8b1d497aaab9c080deec4f27999bdce Mon Sep 17 00:00:00 2001 From: MaulingMonkey Date: Tue, 31 Dec 2019 17:53:37 -0800 Subject: [PATCH 1/8] Use response files when invoking rustc This would workaround windows command line length limits when using lots and lots of features. https://github.com/rust-lang/cargo/issues/7759 --- crates/cargo-test-support/src/lib.rs | 6 +-- src/cargo/core/compiler/mod.rs | 2 + src/cargo/util/command_and_response_file.rs | 24 +++++++++++ src/cargo/util/mod.rs | 2 + src/cargo/util/process_builder.rs | 46 +++++++++++++++++---- 5 files changed, 70 insertions(+), 10 deletions(-) create mode 100644 src/cargo/util/command_and_response_file.rs diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index d18fa5ddaa3..1f36a1dd627 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -115,13 +115,13 @@ use std::fs; use std::io::prelude::*; use std::os; use std::path::{Path, PathBuf}; -use std::process::{Command, Output}; +use std::process::{Output}; use std::str; use std::time::{self, Duration}; use std::usize; use cargo; -use cargo::util::{is_ci, CargoResult, ProcessBuilder, ProcessError, Rustc}; +use cargo::util::{is_ci, CargoResult, CommandAndResponseFile, ProcessBuilder, ProcessError, Rustc}; use filetime; use serde_json::{self, Value}; use url::Url; @@ -828,7 +828,7 @@ impl Execs { p.exec_with_output() } - pub fn build_command(&mut self) -> Command { + pub fn build_command(&mut self) -> CommandAndResponseFile { self.ran = true; // TODO avoid unwrap let p = (&self.process_builder).clone().unwrap(); diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index cba5764bbe9..74ff191198d 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -548,6 +548,7 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult let bcx = cx.bcx; let mut rustdoc = cx.compilation.rustdoc_process(unit.pkg, unit.target)?; rustdoc.inherit_jobserver(&cx.jobserver); + rustdoc.response_file(); rustdoc.arg("--crate-name").arg(&unit.target.crate_name()); add_path_args(bcx, unit, &mut rustdoc); add_cap_lints(bcx, unit, &mut rustdoc); @@ -709,6 +710,7 @@ fn build_base_args<'a, 'cfg>( } = unit.profile; let test = unit.mode.is_any_test(); + cmd.response_file(); cmd.arg("--crate-name").arg(&unit.target.crate_name()); let edition = unit.target.edition(); diff --git a/src/cargo/util/command_and_response_file.rs b/src/cargo/util/command_and_response_file.rs new file mode 100644 index 00000000000..0595e820ced --- /dev/null +++ b/src/cargo/util/command_and_response_file.rs @@ -0,0 +1,24 @@ +use std::ops::{Deref, DerefMut}; +use std::process::Command; + +use tempfile::TempPath; + +/// A wrapper around `Command` which extends the lifetime of associated +/// temporary response files until the command is executed. +pub struct CommandAndResponseFile { + pub command: Command, + pub response_file: Option, +} + +impl Deref for CommandAndResponseFile { + type Target = Command; + fn deref(&self) -> &Self::Target { + &self.command + } +} + +impl DerefMut for CommandAndResponseFile { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.command + } +} diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 94946ae36de..de6827307d0 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -1,6 +1,7 @@ use std::time::Duration; pub use self::canonical_url::CanonicalUrl; +pub use self::command_and_response_file::CommandAndResponseFile; pub use self::config::{homedir, Config, ConfigValue}; pub use self::dependency_queue::DependencyQueue; pub use self::diagnostic_server::RustfixDiagnosticServer; @@ -29,6 +30,7 @@ pub use self::workspace::{ }; mod canonical_url; +mod command_and_response_file; pub mod command_prelude; pub mod config; pub mod cpu; diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index 9ac8ff1cbcd..0802f97f02d 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use std::env; use std::ffi::{OsStr, OsString}; use std::fmt; +use std::io::Write; use std::path::Path; use std::process::{Command, Output, Stdio}; @@ -9,7 +10,7 @@ use failure::Fail; use jobserver::Client; use shell_escape::escape; -use crate::util::{process_error, read2, CargoResult, CargoResultExt}; +use crate::util::{internal, process_error, read2, CargoResult, CargoResultExt, CommandAndResponseFile}; /// A builder object for an external process, similar to `std::process::Command`. #[derive(Clone, Debug)] @@ -29,6 +30,10 @@ pub struct ProcessBuilder { jobserver: Option, /// `true` to include environment variable in display. display_env_vars: bool, + /// `true` to use a [@response_file] to workaround command line length limits. + /// + /// [@response_file]: https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path + response_file: bool, } impl fmt::Display for ProcessBuilder { @@ -149,6 +154,14 @@ impl ProcessBuilder { self } + /// Enables the use of a [@response_file] to workaround command line length limits. + /// + /// [@response_file]: https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path + pub fn response_file(&mut self) -> &mut Self { + self.response_file = true; + self + } + /// Runs the process, waiting for completion, and mapping non-success exit codes to an error. pub fn exec(&self) -> CargoResult<()> { let mut command = self.build_command(); @@ -304,16 +317,22 @@ impl ProcessBuilder { Ok(output) } - /// Converts `ProcessBuilder` into a `std::process::Command`, and handles the jobserver, if + /// Converts `ProcessBuilder` into a `CommandAndResponseFile`, and handles the jobserver, if /// present. - pub fn build_command(&self) -> Command { + pub fn build_command(&self) -> CommandAndResponseFile { let mut command = Command::new(&self.program); if let Some(cwd) = self.get_cwd() { command.current_dir(cwd); } - for arg in &self.args { - command.arg(arg); - } + let response_file = if let Ok(Some(file)) = self.build_response_file() { + command.arg(file.to_path_buf()); + Some(file) + } else { + for arg in &self.args { + command.arg(arg); + } + None + }; for (k, v) in &self.env { match *v { Some(ref v) => { @@ -327,7 +346,19 @@ impl ProcessBuilder { if let Some(ref c) = self.jobserver { c.configure(&mut command); } - command + CommandAndResponseFile { command, response_file } + } + + fn build_response_file(&self) -> CargoResult> { + if !self.response_file || self.args.len() == 0 { + return Ok(None); + } + let mut file = tempfile::NamedTempFile::new()?; + for arg in &self.args { + let arg = arg.to_str().ok_or_else(|| internal(format!("argument {:?} contains invalid unicode", arg)))?; + writeln!(file, "{}", arg)?; + } + Ok(Some(file.into_temp_path())) } } @@ -340,6 +371,7 @@ pub fn process>(cmd: T) -> ProcessBuilder { env: HashMap::new(), jobserver: None, display_env_vars: false, + response_file: false, } } From a0ed3800ce09fb9eec869902e39dddecbee3cf76 Mon Sep 17 00:00:00 2001 From: MaulingMonkey Date: Wed, 1 Jan 2020 16:44:11 -0800 Subject: [PATCH 2/8] Prefix response file paths with '@' --- src/cargo/util/process_builder.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index 0802f97f02d..27af8706240 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -325,7 +325,9 @@ impl ProcessBuilder { command.current_dir(cwd); } let response_file = if let Ok(Some(file)) = self.build_response_file() { - command.arg(file.to_path_buf()); + let mut arg = OsString::from("@"); + arg.push(file.to_path_buf()); + command.arg(arg); Some(file) } else { for arg in &self.args { From 7869ec09676d430580d984383c56c1d7641a2a06 Mon Sep 17 00:00:00 2001 From: MaulingMonkey Date: Wed, 1 Jan 2020 19:07:28 -0800 Subject: [PATCH 3/8] Only use a response file for rustc. It turns out none of the following work: rustdoc @... cargo @... cargo fix @... And build_base_args gets used on more than just rustc commands. --- src/cargo/core/compiler/mod.rs | 2 -- src/cargo/util/process_builder.rs | 17 ++++++++++------- src/cargo/util/rustc.rs | 11 +++++++++-- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 74ff191198d..cba5764bbe9 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -548,7 +548,6 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult let bcx = cx.bcx; let mut rustdoc = cx.compilation.rustdoc_process(unit.pkg, unit.target)?; rustdoc.inherit_jobserver(&cx.jobserver); - rustdoc.response_file(); rustdoc.arg("--crate-name").arg(&unit.target.crate_name()); add_path_args(bcx, unit, &mut rustdoc); add_cap_lints(bcx, unit, &mut rustdoc); @@ -710,7 +709,6 @@ fn build_base_args<'a, 'cfg>( } = unit.profile; let test = unit.mode.is_any_test(); - cmd.response_file(); cmd.arg("--crate-name").arg(&unit.target.crate_name()); let edition = unit.target.edition(); diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index 27af8706240..4ab16c55bc6 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -30,10 +30,10 @@ pub struct ProcessBuilder { jobserver: Option, /// `true` to include environment variable in display. display_env_vars: bool, - /// `true` to use a [@response_file] to workaround command line length limits. + /// Use a [@response_file] for all args after this index to workaround command line length limits. /// /// [@response_file]: https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path - response_file: bool, + response_file_after_arg: usize, } impl fmt::Display for ProcessBuilder { @@ -154,11 +154,11 @@ impl ProcessBuilder { self } - /// Enables the use of a [@response_file] to workaround command line length limits. + /// Use a [@response_file] for subsequent arguments to workaround command line length limits. /// /// [@response_file]: https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path pub fn response_file(&mut self) -> &mut Self { - self.response_file = true; + self.response_file_after_arg = self.response_file_after_arg.min(self.args.len()); self } @@ -325,6 +325,9 @@ impl ProcessBuilder { command.current_dir(cwd); } let response_file = if let Ok(Some(file)) = self.build_response_file() { + for arg in &self.args[..self.response_file_after_arg] { + command.arg(arg); + } let mut arg = OsString::from("@"); arg.push(file.to_path_buf()); command.arg(arg); @@ -352,11 +355,11 @@ impl ProcessBuilder { } fn build_response_file(&self) -> CargoResult> { - if !self.response_file || self.args.len() == 0 { + if self.response_file_after_arg >= self.args.len() { return Ok(None); } let mut file = tempfile::NamedTempFile::new()?; - for arg in &self.args { + for arg in &self.args[self.response_file_after_arg..] { let arg = arg.to_str().ok_or_else(|| internal(format!("argument {:?} contains invalid unicode", arg)))?; writeln!(file, "{}", arg)?; } @@ -373,7 +376,7 @@ pub fn process>(cmd: T) -> ProcessBuilder { env: HashMap::new(), jobserver: None, display_env_vars: false, - response_file: false, + response_file_after_arg: std::usize::MAX, } } diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index eafdc8dd941..62acb77aa2a 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -77,9 +77,14 @@ impl Rustc { Some(ref wrapper) if !wrapper.get_program().is_empty() => { let mut cmd = wrapper.clone(); cmd.arg(path.as_ref()); + cmd.response_file(); cmd } - _ => util::process(path.as_ref()), + _ => { + let mut cmd = util::process(path.as_ref()); + cmd.response_file(); + cmd + }, } } @@ -89,7 +94,9 @@ impl Rustc { } pub fn process_no_wrapper(&self) -> ProcessBuilder { - util::process(&self.path) + let mut cmd = util::process(&self.path); + cmd.response_file(); + cmd } pub fn cached_output(&self, cmd: &ProcessBuilder) -> CargoResult<(String, String)> { From 44c9d1078f4891bdef8ad8615ddaecaa57d31721 Mon Sep 17 00:00:00 2001 From: MaulingMonkey Date: Wed, 1 Jan 2020 19:34:33 -0800 Subject: [PATCH 4/8] rustfmt --- src/cargo/util/command_and_response_file.rs | 4 ++-- src/cargo/util/process_builder.rs | 17 ++++++++++++----- src/cargo/util/rustc.rs | 2 +- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/command_and_response_file.rs b/src/cargo/util/command_and_response_file.rs index 0595e820ced..3118a3e4496 100644 --- a/src/cargo/util/command_and_response_file.rs +++ b/src/cargo/util/command_and_response_file.rs @@ -6,8 +6,8 @@ use tempfile::TempPath; /// A wrapper around `Command` which extends the lifetime of associated /// temporary response files until the command is executed. pub struct CommandAndResponseFile { - pub command: Command, - pub response_file: Option, + pub command: Command, + pub response_file: Option, } impl Deref for CommandAndResponseFile { diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index 4ab16c55bc6..76bd780b918 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -10,7 +10,9 @@ use failure::Fail; use jobserver::Client; use shell_escape::escape; -use crate::util::{internal, process_error, read2, CargoResult, CargoResultExt, CommandAndResponseFile}; +use crate::util::{ + internal, process_error, read2, CargoResult, CargoResultExt, CommandAndResponseFile, +}; /// A builder object for an external process, similar to `std::process::Command`. #[derive(Clone, Debug)] @@ -31,7 +33,7 @@ pub struct ProcessBuilder { /// `true` to include environment variable in display. display_env_vars: bool, /// Use a [@response_file] for all args after this index to workaround command line length limits. - /// + /// /// [@response_file]: https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path response_file_after_arg: usize, } @@ -155,7 +157,7 @@ impl ProcessBuilder { } /// Use a [@response_file] for subsequent arguments to workaround command line length limits. - /// + /// /// [@response_file]: https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path pub fn response_file(&mut self) -> &mut Self { self.response_file_after_arg = self.response_file_after_arg.min(self.args.len()); @@ -351,7 +353,10 @@ impl ProcessBuilder { if let Some(ref c) = self.jobserver { c.configure(&mut command); } - CommandAndResponseFile { command, response_file } + CommandAndResponseFile { + command, + response_file, + } } fn build_response_file(&self) -> CargoResult> { @@ -360,7 +365,9 @@ impl ProcessBuilder { } let mut file = tempfile::NamedTempFile::new()?; for arg in &self.args[self.response_file_after_arg..] { - let arg = arg.to_str().ok_or_else(|| internal(format!("argument {:?} contains invalid unicode", arg)))?; + let arg = arg + .to_str() + .ok_or_else(|| internal(format!("argument {:?} contains invalid unicode", arg)))?; writeln!(file, "{}", arg)?; } Ok(Some(file.into_temp_path())) diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 62acb77aa2a..82ebb7974de 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -84,7 +84,7 @@ impl Rustc { let mut cmd = util::process(path.as_ref()); cmd.response_file(); cmd - }, + } } } From 60437cb3fa025cf3ecc0ddce09908de0c78ded99 Mon Sep 17 00:00:00 2001 From: MaulingMonkey Date: Wed, 1 Jan 2020 19:42:28 -0800 Subject: [PATCH 5/8] cargo +nightly fmt --all --bribe-ci --- crates/cargo-test-support/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 1f36a1dd627..614b9506c47 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -115,13 +115,15 @@ use std::fs; use std::io::prelude::*; use std::os; use std::path::{Path, PathBuf}; -use std::process::{Output}; +use std::process::Output; use std::str; use std::time::{self, Duration}; use std::usize; use cargo; -use cargo::util::{is_ci, CargoResult, CommandAndResponseFile, ProcessBuilder, ProcessError, Rustc}; +use cargo::util::{ + is_ci, CargoResult, CommandAndResponseFile, ProcessBuilder, ProcessError, Rustc, +}; use filetime; use serde_json::{self, Value}; use url::Url; From b9a341ad1ec495d7f679f1e2c225a4974af27792 Mon Sep 17 00:00:00 2001 From: MaulingMonkey Date: Mon, 6 Jan 2020 17:33:09 -0800 Subject: [PATCH 6/8] Only use response files when exceeding CLI limits. Since we'll only resort to response files when we've already failing, it no longer makes sense to configure this per call site in a potentially fiddly manner. As such, this also reverts many of my earlier changes. --- crates/cargo-test-support/src/lib.rs | 8 +- src/cargo/util/command_and_response_file.rs | 24 ---- src/cargo/util/mod.rs | 2 - src/cargo/util/process_builder.rs | 146 ++++++++++++-------- src/cargo/util/rustc.rs | 11 +- 5 files changed, 93 insertions(+), 98 deletions(-) delete mode 100644 src/cargo/util/command_and_response_file.rs diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 614b9506c47..d18fa5ddaa3 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -115,15 +115,13 @@ use std::fs; use std::io::prelude::*; use std::os; use std::path::{Path, PathBuf}; -use std::process::Output; +use std::process::{Command, Output}; use std::str; use std::time::{self, Duration}; use std::usize; use cargo; -use cargo::util::{ - is_ci, CargoResult, CommandAndResponseFile, ProcessBuilder, ProcessError, Rustc, -}; +use cargo::util::{is_ci, CargoResult, ProcessBuilder, ProcessError, Rustc}; use filetime; use serde_json::{self, Value}; use url::Url; @@ -830,7 +828,7 @@ impl Execs { p.exec_with_output() } - pub fn build_command(&mut self) -> CommandAndResponseFile { + pub fn build_command(&mut self) -> Command { self.ran = true; // TODO avoid unwrap let p = (&self.process_builder).clone().unwrap(); diff --git a/src/cargo/util/command_and_response_file.rs b/src/cargo/util/command_and_response_file.rs deleted file mode 100644 index 3118a3e4496..00000000000 --- a/src/cargo/util/command_and_response_file.rs +++ /dev/null @@ -1,24 +0,0 @@ -use std::ops::{Deref, DerefMut}; -use std::process::Command; - -use tempfile::TempPath; - -/// A wrapper around `Command` which extends the lifetime of associated -/// temporary response files until the command is executed. -pub struct CommandAndResponseFile { - pub command: Command, - pub response_file: Option, -} - -impl Deref for CommandAndResponseFile { - type Target = Command; - fn deref(&self) -> &Self::Target { - &self.command - } -} - -impl DerefMut for CommandAndResponseFile { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.command - } -} diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index de6827307d0..94946ae36de 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -1,7 +1,6 @@ use std::time::Duration; pub use self::canonical_url::CanonicalUrl; -pub use self::command_and_response_file::CommandAndResponseFile; pub use self::config::{homedir, Config, ConfigValue}; pub use self::dependency_queue::DependencyQueue; pub use self::diagnostic_server::RustfixDiagnosticServer; @@ -30,7 +29,6 @@ pub use self::workspace::{ }; mod canonical_url; -mod command_and_response_file; pub mod command_prelude; pub mod config; pub mod cpu; diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index 76bd780b918..401404bc657 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -4,15 +4,13 @@ use std::ffi::{OsStr, OsString}; use std::fmt; use std::io::Write; use std::path::Path; -use std::process::{Command, Output, Stdio}; +use std::process::{Command, ExitStatus, Output, Stdio}; use failure::Fail; use jobserver::Client; use shell_escape::escape; -use crate::util::{ - internal, process_error, read2, CargoResult, CargoResultExt, CommandAndResponseFile, -}; +use crate::util::{internal, process_error, read2, CargoResult, CargoResultExt}; /// A builder object for an external process, similar to `std::process::Command`. #[derive(Clone, Debug)] @@ -32,10 +30,6 @@ pub struct ProcessBuilder { jobserver: Option, /// `true` to include environment variable in display. display_env_vars: bool, - /// Use a [@response_file] for all args after this index to workaround command line length limits. - /// - /// [@response_file]: https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path - response_file_after_arg: usize, } impl fmt::Display for ProcessBuilder { @@ -156,20 +150,15 @@ impl ProcessBuilder { self } - /// Use a [@response_file] for subsequent arguments to workaround command line length limits. - /// - /// [@response_file]: https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path - pub fn response_file(&mut self) -> &mut Self { - self.response_file_after_arg = self.response_file_after_arg.min(self.args.len()); - self - } - /// Runs the process, waiting for completion, and mapping non-success exit codes to an error. pub fn exec(&self) -> CargoResult<()> { - let mut command = self.build_command(); - let exit = command.status().chain_err(|| { - process_error(&format!("could not execute process {}", self), None, None) - })?; + let exit = match self.build_command().status() { + Err(ref err) if imp::command_line_too_big(err) => self + .build_command_and_response_file() + .and_then(|(mut cmd, _file)| cmd.status().map_err(|e| e.into())), + other => other.map_err(|e| e.into()), + } + .chain_err(|| process_error(&format!("could not execute process {}", self), None, None))?; if exit.success() { Ok(()) @@ -204,11 +193,13 @@ impl ProcessBuilder { /// Executes the process, returning the stdio output, or an error if non-zero exit status. pub fn exec_with_output(&self) -> CargoResult { - let mut command = self.build_command(); - - let output = command.output().chain_err(|| { - process_error(&format!("could not execute process {}", self), None, None) - })?; + let output = match self.build_command().output() { + Err(ref err) if imp::command_line_too_big(err) => self + .build_command_and_response_file() + .and_then(|(mut cmd, _file)| cmd.output().map_err(|e| e.into())), + other => other.map_err(|e| e.into()), + } + .chain_err(|| process_error(&format!("could not execute process {}", self), None, None))?; if output.status.success() { Ok(output) @@ -246,8 +237,20 @@ impl ProcessBuilder { .stdin(Stdio::null()); let mut callback_error = None; - let status = (|| { - let mut child = cmd.spawn()?; + let status = (|| -> CargoResult { + let mut response_file = None; + let mut child = match cmd.spawn() { + Err(ref err) if imp::command_line_too_big(err) => self + .build_command_and_response_file() + .and_then(|(mut cmd, file)| { + cmd.stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(Stdio::null()); + response_file = Some(file); + Ok(cmd.spawn()?) + }), + other => other.map_err(|e| e.into()), + }?; let out = child.stdout.take().unwrap(); let err = child.stderr.take().unwrap(); read2(out, err, &mut |is_out, data, eof| { @@ -288,7 +291,7 @@ impl ProcessBuilder { data.drain(..idx); } })?; - child.wait() + Ok(child.wait()?) })() .chain_err(|| process_error(&format!("could not execute process {}", self), None, None))?; let output = Output { @@ -319,27 +322,16 @@ impl ProcessBuilder { Ok(output) } - /// Converts `ProcessBuilder` into a `CommandAndResponseFile`, and handles the jobserver, if + /// Converts `ProcessBuilder` into a `std::process::Command`, and handles the jobserver, if /// present. - pub fn build_command(&self) -> CommandAndResponseFile { + pub fn build_command(&self) -> Command { let mut command = Command::new(&self.program); if let Some(cwd) = self.get_cwd() { command.current_dir(cwd); } - let response_file = if let Ok(Some(file)) = self.build_response_file() { - for arg in &self.args[..self.response_file_after_arg] { - command.arg(arg); - } - let mut arg = OsString::from("@"); - arg.push(file.to_path_buf()); + for arg in &self.args { command.arg(arg); - Some(file) - } else { - for arg in &self.args { - command.arg(arg); - } - None - }; + } for (k, v) in &self.env { match *v { Some(ref v) => { @@ -353,24 +345,54 @@ impl ProcessBuilder { if let Some(ref c) = self.jobserver { c.configure(&mut command); } - CommandAndResponseFile { - command, - response_file, - } + command } - fn build_response_file(&self) -> CargoResult> { - if self.response_file_after_arg >= self.args.len() { - return Ok(None); + /// Converts `ProcessBuilder` into a `std::process::Command` and a rustc style response + /// file. Also handles the jobserver, if present. + pub fn build_command_and_response_file(&self) -> CargoResult<(Command, tempfile::TempPath)> { + // The rust linker also jumps through similar hoops, although with a different + // of response file, which this borrows from. Some references: + // https://github.com/rust-lang/rust/blob/ef92009c1dbe2750f1d24a6619b827721fb49749/src/librustc_codegen_ssa/back/link.rs#L935 + // https://github.com/rust-lang/rust/blob/ef92009c1dbe2750f1d24a6619b827721fb49749/src/librustc_codegen_ssa/back/command.rs#L135 + + let mut command = Command::new(&self.program); + if let Some(cwd) = self.get_cwd() { + command.current_dir(cwd); } - let mut file = tempfile::NamedTempFile::new()?; - for arg in &self.args[self.response_file_after_arg..] { - let arg = arg - .to_str() - .ok_or_else(|| internal(format!("argument {:?} contains invalid unicode", arg)))?; - writeln!(file, "{}", arg)?; + // cmd.exe can handle up to 8k work of args, this leaves some headroom if using a .cmd rustc wrapper. + let mut cmd_remaining: usize = 1024 * 6; + let mut response_file = tempfile::NamedTempFile::new()?; + for arg in &self.args { + cmd_remaining = cmd_remaining.saturating_sub(arg.len()); + if cmd_remaining > 0 { + command.arg(arg); + } else if let Some(arg) = arg.to_str() { + writeln!(response_file, "{}", arg)?; + } else { + return Err(internal(format!( + "argument {:?} contains invalid unicode", + arg + ))); + } } - Ok(Some(file.into_temp_path())) + let mut arg = OsString::from("@"); + arg.push(response_file.path()); + command.arg(arg); + for (k, v) in &self.env { + match *v { + Some(ref v) => { + command.env(k, v); + } + None => { + command.env_remove(k); + } + } + } + if let Some(ref c) = self.jobserver { + c.configure(&mut command); + } + Ok((command, response_file.into_temp_path())) } } @@ -383,7 +405,6 @@ pub fn process>(cmd: T) -> ProcessBuilder { env: HashMap::new(), jobserver: None, display_env_vars: false, - response_file_after_arg: std::usize::MAX, } } @@ -404,6 +425,10 @@ mod imp { )) .into()) } + + pub fn command_line_too_big(err: &std::io::Error) -> bool { + err.raw_os_error() == Some(::libc::E2BIG) + } } #[cfg(windows)] @@ -428,4 +453,9 @@ mod imp { // Just execute the process as normal. process_builder.exec() } + + pub fn command_line_too_big(err: &std::io::Error) -> bool { + const ERROR_FILENAME_EXCED_RANGE: i32 = 206; + err.raw_os_error() == Some(ERROR_FILENAME_EXCED_RANGE) + } } diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 82ebb7974de..eafdc8dd941 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -77,14 +77,9 @@ impl Rustc { Some(ref wrapper) if !wrapper.get_program().is_empty() => { let mut cmd = wrapper.clone(); cmd.arg(path.as_ref()); - cmd.response_file(); - cmd - } - _ => { - let mut cmd = util::process(path.as_ref()); - cmd.response_file(); cmd } + _ => util::process(path.as_ref()), } } @@ -94,9 +89,7 @@ impl Rustc { } pub fn process_no_wrapper(&self) -> ProcessBuilder { - let mut cmd = util::process(&self.path); - cmd.response_file(); - cmd + util::process(&self.path) } pub fn cached_output(&self, cmd: &ProcessBuilder) -> CargoResult<(String, String)> { From 563fdf879d48812e1732f9070b0427cdb82d3c97 Mon Sep 17 00:00:00 2001 From: MaulingMonkey Date: Tue, 7 Jan 2020 17:14:08 -0800 Subject: [PATCH 7/8] process_builder style cleanup --- src/cargo/util/process_builder.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index 401404bc657..d81e5d8e696 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -10,7 +10,7 @@ use failure::Fail; use jobserver::Client; use shell_escape::escape; -use crate::util::{internal, process_error, read2, CargoResult, CargoResultExt}; +use crate::util::{process_error, read2, CargoResult, CargoResultExt}; /// A builder object for an external process, similar to `std::process::Command`. #[derive(Clone, Debug)] @@ -370,10 +370,7 @@ impl ProcessBuilder { } else if let Some(arg) = arg.to_str() { writeln!(response_file, "{}", arg)?; } else { - return Err(internal(format!( - "argument {:?} contains invalid unicode", - arg - ))); + failure::bail!("argument {:?} contains invalid unicode", arg); } } let mut arg = OsString::from("@"); @@ -427,7 +424,7 @@ mod imp { } pub fn command_line_too_big(err: &std::io::Error) -> bool { - err.raw_os_error() == Some(::libc::E2BIG) + err.raw_os_error() == Some(libc::E2BIG) } } @@ -436,6 +433,7 @@ mod imp { use crate::util::{process_error, ProcessBuilder}; use crate::CargoResult; use winapi::shared::minwindef::{BOOL, DWORD, FALSE, TRUE}; + use winapi::shared::winerror::ERROR_FILENAME_EXCED_RANGE; use winapi::um::consoleapi::SetConsoleCtrlHandler; unsafe extern "system" fn ctrlc_handler(_: DWORD) -> BOOL { @@ -455,7 +453,6 @@ mod imp { } pub fn command_line_too_big(err: &std::io::Error) -> bool { - const ERROR_FILENAME_EXCED_RANGE: i32 = 206; - err.raw_os_error() == Some(ERROR_FILENAME_EXCED_RANGE) + err.raw_os_error() == Some(ERROR_FILENAME_EXCED_RANGE as i32) } } From f3ff04b1c5d5671e27cea9f0cc146d5f57496ae9 Mon Sep 17 00:00:00 2001 From: MaulingMonkey Date: Tue, 7 Jan 2020 18:04:52 -0800 Subject: [PATCH 8/8] Pull process_builder retry logic into status/output/spawn_with --- src/cargo/util/process_builder.rs | 81 ++++++++++++++++++------------- 1 file changed, 46 insertions(+), 35 deletions(-) diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index d81e5d8e696..62862e88575 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -4,11 +4,12 @@ use std::ffi::{OsStr, OsString}; use std::fmt; use std::io::Write; use std::path::Path; -use std::process::{Command, ExitStatus, Output, Stdio}; +use std::process::{Child, Command, ExitStatus, Output, Stdio}; use failure::Fail; use jobserver::Client; use shell_escape::escape; +use tempfile::{NamedTempFile, TempPath}; use crate::util::{process_error, read2, CargoResult, CargoResultExt}; @@ -152,13 +153,9 @@ impl ProcessBuilder { /// Runs the process, waiting for completion, and mapping non-success exit codes to an error. pub fn exec(&self) -> CargoResult<()> { - let exit = match self.build_command().status() { - Err(ref err) if imp::command_line_too_big(err) => self - .build_command_and_response_file() - .and_then(|(mut cmd, _file)| cmd.status().map_err(|e| e.into())), - other => other.map_err(|e| e.into()), - } - .chain_err(|| process_error(&format!("could not execute process {}", self), None, None))?; + let exit = self.status().chain_err(|| { + process_error(&format!("could not execute process {}", self), None, None) + })?; if exit.success() { Ok(()) @@ -193,13 +190,9 @@ impl ProcessBuilder { /// Executes the process, returning the stdio output, or an error if non-zero exit status. pub fn exec_with_output(&self) -> CargoResult { - let output = match self.build_command().output() { - Err(ref err) if imp::command_line_too_big(err) => self - .build_command_and_response_file() - .and_then(|(mut cmd, _file)| cmd.output().map_err(|e| e.into())), - other => other.map_err(|e| e.into()), - } - .chain_err(|| process_error(&format!("could not execute process {}", self), None, None))?; + let output = self.output().chain_err(|| { + process_error(&format!("could not execute process {}", self), None, None) + })?; if output.status.success() { Ok(output) @@ -231,26 +224,13 @@ 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 status = (|| -> CargoResult { - let mut response_file = None; - let mut child = match cmd.spawn() { - Err(ref err) if imp::command_line_too_big(err) => self - .build_command_and_response_file() - .and_then(|(mut cmd, file)| { - cmd.stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .stdin(Stdio::null()); - response_file = Some(file); - Ok(cmd.spawn()?) - }), - other => other.map_err(|e| e.into()), - }?; + let (mut child, _response_file) = self.spawn_with(|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| { @@ -350,7 +330,7 @@ impl ProcessBuilder { /// Converts `ProcessBuilder` into a `std::process::Command` and a rustc style response /// file. Also handles the jobserver, if present. - pub fn build_command_and_response_file(&self) -> CargoResult<(Command, tempfile::TempPath)> { + pub fn build_command_and_response_file(&self) -> CargoResult<(Command, TempPath)> { // The rust linker also jumps through similar hoops, although with a different // of response file, which this borrows from. Some references: // https://github.com/rust-lang/rust/blob/ef92009c1dbe2750f1d24a6619b827721fb49749/src/librustc_codegen_ssa/back/link.rs#L935 @@ -362,7 +342,7 @@ impl ProcessBuilder { } // cmd.exe can handle up to 8k work of args, this leaves some headroom if using a .cmd rustc wrapper. let mut cmd_remaining: usize = 1024 * 6; - let mut response_file = tempfile::NamedTempFile::new()?; + let mut response_file = NamedTempFile::new()?; for arg in &self.args { cmd_remaining = cmd_remaining.saturating_sub(arg.len()); if cmd_remaining > 0 { @@ -391,6 +371,37 @@ impl ProcessBuilder { } Ok((command, response_file.into_temp_path())) } + + fn status(&self) -> CargoResult { + let (mut child, _response_file) = self.spawn_with(|_cmd| {})?; + Ok(child.wait()?) + } + + fn output(&self) -> CargoResult { + let (child, _response_file) = self.spawn_with(|cmd| { + cmd.stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(Stdio::null()); + })?; + Ok(child.wait_with_output()?) + } + + fn spawn_with( + &self, + mut modify_command: impl FnMut(&mut Command), + ) -> CargoResult<(Child, Option)> { + let mut command = self.build_command(); + modify_command(&mut command); + match command.spawn() { + Ok(child) => Ok((child, None)), + Err(ref err) if imp::command_line_too_big(err) => { + let (mut command, response_file) = self.build_command_and_response_file()?; + modify_command(&mut command); + Ok((command.spawn()?, Some(response_file))) + } + Err(other) => Err(other.into()), + } + } } /// A helper function to create a `ProcessBuilder`.