Repository: taiki-e/cargo-hack (language: rust)

Issue:
Should print the command actually executed when error occurred
Currently, error is printed like the following:

```console
$ cargo hack check -p futures
info: running `cargo check` on futures
...
error: could not compile `futures`

To learn more, run the command again with --verbose.
error: process didn't exit successfully: `cargo check` (exit code: 101)
```

```text
error: process didn't exit successfully: `cargo check` (exit code: 101)
```
However,  the command actually was executed is `cargo check --manifest-path=futures/Cargo.toml`, not `cargo check`



Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 12bfa9f5..648a6e72 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,10 @@ This project adheres to [Semantic Versioning](https://semver.org).
 
 ## [Unreleased]
 
+* cargo-hack now prints the command actually executed when error occurred.
+
+* `--verbose` flag is no longer propagated to cargo.
+
 * Improve compile time by removing some dependencies.
 
 ## [0.3.12] - 2020-09-18
diff --git a/src/cli.rs b/src/cli.rs
index a5d09f17..f45191e8 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -59,7 +59,7 @@ const HELP: &[(&str, &str, &str, &[&str])] = &[
         "If used this flag with --workspace, --each-feature, or --feature-powerset, artifacts will be removed before each run.",
         "Note that dependencies artifacts will be preserved.",
     ]),
-    ("-v", "--verbose", "Use verbose output", &["This flag will be propagated to cargo."]),
+    ("-v", "--verbose", "Use verbose output", &[]),
     ("", "--color <WHEN>", "Coloring: auto, always, never", &[
         "This flag will be propagated to cargo.",
     ]),
@@ -188,14 +188,14 @@ pub(crate) struct Args {
     pub(crate) skip_no_default_features: bool,
     /// --clean-per-run
     pub(crate) clean_per_run: bool,
+    /// -v, --verbose, -vv
+    pub(crate) verbose: bool,
 
     // flags that will be propagated to cargo
     /// --features <FEATURES>...
     pub(crate) features: Vec<String>,
     /// --color <WHEN>
     pub(crate) color: Option<Coloring>,
-    /// -v, --verbose, -vv
-    pub(crate) verbose: bool,
 }
 
 #[derive(Clone, Copy)]
@@ -269,6 +269,7 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
     let mut ignore_non_exist_features = false;
     let mut skip_no_default_features = false;
     let mut clean_per_run = false;
+    let mut verbose = false;
 
     let res = (|| -> Result<()> {
         while let Some(arg) = args.next() {
@@ -394,7 +395,7 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
                 );
             }
 
-            match arg.as_str() {
+            match &*arg {
                 "--workspace" | "--all" => {
                     if let Some(arg) = workspace.replace(arg) {
                         return Err(multi_arg(&arg, subcommand.as_ref()));
@@ -419,6 +420,8 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
                     }
                     ignore_non_exist_features = true;
                 }
+                // allow multiple uses
+                "--verbose" | "-v" | "-vv" => verbose = true,
                 _ => leading.push(arg),
             }
         }
@@ -428,7 +431,6 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
 
     let color = color.map(|c| c.parse()).transpose()?;
     *coloring = color;
-    let verbose = leading.iter().any(|a| a == "--verbose" || a == "-v" || a == "-vv");
 
     res?;
 
@@ -471,7 +473,7 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
             }
         }
     }
-    if let Some(pos) = leading.iter().position(|a| match a.as_str() {
+    if let Some(pos) = leading.iter().position(|a| match &**a {
         "--example" | "--examples" | "--test" | "--tests" | "--bench" | "--benches"
         | "--all-targets" => true,
         _ => a.starts_with("--example=") || a.starts_with("--test=") || a.starts_with("--bench="),
diff --git a/src/main.rs b/src/main.rs
index e5e2835a..30aa1e8c 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -131,14 +131,14 @@ fn exec_on_package(
         line.arg("--manifest-path");
         line.arg(&package.manifest_path);
 
-        no_dev_deps(args, package, &line, restore, info)
+        no_dev_deps(args, package, &mut line, restore, info)
     }
 }
 
 fn no_dev_deps(
     args: &Args,
     package: &Package<'_>,
-    line: &ProcessBuilder<'_>,
+    line: &mut ProcessBuilder<'_>,
     restore: &Restore,
     info: &mut Info,
 ) -> Result<()> {
diff --git a/src/package.rs b/src/package.rs
index 733c3107..86f07dd6 100644
--- a/src/package.rs
+++ b/src/package.rs
@@ -120,7 +120,7 @@ impl<'a> Kind<'a> {
 pub(crate) fn exec(
     args: &Args,
     package: &Package<'_>,
-    line: &ProcessBuilder<'_>,
+    line: &mut ProcessBuilder<'_>,
     info: &mut Info,
 ) -> Result<()> {
     match &package.kind {
@@ -137,7 +137,7 @@ pub(crate) fn exec(
 
     if !args.skip.iter().any(|x| x == "default") {
         // run with default features
-        exec_cargo(args, package, &line, info, true)?;
+        exec_cargo(args, package, &mut line, info, true)?;
     }
 
     line.arg("--no-default-features");
@@ -147,7 +147,7 @@ pub(crate) fn exec(
         //
         // `default` is not skipped because `cfg(feature = "default")` is work
         // if `default` feature specified.
-        exec_cargo(args, package, &line, info, true)?;
+        exec_cargo(args, package, &mut line, info, true)?;
     }
 
     match &package.kind {
@@ -174,13 +174,13 @@ fn exec_cargo_with_features(
 ) -> Result<()> {
     let mut line = line.clone();
     line.append_features(features);
-    exec_cargo(args, package, &line, info, true)
+    exec_cargo(args, package, &mut line, info, true)
 }
 
 fn exec_cargo(
     args: &Args,
     package: &Package<'_>,
-    line: &ProcessBuilder<'_>,
+    line: &mut ProcessBuilder<'_>,
     info: &mut Info,
     show_progress: bool,
 ) -> Result<()> {
@@ -190,9 +190,13 @@ fn exec_cargo(
         cargo_clean(line.get_program(), args, package)?;
     }
 
-    // running `<command>` on <package> (<count>/<total>)
+    // running `<command>` (on <package>) (<count>/<total>)
     let mut msg = String::new();
-    write!(msg, "running {} on {}", line, &package.name).unwrap();
+    if args.verbose {
+        write!(msg, "running {}", line).unwrap();
+    } else {
+        write!(msg, "running {} on {}", line, &package.name).unwrap();
+    }
     if show_progress {
         write!(msg, " ({}/{})", info.count, info.total).unwrap();
     }
diff --git a/src/process.rs b/src/process.rs
index 0bc838b5..880f1193 100644
--- a/src/process.rs
+++ b/src/process.rs
@@ -1,5 +1,6 @@
 use anyhow::Context;
 use std::{
+    env,
     ffi::{OsStr, OsString},
     fmt,
     path::Path,
@@ -24,6 +25,8 @@ pub(crate) struct ProcessBuilder<'a> {
 
     /// A list of arguments to pass to the program (between `leading_args` and '--').
     args: Vec<OsString>,
+    /// A comma-separated list of features.
+    /// This list always has a trailing comma if it is not empty.
     // cargo less than Rust 1.38 cannot handle multiple '--features' flags, so it creates another String.
     features: String,
 
@@ -112,16 +115,24 @@ impl<'a> ProcessBuilder<'a> {
         &self.program
     }
 
+    /// Gets the comma-separated features list
+    fn get_features(&self) -> &str {
+        // drop a trailing comma if it is not empty.
+        &self.features[..self.features.len().saturating_sub(1)]
+    }
+
     /// Runs the process, waiting for completion, and mapping non-success exit codes to an error.
-    pub(crate) fn exec(&self) -> Result<()> {
+    pub(crate) fn exec(&mut self) -> Result<()> {
         let mut command = self.build_command();
         let exit = command.status().with_context(|| {
+            self.verbose = true;
             ProcessError::new(&format!("could not execute process {}", self), None, None)
         })?;
 
         if exit.success() {
             Ok(())
         } else {
+            self.verbose = true;
             Err(ProcessError::new(
                 &format!("process didn't exit successfully: {}", self),
                 Some(exit),
@@ -160,7 +171,7 @@ impl<'a> ProcessBuilder<'a> {
         command.args(&self.args);
         if !self.features.is_empty() {
             command.arg("--features");
-            command.arg(&self.features[..self.features.len() - 1]); // drop tail `,`
+            command.arg(self.get_features());
         }
         if !self.trailing_args.is_empty() {
             command.arg("--");
@@ -181,24 +192,27 @@ impl fmt::Display for ProcessBuilder<'_> {
             write!(f, " {}", arg)?;
         }
 
-        if self.verbose {
-            for arg in &self.args {
-                write!(f, " {}", arg.to_string_lossy())?;
-            }
-        } else {
-            let mut args = self.args.iter();
-            while let Some(arg) = args.next() {
+        let mut args = self.args.iter();
+        while let Some(arg) = args.next() {
+            if arg == "--manifest-path" {
+                let path = Path::new(args.next().unwrap());
                 // Displaying `--manifest-path` is redundant.
-                if arg == "--manifest-path" {
-                    let _ = args.next();
-                    continue;
+                if self.verbose {
+                    let path = env::current_dir()
+                        .ok()
+                        .and_then(|cwd| path.strip_prefix(&cwd).ok())
+                        .unwrap_or(path);
+
+                    write!(f, " --manifest-path")?;
+                    write!(f, " {}", path.display())?;
                 }
+            } else {
                 write!(f, " {}", arg.to_string_lossy())?;
             }
         }
 
         if !self.features.is_empty() {
-            write!(f, " --features {}", &self.features[..self.features.len() - 1])?;
+            write!(f, " --features {}", self.get_features())?;
         }
 
         if !self.trailing_args.is_empty() {


Test patch:
diff --git a/tests/long-help.txt b/tests/long-help.txt
index c8d0a3de..19db9a88 100644
--- a/tests/long-help.txt
+++ b/tests/long-help.txt
@@ -75,8 +75,6 @@ OPTIONS:
     -v, --verbose
             Use verbose output.
 
-            This flag will be propagated to cargo.
-
         --color <WHEN>
             Coloring: auto, always, never.
 
diff --git a/tests/test.rs b/tests/test.rs
index a48ab106..18f33b3c 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -7,6 +7,11 @@ use std::{
     process::{Command, Output},
 };
 
+#[cfg(not(windows))]
+const SEPARATOR: char = '/';
+#[cfg(windows)]
+const SEPARATOR: char = '\\';
+
 fn cargo_hack() -> Command {
     // TODO: update to use CARGO_BIN_EXE (https://github.com/rust-lang/cargo/pull/7697, require Rust 1.43).
     let mut current = env::current_exe().unwrap();
@@ -957,3 +962,25 @@ fn list_separator() {
         .assert_success()
         .assert_stderr_contains("running `cargo run --features real,renemed` on optional_deps");
 }
+
+#[test]
+fn verbose() {
+    cargo_hack()
+        .args(&["check", "--verbose"])
+        .current_dir(test_dir("tests/fixtures/virtual"))
+        .output()
+        .unwrap()
+        .assert_success()
+        .assert_stderr_contains(&format!(
+            "running `cargo check --manifest-path member1{0}Cargo.toml`",
+            SEPARATOR
+        ))
+        .assert_stderr_contains(&format!(
+            "running `cargo check --manifest-path member2{0}Cargo.toml`",
+            SEPARATOR
+        ))
+        .assert_stderr_contains(&format!(
+            "running `cargo check --manifest-path dir{0}not_find_manifest{0}Cargo.toml`",
+            SEPARATOR
+        ));
+}