Skip to content

Commit

Permalink
Simplify the error output on failed Command invocation (#1397)
Browse files Browse the repository at this point in the history
  • Loading branch information
@madsmtm
madsmtm authored Feb 14, 2025
1 parent 328d0ff commit fcf940e
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 129 deletions.
76 changes: 14 additions & 62 deletions src/command_helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ fn write_warning(line: &[u8]) {

fn wait_on_child(
cmd: &Command,
program: &Path,
child: &mut Child,
cargo_output: &CargoOutput,
) -> Result<(), Error> {
Expand All @@ -258,12 +257,7 @@ fn wait_on_child(
Err(e) => {
return Err(Error::new(
ErrorKind::ToolExecError,
format!(
"Failed to wait on spawned child process, command {:?} with args {}: {}.",
cmd,
program.display(),
e
),
format!("failed to wait on spawned child process `{cmd:?}`: {e}"),
));
}
};
Expand All @@ -275,12 +269,7 @@ fn wait_on_child(
} else {
Err(Error::new(
ErrorKind::ToolExecError,
format!(
"Command {:?} with args {} did not execute successfully (status code {}).",
cmd,
program.display(),
status
),
format!("command did not execute successfully (status code {status}): {cmd:?}"),
))
}
}
Expand Down Expand Up @@ -350,28 +339,16 @@ pub(crate) fn objects_from_files(files: &[Arc<Path>], dst: &Path) -> Result<Vec<
Ok(objects)
}

pub(crate) fn run(
cmd: &mut Command,
program: impl AsRef<Path>,
cargo_output: &CargoOutput,
) -> Result<(), Error> {
let program = program.as_ref();

let mut child = spawn(cmd, program, cargo_output)?;
wait_on_child(cmd, program, &mut child, cargo_output)
pub(crate) fn run(cmd: &mut Command, cargo_output: &CargoOutput) -> Result<(), Error> {
let mut child = spawn(cmd, cargo_output)?;
wait_on_child(cmd, &mut child, cargo_output)
}

pub(crate) fn run_output(
cmd: &mut Command,
program: impl AsRef<Path>,
cargo_output: &CargoOutput,
) -> Result<Vec<u8>, Error> {
let program = program.as_ref();

pub(crate) fn run_output(cmd: &mut Command, cargo_output: &CargoOutput) -> Result<Vec<u8>, Error> {
// We specifically need the output to be captured, so override default
let mut captured_cargo_output = cargo_output.clone();
captured_cargo_output.output = OutputKind::Capture;
let mut child = spawn(cmd, program, &captured_cargo_output)?;
let mut child = spawn(cmd, &captured_cargo_output)?;

let mut stdout = vec![];
child
Expand All @@ -382,16 +359,12 @@ pub(crate) fn run_output(
.unwrap();

// Don't care about this output, use the normal settings
wait_on_child(cmd, program, &mut child, cargo_output)?;
wait_on_child(cmd, &mut child, cargo_output)?;

Ok(stdout)
}

pub(crate) fn spawn(
cmd: &mut Command,
program: &Path,
cargo_output: &CargoOutput,
) -> Result<Child, Error> {
pub(crate) fn spawn(cmd: &mut Command, cargo_output: &CargoOutput) -> Result<Child, Error> {
struct ResetStderr<'cmd>(&'cmd mut Command);

impl Drop for ResetStderr<'_> {
Expand All @@ -414,28 +387,18 @@ pub(crate) fn spawn(
Ok(child) => Ok(child),
Err(ref e) if e.kind() == io::ErrorKind::NotFound => {
let extra = if cfg!(windows) {
" (see https://docs.rs/cc/latest/cc/#compile-time-requirements \
for help)"
" (see https://docs.rs/cc/latest/cc/#compile-time-requirements for help)"
} else {
""
};
Err(Error::new(
ErrorKind::ToolNotFound,
format!(
"Failed to find tool. Is `{}` installed?{}",
program.display(),
extra
),
format!("failed to find tool {:?}: {e}{extra}", cmd.0.get_program()),
))
}
Err(e) => Err(Error::new(
ErrorKind::ToolExecError,
format!(
"Command {:?} with args {} failed to start: {:?}",
cmd.0,
program.display(),
e
),
format!("command `{:?}` failed to start: {e}", cmd.0),
)),
}
}
Expand Down Expand Up @@ -465,7 +428,6 @@ pub(crate) fn command_add_output_file(cmd: &mut Command, dst: &Path, args: CmdAd
#[cfg(feature = "parallel")]
pub(crate) fn try_wait_on_child(
cmd: &Command,
program: &Path,
child: &mut Child,
stdout: &mut dyn io::Write,
stderr_forwarder: &mut StderrForwarder,
Expand All @@ -483,12 +445,7 @@ pub(crate) fn try_wait_on_child(
} else {
Err(Error::new(
ErrorKind::ToolExecError,
format!(
"Command {:?} with args {} did not execute successfully (status code {}).",
cmd,
program.display(),
status
),
format!("command did not execute successfully (status code {status}): {cmd:?}"),
))
}
}
Expand All @@ -497,12 +454,7 @@ pub(crate) fn try_wait_on_child(
stderr_forwarder.forward_all();
Err(Error::new(
ErrorKind::ToolExecError,
format!(
"Failed to wait on spawned child process, command {:?} with args {}: {}.",
cmd,
program.display(),
e
),
format!("failed to wait on spawned child process `{cmd:?}`: {e}"),
))
}
}
Expand Down
88 changes: 25 additions & 63 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1594,8 +1594,8 @@ impl Build {

if objs.len() <= 1 {
for obj in objs {
let (mut cmd, name) = self.create_compile_object_cmd(obj)?;
run(&mut cmd, &name, &self.cargo_output)?;
let mut cmd = self.create_compile_object_cmd(obj)?;
run(&mut cmd, &self.cargo_output)?;
}

return Ok(());
Expand Down Expand Up @@ -1623,12 +1623,8 @@ impl Build {
// acquire the appropriate tokens, Once all objects have been compiled
// we wait on all the processes and propagate the results of compilation.

let pendings = Cell::new(Vec::<(
Command,
Cow<'static, Path>,
KillOnDrop,
parallel::job_token::JobToken,
)>::new());
let pendings =
Cell::new(Vec::<(Command, KillOnDrop, parallel::job_token::JobToken)>::new());
let is_disconnected = Cell::new(false);
let has_made_progress = Cell::new(false);

Expand All @@ -1649,14 +1645,8 @@ impl Build {

cell_update(&pendings, |mut pendings| {
// Try waiting on them.
pendings.retain_mut(|(cmd, program, child, _token)| {
match try_wait_on_child(
cmd,
program,
&mut child.0,
&mut stdout,
&mut child.1,
) {
pendings.retain_mut(|(cmd, child, _token)| {
match try_wait_on_child(cmd, &mut child.0, &mut stdout, &mut child.1) {
Ok(Some(())) => {
// Task done, remove the entry
has_made_progress.set(true);
Expand Down Expand Up @@ -1695,14 +1685,14 @@ impl Build {
};
let spawn_future = async {
for obj in objs {
let (mut cmd, program) = self.create_compile_object_cmd(obj)?;
let mut cmd = self.create_compile_object_cmd(obj)?;
let token = tokens.acquire().await?;
let mut child = spawn(&mut cmd, &program, &self.cargo_output)?;
let mut child = spawn(&mut cmd, &self.cargo_output)?;
let mut stderr_forwarder = StderrForwarder::new(&mut child);
stderr_forwarder.set_non_blocking()?;

cell_update(&pendings, |mut pendings| {
pendings.push((cmd, program, KillOnDrop(child, stderr_forwarder), token));
pendings.push((cmd, KillOnDrop(child, stderr_forwarder), token));
pendings
});

Expand Down Expand Up @@ -1741,17 +1731,14 @@ impl Build {
check_disabled()?;

for obj in objs {
let (mut cmd, name) = self.create_compile_object_cmd(obj)?;
run(&mut cmd, &name, &self.cargo_output)?;
let mut cmd = self.create_compile_object_cmd(obj)?;
run(&mut cmd, &self.cargo_output)?;
}

Ok(())
}

fn create_compile_object_cmd(
&self,
obj: &Object,
) -> Result<(Command, Cow<'static, Path>), Error> {
fn create_compile_object_cmd(&self, obj: &Object) -> Result<Command, Error> {
let asm_ext = AsmFileExt::from_path(&obj.src);
let is_asm = asm_ext.is_some();
let target = self.get_target()?;
Expand All @@ -1761,23 +1748,14 @@ impl Build {
let gnu = compiler.family == ToolFamily::Gnu;

let is_assembler_msvc = msvc && asm_ext == Some(AsmFileExt::DotAsm);
let (mut cmd, name) = if is_assembler_msvc {
let (cmd, name) = self.msvc_macro_assembler()?;
(cmd, Cow::Borrowed(Path::new(name)))
let mut cmd = if is_assembler_msvc {
self.msvc_macro_assembler()?
} else {
let mut cmd = compiler.to_command();
for (a, b) in self.env.iter() {
cmd.env(a, b);
}
(
cmd,
compiler
.path
.file_name()
.ok_or_else(|| Error::new(ErrorKind::IOError, "Failed to get compiler path."))
.map(PathBuf::from)
.map(Cow::Owned)?,
)
cmd
};
let is_arm = matches!(target.arch, "aarch64" | "arm");
command_add_output_file(
Expand Down Expand Up @@ -1817,7 +1795,7 @@ impl Build {
self.fix_env_for_apple_os(&mut cmd)?;
}

Ok((cmd, name))
Ok(cmd)
}

/// This will return a result instead of panicking; see [`Self::expand()`] for
Expand Down Expand Up @@ -1852,12 +1830,7 @@ impl Build {

cmd.args(self.files.iter().map(std::ops::Deref::deref));

let name = compiler
.path
.file_name()
.ok_or_else(|| Error::new(ErrorKind::IOError, "Failed to get compiler path."))?;

run_output(&mut cmd, name, &self.cargo_output)
run_output(&mut cmd, &self.cargo_output)
}

/// Run the compiler, returning the macro-expanded version of the input files.
Expand Down Expand Up @@ -2476,7 +2449,7 @@ impl Build {
flags_env_var_value.is_ok()
}

fn msvc_macro_assembler(&self) -> Result<(Command, &'static str), Error> {
fn msvc_macro_assembler(&self) -> Result<Command, Error> {
let target = self.get_target()?;
let tool = if target.arch == "x86_64" {
"ml64.exe"
Expand Down Expand Up @@ -2531,7 +2504,7 @@ impl Build {
cmd.arg("-safeseh");
}

Ok((cmd, tool))
Ok(cmd)
}

fn assemble(&self, lib_name: &str, dst: &Path, objs: &[Object]) -> Result<(), Error> {
Expand Down Expand Up @@ -2559,7 +2532,7 @@ impl Build {
let dlink = out_dir.join(lib_name.to_owned() + "_dlink.o");
let mut nvcc = self.get_compiler().to_command();
nvcc.arg("--device-link").arg("-o").arg(&dlink).arg(dst);
run(&mut nvcc, "nvcc", &self.cargo_output)?;
run(&mut nvcc, &self.cargo_output)?;
self.assemble_progressive(dst, &[dlink.as_path()])?;
}

Expand Down Expand Up @@ -2587,12 +2560,12 @@ impl Build {
// Non-msvc targets (those using `ar`) need a separate step to add
// the symbol table to archives since our construction command of
// `cq` doesn't add it for us.
let (mut ar, cmd, _any_flags) = self.get_ar()?;
let mut ar = self.try_get_archiver()?;

// NOTE: We add `s` even if flags were passed using $ARFLAGS/ar_flag, because `s`
// here represents a _mode_, not an arbitrary flag. Further discussion of this choice
// can be seen in https://github.com/rust-lang/cc-rs/pull/763.
run(ar.arg("s").arg(dst), &cmd, &self.cargo_output)?;
run(ar.arg("s").arg(dst), &self.cargo_output)?;
}

Ok(())
Expand All @@ -2601,7 +2574,7 @@ impl Build {
fn assemble_progressive(&self, dst: &Path, objs: &[&Path]) -> Result<(), Error> {
let target = self.get_target()?;

let (mut cmd, program, any_flags) = self.get_ar()?;
let (mut cmd, program, any_flags) = self.try_get_archiver_and_flags()?;
if target.env == "msvc" && !program.to_string_lossy().contains("llvm-ar") {
// NOTE: -out: here is an I/O flag, and so must be included even if $ARFLAGS/ar_flag is
// in use. -nologo on the other hand is just a regular flag, and one that we'll skip if
Expand All @@ -2619,7 +2592,7 @@ impl Build {
cmd.arg(dst);
}
cmd.args(objs);
run(&mut cmd, &program, &self.cargo_output)?;
run(&mut cmd, &self.cargo_output)?;
} else {
// Set an environment variable to tell the OSX archiver to ensure
// that all dates listed in the archive are zero, improving
Expand Down Expand Up @@ -2648,11 +2621,7 @@ impl Build {
// NOTE: We add cq here regardless of whether $ARFLAGS/ar_flag have been used because
// it dictates the _mode_ ar runs in, which the setter of $ARFLAGS/ar_flag can't
// dictate. See https://github.com/rust-lang/cc-rs/pull/763 for further discussion.
run(
cmd.arg("cq").arg(dst).args(objs),
&program,
&self.cargo_output,
)?;
run(cmd.arg("cq").arg(dst).args(objs), &self.cargo_output)?;
}

Ok(())
Expand Down Expand Up @@ -3114,10 +3083,6 @@ impl Build {
}
}

fn get_ar(&self) -> Result<(Command, PathBuf, bool), Error> {
self.try_get_archiver_and_flags()
}

/// Get the archiver (ar) that's in use for this configuration.
///
/// You can use [`Command::get_program`] to get just the path to the command.
Expand Down Expand Up @@ -3764,7 +3729,6 @@ impl Build {
.arg("--show-sdk-path")
.arg("--sdk")
.arg(sdk),
"xcrun",
&self.cargo_output,
)?;

Expand Down Expand Up @@ -3821,7 +3785,6 @@ impl Build {
.arg("--show-sdk-version")
.arg("--sdk")
.arg(sdk),
"xcrun",
&self.cargo_output,
)
.ok()?;
Expand Down Expand Up @@ -3992,7 +3955,6 @@ impl Build {
) -> Option<PathBuf> {
let search_dirs = run_output(
cc.arg("-print-search-dirs"),
"cc",
// this doesn't concern the compilation so we always want to show warnings.
cargo_output,
)
Expand Down
Loading

0 comments on commit fcf940e

Please sign in to comment.