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

Simplify the error output on failed Command invocation #1397

Merged
merged 2 commits into from
Feb 14, 2025
Merged

Simplify the error output on failed Command invocation #1397

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c2816a
Simplify the error output on failed `Command` invocation
madsmtm Feb 10, 2025
c3c2e07
Display error before the extra
madsmtm Feb 14, 2025
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
76 changes: 14 additions & 62 deletions src/command_helpers.rs
View file
Open in desktop
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 {:?}{extra}: {e}", 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
Open in desktop
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