Simplify the error output on failed Command invocation (#1397)

Author: madsmtm

src/command_helpers.rs

@@ -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> {
@@ -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}"),
             ));
         }
     };
@@ -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:?}"),
         ))
     }
 }
@@ -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
@@ -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<'_> {
@@ -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),
         )),
     }
 }
@@ -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,
@@ -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:?}"),
                 ))
             }
         }
@@ -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}"),
             ))
         }
     }

src/lib.rs

@@ -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(());
@@ -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);
 
@@ -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);
@@ -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
                 });
 
@@ -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()?;
@@ -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(
@@ -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
@@ -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.
@@ -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"
@@ -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> {
@@ -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()])?;
         }
 
@@ -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(())
@@ -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
@@ -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
@@ -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(())
@@ -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.
@@ -3764,7 +3729,6 @@ impl Build {
                 .arg("--show-sdk-path")
                 .arg("--sdk")
                 .arg(sdk),
-            "xcrun",
             &self.cargo_output,
         )?;
 
@@ -3821,7 +3785,6 @@ impl Build {
                     .arg("--show-sdk-version")
                     .arg("--sdk")
                     .arg(sdk),
-                "xcrun",
                 &self.cargo_output,
             )
             .ok()?;
@@ -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,
         )