-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
9161b3a
Use response files when invoking rustc
MaulingMonkey a0ed380
Prefix response file paths with '@'
MaulingMonkey 7869ec0
Only use a response file for rustc.
MaulingMonkey 44c9d10
rustfmt
MaulingMonkey 60437cb
cargo +nightly fmt --all --bribe-ci
MaulingMonkey b9a341a
Only use response files when exceeding CLI limits.
MaulingMonkey 563fdf8
process_builder style cleanup
MaulingMonkey f3ff04b
Pull process_builder retry logic into status/output/spawn_with
MaulingMonkey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,14 @@ 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::{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}; | ||
|
||
|
@@ -151,8 +153,7 @@ 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(|| { | ||
let exit = self.status().chain_err(|| { | ||
process_error(&format!("could not execute process {}", self), None, None) | ||
})?; | ||
|
||
|
@@ -189,9 +190,7 @@ 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(|| { | ||
let output = self.output().chain_err(|| { | ||
process_error(&format!("could not execute process {}", self), None, None) | ||
})?; | ||
|
||
|
@@ -225,14 +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 = (|| { | ||
let mut child = cmd.spawn()?; | ||
let status = (|| -> CargoResult<ExitStatus> { | ||
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| { | ||
|
@@ -273,7 +271,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 { | ||
|
@@ -329,6 +327,81 @@ 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, 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; | ||
let mut response_file = 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 { | ||
failure::bail!("argument {:?} contains invalid unicode", arg); | ||
} | ||
} | ||
let mut arg = OsString::from("@"); | ||
arg.push(response_file.path()); | ||
command.arg(arg); | ||
for (k, v) in &self.env { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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())) | ||
} | ||
|
||
fn status(&self) -> CargoResult<ExitStatus> { | ||
let (mut child, _response_file) = self.spawn_with(|_cmd| {})?; | ||
Ok(child.wait()?) | ||
} | ||
|
||
fn output(&self) -> CargoResult<Output> { | ||
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<TempPath>)> { | ||
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`. | ||
|
@@ -360,13 +433,18 @@ mod imp { | |
)) | ||
.into()) | ||
} | ||
|
||
pub fn command_line_too_big(err: &std::io::Error) -> bool { | ||
err.raw_os_error() == Some(libc::E2BIG) | ||
} | ||
} | ||
|
||
#[cfg(windows)] | ||
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 { | ||
|
@@ -384,4 +462,8 @@ mod imp { | |
// Just execute the process as normal. | ||
process_builder.exec() | ||
} | ||
|
||
pub fn command_line_too_big(err: &std::io::Error) -> bool { | ||
err.raw_os_error() == Some(ERROR_FILENAME_EXCED_RANGE as i32) | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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