Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use response files for rustc #7762

Closed
wants to merge 8 commits into from
Closed

Use response files for rustc #7762

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9161b3a
Use response files when invoking rustc
MaulingMonkey Jan 1, 2020
a0ed380
Prefix response file paths with '@'
MaulingMonkey Jan 2, 2020
7869ec0
Only use a response file for rustc.
MaulingMonkey Jan 2, 2020
44c9d10
rustfmt
MaulingMonkey Jan 2, 2020
60437cb
cargo +nightly fmt --all --bribe-ci
MaulingMonkey Jan 2, 2020
b9a341a
Only use response files when exceeding CLI limits.
MaulingMonkey Jan 7, 2020
563fdf8
process_builder style cleanup
MaulingMonkey Jan 8, 2020
f3ff04b
Pull process_builder retry logic into status/output/spawn_with
MaulingMonkey Jan 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 88 additions & 14 deletions src/cargo/util/process_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ 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};
use std::process::{Command, ExitStatus, Output, Stdio};

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};

/// A builder object for an external process, similar to `std::process::Command`.
#[derive(Clone, Debug)]
Expand Down Expand Up @@ -151,10 +152,13 @@ impl ProcessBuilder {

/// 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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this dance perhaps be folded into an internal spawn() method? That way this wouldn't have to be duplicated across exec, exec_with_output, and exec_with_streaming

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, the pattern of try-then-if-failed-try-again didn't actually work out for rustc on Windows so I think we'll want to tweak the logic to roughly match what rustc does, unconditionally using a response file for "very likely to fail" requests on Windows.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this dance perhaps be folded into an internal spawn() method? That way this wouldn't have to be duplicated across exec, exec_with_output, and exec_with_streaming

f3ff04b folds what can be folded, but it's actually a net increase to the amount of code. Each of those fns uses slightly different entry points (status, output, spawn) and slightly different piping as a result.

The new status/output fns only have a single call site each, but at least they make the error chaining easier...? I'd love to have a better alternative to spawn_with's callback, but Stdio doesn't seem to be clonable...

Additionally, the pattern of try-then-if-failed-try-again didn't actually work out for rustc on Windows so I think we'll want to tweak the logic to roughly match what rustc does, unconditionally using a response file for "very likely to fail" requests on Windows.

It's been working in my testing, but I suppose the concern is a RUSTC_WRAPPER.cmd ?

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(())
Expand Down Expand Up @@ -189,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<Output> {
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)
Expand Down Expand Up @@ -231,8 +237,20 @@ impl ProcessBuilder {
.stdin(Stdio::null());

let mut callback_error = None;
let status = (|| {
let mut child = cmd.spawn()?;
let status = (|| -> CargoResult<ExitStatus> {
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| {
Expand Down Expand Up @@ -273,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 {
Expand Down Expand Up @@ -329,6 +347,53 @@ impl ProcessBuilder {
}
command
}

/// 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);
}
// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to avoid this heuristic and simply pass everything through a response file once we've failed once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A RUSTC_WRAPPER is likely to fail to understand response files. This:

some_rustc_wrapper path/to/rustc.exe --flag --flag --flag ... @remaining_args

Is likely to work, while this:

some_rustc_wrapper @all_args_including_path_to_rustc_exe

Will almost certainly break. Perhaps that's fine - or even preferred? Loudly breaking instead of silently failing to recognize/modify flags found in @remaining_args ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er yes that's a good point, could the arguments to rustc be handled a bit differently in that case so only rustc's arguments get into the @-file?

This may be a bit too low-level of a location to put the insertion of the @-file

let mut response_file = tempfile::NamedTempFile::new()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this place the temporary file in the target folder so we don't spray too many temporary files throughout /tmp if ctrl-c is used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this may require moving this logic of try-with-response-file to somewhere that's more rustc-specific rather than in general for all processes spawned by Cargo)

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!(
MaulingMonkey marked this conversation as resolved.
Show resolved Hide resolved
"argument {:?} contains invalid unicode",
arg
)));
}
}
let mut arg = OsString::from("@");
arg.push(response_file.path());
command.arg(arg);
for (k, v) in &self.env {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely duplicated (as well as the jobserver bit below) with a different method in this struct, can those be deduplicated?

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()))
}
}

/// A helper function to create a `ProcessBuilder`.
Expand Down Expand Up @@ -360,6 +425,10 @@ mod imp {
))
.into())
}

pub fn command_line_too_big(err: &std::io::Error) -> bool {
err.raw_os_error() == Some(::libc::E2BIG)
MaulingMonkey marked this conversation as resolved.
Show resolved Hide resolved
}
}

#[cfg(windows)]
Expand All @@ -384,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;
MaulingMonkey marked this conversation as resolved.
Show resolved Hide resolved
err.raw_os_error() == Some(ERROR_FILENAME_EXCED_RANGE)
}
}