Merge #806

806: improve error messages r=Alexhuszagh a=Emilgardis

this makes error messages a bit better by displaying stdout/stderr if it was captured

also improves command output in verbose mode or in errors

new:
<img width="715" alt="the new output on an error on verbose mode" src="https://user-images.githubusercontent.com/1502855/174091357-d9122066-cd1c-4a21-8392-2572ef93b734.png">

old:
<img width="577" alt="the old output on an error on verbose mode, has a some unnecessary double quoutes and doesn't tell what the actual reported error was/is" src="https://user-images.githubusercontent.com/1502855/174019703-7c5e413b-9f4b-487f-88e0-66a422a99302.png">


Co-authored-by: Emil Gardström <[email protected]>

Author: bors[bot]

Cargo.toml

@@ -19,7 +19,7 @@ members = ["xtask"]
 [dependencies]
 atty = "0.2"
 clap = { version = "3.2.2", features = ["derive", "unstable-v4"] }
-color-eyre = "0.6"
+color-eyre = { version = "0.6", default-features = false }
 eyre = "0.6"
 thiserror = "1"
 home = "0.5"

src/cargo.rs

@@ -111,7 +111,7 @@ pub fn cargo_metadata_with_args(
             .ok()
             .unwrap_or_else(|| "cargo".to_string()),
     );
-    command.arg("metadata").arg("--format-version=1");
+    command.arg("metadata").args(&["--format-version", "1"]);
     if let Some(cd) = cd {
         command.current_dir(cd);
     }
@@ -154,9 +154,6 @@ pub fn run(args: &[String], verbose: bool) -> Result<ExitStatus, CommandError> {
 }
 
 /// run cargo and get the output, does not check the exit status
-pub fn run_and_get_output(
-    args: &[String],
-    verbose: bool,
-) -> Result<std::process::Output, CommandError> {
+pub fn run_and_get_output(args: &[String], verbose: bool) -> Result<std::process::Output> {
     Command::new("cargo").args(args).run_and_get_output(verbose)
 }

src/docker/engine.rs

@@ -5,8 +5,8 @@ use std::process::Command;
 use crate::errors::*;
 use crate::extensions::CommandExt;
 
-const DOCKER: &str = "docker";
-const PODMAN: &str = "podman";
+pub const DOCKER: &str = "docker";
+pub const PODMAN: &str = "podman";
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 pub enum EngineType {

src/errors.rs

@@ -10,10 +10,35 @@ pub fn install_panic_hook() -> Result<()> {
 
 #[derive(Debug, thiserror::Error)]
 pub enum CommandError {
-    #[error("`{1}` failed with exit code: {0}")]
-    NonZeroExitCode(std::process::ExitStatus, String),
-    #[error("could not execute `{0}`")]
-    CouldNotExecute(#[source] Box<dyn std::error::Error + Send + Sync>, String),
+    #[error("`{command}` failed with {status}")]
+    NonZeroExitCode {
+        status: std::process::ExitStatus,
+        command: String,
+        stderr: Vec<u8>,
+        stdout: Vec<u8>,
+    },
+    #[error("could not execute `{command}`")]
+    CouldNotExecute {
+        #[source]
+        source: Box<dyn std::error::Error + Send + Sync>,
+        command: String,
+    },
     #[error("`{0:?}` output was not UTF-8")]
     Utf8Error(#[source] std::string::FromUtf8Error, std::process::Output),
 }
+
+impl CommandError {
+    /// Attach valuable information to this [`CommandError`](Self)
+    pub fn to_section_report(self) -> eyre::Report {
+        match &self {
+            CommandError::NonZeroExitCode { stderr, stdout, .. } => {
+                let stderr = String::from_utf8_lossy(stderr).trim().to_string();
+                let stdout = String::from_utf8_lossy(stdout).trim().to_string();
+                eyre::eyre!(self)
+                    .section(color_eyre::SectionExt::header(stderr, "Stderr:"))
+                    .section(color_eyre::SectionExt::header(stdout, "Stdout:"))
+            }
+            _ => eyre::eyre!(self),
+        }
+    }
+}

src/extensions.rs

@@ -1,45 +1,98 @@
 use std::borrow::Cow;
 use std::fmt;
-use std::process::{Command, ExitStatus};
+use std::process::{Command, ExitStatus, Output};
 
 use crate::errors::*;
 
+pub const STRIPPED_BINS: &[&str] = &[crate::docker::DOCKER, crate::docker::PODMAN, "cargo"];
+
 pub trait CommandExt {
     fn print_verbose(&self, verbose: bool);
-    fn status_result(&self, status: ExitStatus) -> Result<(), CommandError>;
+    fn status_result(
+        &self,
+        verbose: bool,
+        status: ExitStatus,
+        output: Option<&Output>,
+    ) -> Result<(), CommandError>;
     fn run(&mut self, verbose: bool, silence_stdout: bool) -> Result<(), CommandError>;
     fn run_and_get_status(
         &mut self,
         verbose: bool,
         silence_stdout: bool,
     ) -> Result<ExitStatus, CommandError>;
-    fn run_and_get_stdout(&mut self, verbose: bool) -> Result<String, CommandError>;
-    fn run_and_get_output(&mut self, verbose: bool) -> Result<std::process::Output, CommandError>;
+    fn run_and_get_stdout(&mut self, verbose: bool) -> Result<String>;
+    fn run_and_get_output(&mut self, verbose: bool) -> Result<std::process::Output>;
+    fn command_pretty(&self, verbose: bool, strip: impl for<'a> Fn(&'a str) -> bool) -> String;
 }
 
 impl CommandExt for Command {
+    fn command_pretty(&self, verbose: bool, strip: impl for<'a> Fn(&'a str) -> bool) -> String {
+        // a dummy implementor of display to avoid using unwraps
+        struct C<'c, F>(&'c Command, bool, F);
+        impl<F> std::fmt::Display for C<'_, F>
+        where
+            F: for<'a> Fn(&'a str) -> bool,
+        {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                let cmd = self.0;
+                write!(
+                    f,
+                    "{}",
+                    // if verbose, never strip, if not, let user choose
+                    crate::file::pretty_path(cmd.get_program(), move |c| if self.1 {
+                        false
+                    } else {
+                        self.2(c)
+                    })
+                )?;
+                let args = cmd.get_args();
+                if args.len() > 1 {
+                    write!(
+                        f,
+                        " {}",
+                        shell_words::join(args.map(|o| o.to_string_lossy()))
+                    )?;
+                }
+                Ok(())
+            }
+        }
+        format!("{}", C(self, verbose, strip))
+    }
+
     fn print_verbose(&self, verbose: bool) {
+        // TODO: introduce verbosity levels, v = 1, strip cmd, v > 1, don't strip cmd
         if verbose {
             if let Some(cwd) = self.get_current_dir() {
-                println!("+ {:?} {:?}", cwd, self);
+                println!("+ {:?} {}", cwd, self.command_pretty(true, |_| false));
             } else {
-                println!("+ {:?}", self);
+                println!("+ {}", self.command_pretty(true, |_| false));
             }
         }
     }
 
-    fn status_result(&self, status: ExitStatus) -> Result<(), CommandError> {
+    fn status_result(
+        &self,
+        verbose: bool,
+        status: ExitStatus,
+        output: Option<&Output>,
+    ) -> Result<(), CommandError> {
         if status.success() {
             Ok(())
         } else {
-            Err(CommandError::NonZeroExitCode(status, format!("{self:?}")))
+            Err(CommandError::NonZeroExitCode {
+                status,
+                command: self
+                    .command_pretty(verbose, |ref cmd| STRIPPED_BINS.iter().any(|f| f == cmd)),
+                stderr: output.map(|out| out.stderr.clone()).unwrap_or_default(),
+                stdout: output.map(|out| out.stdout.clone()).unwrap_or_default(),
+            })
         }
     }
 
     /// Runs the command to completion
     fn run(&mut self, verbose: bool, silence_stdout: bool) -> Result<(), CommandError> {
         let status = self.run_and_get_status(verbose, silence_stdout)?;
-        self.status_result(status)
+        self.status_result(verbose, status, None)
     }
 
     /// Runs the command to completion
@@ -53,37 +106,53 @@ impl CommandExt for Command {
             self.stdout(std::process::Stdio::null());
         }
         self.status()
-            .map_err(|e| CommandError::CouldNotExecute(Box::new(e), format!("{self:?}")))
+            .map_err(|e| CommandError::CouldNotExecute {
+                source: Box::new(e),
+                command: self
+                    .command_pretty(verbose, |ref cmd| STRIPPED_BINS.iter().any(|f| f == cmd)),
+            })
             .map_err(Into::into)
     }
 
     /// Runs the command to completion and returns its stdout
-    fn run_and_get_stdout(&mut self, verbose: bool) -> Result<String, CommandError> {
+    fn run_and_get_stdout(&mut self, verbose: bool) -> Result<String> {
         let out = self.run_and_get_output(verbose)?;
-        self.status_result(out.status)?;
-        out.stdout()
+        self.status_result(verbose, out.status, Some(&out))
+            .map_err(CommandError::to_section_report)?;
+        out.stdout().map_err(Into::into)
     }
 
     /// Runs the command to completion and returns the status and its [output](std::process::Output).
     ///
     /// # Notes
     ///
     /// This command does not check the status.
-    fn run_and_get_output(&mut self, verbose: bool) -> Result<std::process::Output, CommandError> {
+    fn run_and_get_output(&mut self, verbose: bool) -> Result<std::process::Output> {
         self.print_verbose(verbose);
-        self.output()
-            .map_err(|e| CommandError::CouldNotExecute(Box::new(e), format!("{self:?}")))
+        self.output().map_err(|e| {
+            CommandError::CouldNotExecute {
+                source: Box::new(e),
+                command: self
+                    .command_pretty(verbose, |ref cmd| STRIPPED_BINS.iter().any(|f| f == cmd)),
+            }
+            .to_section_report()
+        })
     }
 }
 
 pub trait OutputExt {
     fn stdout(&self) -> Result<String, CommandError>;
+    fn stderr(&self) -> Result<String, CommandError>;
 }
 
 impl OutputExt for std::process::Output {
     fn stdout(&self) -> Result<String, CommandError> {
         String::from_utf8(self.stdout.clone()).map_err(|e| CommandError::Utf8Error(e, self.clone()))
     }
+
+    fn stderr(&self) -> Result<String, CommandError> {
+        String::from_utf8(self.stderr.clone()).map_err(|e| CommandError::Utf8Error(e, self.clone()))
+    }
 }
 
 pub struct SafeCommand {

src/file.rs

@@ -1,3 +1,5 @@
+use std::borrow::Cow;
+use std::ffi::OsStr;
 use std::fs::{self, File};
 use std::io::Read;
 use std::path::{Path, PathBuf};
@@ -36,6 +38,55 @@ fn _canonicalize(path: &Path) -> Result<PathBuf> {
     }
 }
 
+/// Pretty format a file path. Also removes the path prefix from a command if wanted
+pub fn pretty_path(path: impl AsRef<Path>, strip: impl for<'a> Fn(&'a str) -> bool) -> String {
+    let path = path.as_ref();
+    // TODO: Use Path::file_prefix
+    let file_stem = path.file_stem();
+    let file_name = path.file_name();
+    let path = if let (Some(file_stem), Some(file_name)) = (file_stem, file_name) {
+        if let Some(file_name) = file_name.to_str() {
+            if strip(file_name) {
+                Cow::Borrowed(file_stem)
+            } else {
+                Cow::Borrowed(path.as_os_str())
+            }
+        } else {
+            Cow::Borrowed(path.as_os_str())
+        }
+    } else {
+        maybe_canonicalize(path)
+    };
+
+    if let Some(path) = path.to_str() {
+        shell_escape(path).to_string()
+    } else {
+        format!("{path:?}")
+    }
+}
+
+pub fn shell_escape(string: &str) -> Cow<'_, str> {
+    let escape: &[char] = if cfg!(target_os = "windows") {
+        &['%', '$', '`', '!', '"']
+    } else {
+        &['$', '\'', '\\', '!', '"']
+    };
+
+    if string.contains(escape) {
+        Cow::Owned(format!("{string:?}"))
+    } else if string.contains(' ') {
+        Cow::Owned(format!("\"{string}\""))
+    } else {
+        Cow::Borrowed(string)
+    }
+}
+
+pub fn maybe_canonicalize(path: &Path) -> Cow<'_, OsStr> {
+    canonicalize(path)
+        .map(|p| Cow::Owned(p.as_os_str().to_owned()))
+        .unwrap_or_else(|_| path.as_os_str().into())
+}
+
 pub fn write_file(path: impl AsRef<Path>, overwrite: bool) -> Result<File> {
     let path = path.as_ref();
     fs::create_dir_all(
@@ -57,3 +108,55 @@ pub fn write_file(path: impl AsRef<Path>, overwrite: bool) -> Result<File> {
     open.open(&path)
         .wrap_err(format!("couldn't write to file `{}`", path.display()))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    #[cfg(target_family = "windows")]
+    fn pretty_path_windows() {
+        assert_eq!(
+            pretty_path("C:\\path\\bin\\cargo.exe", |f| f.contains("cargo")),
+            "cargo".to_string()
+        );
+        assert_eq!(
+            pretty_path("C:\\Program Files\\Docker\\bin\\docker.exe", |_| false),
+            "\"C:\\Program Files\\Docker\\bin\\docker.exe\"".to_string()
+        );
+        assert_eq!(
+            pretty_path("C:\\Program Files\\single'quote\\cargo.exe", |c| c
+                .contains("cargo")),
+            "cargo".to_string()
+        );
+        assert_eq!(
+            pretty_path("C:\\Program Files\\single'quote\\cargo.exe", |_| false),
+            "\"C:\\Program Files\\single'quote\\cargo.exe\"".to_string()
+        );
+        assert_eq!(
+            pretty_path("C:\\Program Files\\%not_var%\\cargo.exe", |_| false),
+            "\"C:\\\\Program Files\\\\%not_var%\\\\cargo.exe\"".to_string()
+        );
+    }
+
+    #[test]
+    #[cfg(target_family = "unix")]
+    fn pretty_path_linux() {
+        assert_eq!(
+            pretty_path("/usr/bin/cargo", |f| f.contains("cargo")),
+            "cargo".to_string()
+        );
+        assert_eq!(
+            pretty_path("/home/user/my rust/bin/cargo", |_| false),
+            "\"/home/user/my rust/bin/cargo\"".to_string(),
+        );
+        assert_eq!(
+            pretty_path("/home/user/single'quote/cargo", |c| c.contains("cargo")),
+            "cargo".to_string()
+        );
+        assert_eq!(
+            pretty_path("/home/user/single'quote/cargo", |_| false),
+            "\"/home/user/single'quote/cargo\"".to_string()
+        );
+    }
+}

src/rustc.rs

@@ -40,7 +40,6 @@ pub fn target_list(verbose: bool) -> Result<TargetList> {
         .map(|s| TargetList {
             triples: s.lines().map(|l| l.to_owned()).collect(),
         })
-        .map_err(Into::into)
 }
 
 pub fn sysroot(host: &Host, target: &Target, verbose: bool) -> Result<PathBuf> {

src/rustup.rs

@@ -45,13 +45,18 @@ pub fn installed_toolchains(verbose: bool) -> Result<Vec<String>> {
 pub fn available_targets(toolchain: &str, verbose: bool) -> Result<AvailableTargets> {
     let mut cmd = Command::new("rustup");
     cmd.args(&["target", "list", "--toolchain", toolchain]);
-    let output = cmd.run_and_get_output(verbose)?;
+    let output = cmd
+        .run_and_get_output(verbose)
+        .suggestion("is rustup installed?")?;
 
     if !output.status.success() {
         if String::from_utf8_lossy(&output.stderr).contains("is a custom toolchain") {
             eyre::bail!("{toolchain} is a custom toolchain. To use it, you'll need to set the environment variable `CROSS_CUSTOM_TOOLCHAIN=1`")
         }
-        return Err(cmd.status_result(output.status).unwrap_err().into());
+        return Err(cmd
+            .status_result(verbose, output.status, Some(&output))
+            .unwrap_err()
+            .to_section_report());
     }
     let out = output.stdout()?;
     let mut default = String::new();