Merge #68

68: Always print progress r=taiki-e a=taiki-e

Currently, progress is only printed when some options were specified (see #32), but I think it's better to always print it.

Before:
```console
$ cargo hack check --all
info: running `cargo check` on member1
...
info: running `cargo check` on member2
...
info: running `cargo check` on member3
...
```

After
```console
$ cargo hack check --all
info: running `cargo check` on member1 (1/3)
...
info: running `cargo check` on member2 (2/3)
...
info: running `cargo check` on member3 (3/3)
...
```

Co-authored-by: Taiki Endo <te316e89@gmail.com>

src/main.rs

@@ -26,7 +26,7 @@ use crate::{
     cli::{Args, Coloring},
     manifest::{find_root_manifest_for_wd, Manifest},
     metadata::Metadata,
-    package::{Kind, Package},
+    package::{Kind, Package, Progress},
     process::ProcessBuilder,
     restore::Restore,
 };
@@ -80,7 +80,7 @@ fn exec_on_workspace(
         line.arg(color.as_str());
     }
 
-    let mut total = 0;
+    let mut progress = Progress::default();
     let packages = if args.workspace {
         args.exclude.iter().for_each(|spec| {
             if !metadata.packages.iter().any(|package| package.name == *spec) {
@@ -95,7 +95,7 @@ fn exec_on_workspace(
 
         let packages =
             metadata.packages.iter().filter(|package| !args.exclude.contains(&&*package.name));
-        Package::from_iter(args, &mut total, packages)?
+        Package::from_iter(args, packages, &mut progress)?
     } else if !args.package.is_empty() {
         if let Some(spec) = args
             .package
@@ -107,32 +107,26 @@ fn exec_on_workspace(
 
         let packages =
             metadata.packages.iter().filter(|package| args.package.contains(&&*package.name));
-        Package::from_iter(args, &mut total, packages)?
+        Package::from_iter(args, packages, &mut progress)?
     } else if current_manifest.is_virtual() {
-        Package::from_iter(args, &mut total, &metadata.packages)?
+        Package::from_iter(args, &metadata.packages, &mut progress)?
     } else {
         let current_package = current_manifest.package_name();
         let package = metadata.packages.iter().find(|package| package.name == *current_package);
-        Package::from_iter(args, &mut total, package)?
+        Package::from_iter(args, package, &mut progress)?
     };
 
-    let mut info = Info { total, count: 0 };
     packages
         .iter()
-        .try_for_each(|package| exec_on_package(args, package, &line, &restore, &mut info))
-}
-
-struct Info {
-    total: usize,
-    count: usize,
+        .try_for_each(|package| exec_on_package(args, package, &line, &restore, &mut progress))
 }
 
 fn exec_on_package(
     args: &Args<'_>,
     package: &Package<'_>,
     line: &ProcessBuilder<'_>,
     restore: &Restore,
-    info: &mut Info,
+    progress: &mut Progress,
 ) -> Result<()> {
     if let Kind::SkipAsPrivate = package.kind {
         info!(args.color, "skipped running on private crate {}", package.name_verbose(args));
@@ -144,7 +138,7 @@ fn exec_on_package(
         line.arg("--manifest-path");
         line.arg(&package.manifest_path);
 
-        no_dev_deps(args, package, &mut line, restore, info)
+        no_dev_deps(args, package, &mut line, restore, progress)
     }
 }
 
@@ -153,7 +147,7 @@ fn no_dev_deps(
     package: &Package<'_>,
     line: &mut ProcessBuilder<'_>,
     restore: &Restore,
-    info: &mut Info,
+    progress: &mut Progress,
 ) -> Result<()> {
     if args.no_dev_deps || args.remove_dev_deps {
         let new = package.manifest.remove_dev_deps();
@@ -163,11 +157,11 @@ fn no_dev_deps(
             format!("failed to update manifest file: {}", package.manifest_path.display())
         })?;
 
-        package::exec(args, package, line, info)?;
+        package::exec(args, package, line, progress)?;
 
         handle.done()
     } else {
-        package::exec(args, package, line, info)
+        package::exec(args, package, line, progress)
     }
 }
 

src/package.rs

@@ -2,34 +2,44 @@ use std::{ffi::OsStr, fmt::Write, ops::Deref};
 
 use crate::{
     metadata::{self, Dependency},
-    Args, Info, Manifest, ProcessBuilder, Result,
+    Args, Manifest, ProcessBuilder, Result,
 };
 
+#[derive(Default)]
+pub(crate) struct Progress {
+    total: usize,
+    count: usize,
+}
+
 pub(crate) struct Package<'a> {
     package: &'a metadata::Package,
     pub(crate) manifest: Manifest,
     pub(crate) kind: Kind<'a>,
 }
 
 impl<'a> Package<'a> {
-    fn new(args: &'a Args<'_>, total: &mut usize, package: &'a metadata::Package) -> Result<Self> {
+    fn new(
+        args: &'a Args<'_>,
+        package: &'a metadata::Package,
+        progress: &mut Progress,
+    ) -> Result<Self> {
         let manifest = Manifest::new(&package.manifest_path)?;
 
         if args.ignore_private && manifest.is_private() {
             Ok(Self { package, manifest, kind: Kind::SkipAsPrivate })
         } else {
-            Ok(Self { package, manifest, kind: Kind::determine(args, package, total) })
+            Ok(Self { package, manifest, kind: Kind::determine(args, package, progress) })
         }
     }
 
     pub(crate) fn from_iter(
         args: &'a Args<'_>,
-        total: &mut usize,
         packages: impl IntoIterator<Item = &'a metadata::Package>,
+        progress: &mut Progress,
     ) -> Result<Vec<Self>> {
         packages
             .into_iter()
-            .map(|package| Package::new(args, total, package))
+            .map(|package| Package::new(args, package, progress))
             .collect::<Result<Vec<_>>>()
     }
 }
@@ -46,20 +56,24 @@ pub(crate) enum Kind<'a> {
     // If there is no subcommand, then kind need not be determined.
     NoSubcommand,
     SkipAsPrivate,
-    Nomal { show_progress: bool },
+    Nomal,
     Each { features: Vec<&'a str> },
     Powerset { features: Vec<Vec<&'a str>> },
 }
 
 impl<'a> Kind<'a> {
-    fn determine(args: &'a Args<'_>, package: &'a metadata::Package, total: &mut usize) -> Self {
+    fn determine(
+        args: &'a Args<'_>,
+        package: &'a metadata::Package,
+        progress: &mut Progress,
+    ) -> Self {
         if args.subcommand.is_none() {
             return Kind::NoSubcommand;
         }
 
         if !args.each_feature && !args.feature_powerset {
-            *total += 1;
-            return Kind::Nomal { show_progress: false };
+            progress.total += 1;
+            return Kind::Nomal;
         }
 
         let features = if args.include_features.is_empty() {
@@ -92,10 +106,10 @@ impl<'a> Kind<'a> {
             if (package.features.is_empty() || !args.include_features.is_empty())
                 && features.is_empty()
             {
-                *total += 1;
-                Kind::Nomal { show_progress: true }
+                progress.total += 1;
+                Kind::Nomal
             } else {
-                *total += features.len()
+                progress.total += features.len()
                     + (!args.exclude_features.contains(&"default")) as usize
                     + (!args.exclude_no_default_features) as usize
                     + (!args.exclude_all_features) as usize;
@@ -107,11 +121,11 @@ impl<'a> Kind<'a> {
             if (package.features.is_empty() || !args.include_features.is_empty())
                 && features.is_empty()
             {
-                *total += 1;
-                Kind::Nomal { show_progress: true }
+                progress.total += 1;
+                Kind::Nomal
             } else {
                 // -1: the first element of a powerset is `[]`
-                *total += features.len() - 1
+                progress.total += features.len() - 1
                     + (!args.exclude_features.contains(&"default")) as usize
                     + (!args.exclude_no_default_features) as usize
                     + (!args.exclude_all_features) as usize;
@@ -127,14 +141,14 @@ pub(crate) fn exec(
     args: &Args<'_>,
     package: &Package<'_>,
     line: &mut ProcessBuilder<'_>,
-    info: &mut Info,
+    progress: &mut Progress,
 ) -> Result<()> {
     match &package.kind {
         Kind::NoSubcommand => return Ok(()),
         Kind::SkipAsPrivate => unreachable!(),
-        Kind::Nomal { show_progress } => {
+        Kind::Nomal => {
             // only run with default features
-            return exec_cargo(args, package, line, info, *show_progress);
+            return exec_cargo(args, package, line, progress);
         }
         Kind::Each { .. } | Kind::Powerset { .. } => {}
     }
@@ -143,7 +157,7 @@ pub(crate) fn exec(
 
     if !args.exclude_features.contains(&"default") {
         // run with default features
-        exec_cargo(args, package, &mut line, info, true)?;
+        exec_cargo(args, package, &mut line, progress)?;
     }
 
     if !args.no_default_features {
@@ -155,29 +169,29 @@ pub(crate) fn exec(
         //
         // `default` is not skipped because `cfg(feature = "default")` is work
         // if `default` feature specified.
-        exec_cargo(args, package, &mut line, info, true)?;
+        exec_cargo(args, package, &mut line, progress)?;
     }
 
     match &package.kind {
         Kind::Each { features } => {
-            features
-                .iter()
-                .try_for_each(|f| exec_cargo_with_features(args, package, &line, info, Some(f)))?;
+            features.iter().try_for_each(|f| {
+                exec_cargo_with_features(args, package, &line, progress, Some(f))
+            })?;
         }
         Kind::Powerset { features } => {
             // The first element of a powerset is `[]` so it should be skipped.
             features
                 .iter()
                 .skip(1)
-                .try_for_each(|f| exec_cargo_with_features(args, package, &line, info, f))?;
+                .try_for_each(|f| exec_cargo_with_features(args, package, &line, progress, f))?;
         }
         _ => unreachable!(),
     }
 
     if !args.exclude_all_features {
         // run with all features
         line.arg("--all-features");
-        exec_cargo(args, package, &mut line, info, true)?;
+        exec_cargo(args, package, &mut line, progress)?;
     }
 
     Ok(())
@@ -187,22 +201,21 @@ fn exec_cargo_with_features(
     args: &Args<'_>,
     package: &Package<'_>,
     line: &ProcessBuilder<'_>,
-    info: &mut Info,
+    progress: &mut Progress,
     features: impl IntoIterator<Item = impl AsRef<str>>,
 ) -> Result<()> {
     let mut line = line.clone();
     line.append_features(features);
-    exec_cargo(args, package, &mut line, info, true)
+    exec_cargo(args, package, &mut line, progress)
 }
 
 fn exec_cargo(
     args: &Args<'_>,
     package: &Package<'_>,
     line: &mut ProcessBuilder<'_>,
-    info: &mut Info,
-    show_progress: bool,
+    progress: &mut Progress,
 ) -> Result<()> {
-    info.count += 1;
+    progress.count += 1;
 
     if args.clean_per_run {
         cargo_clean(line.get_program(), args, package)?;
@@ -215,9 +228,7 @@ fn exec_cargo(
     } else {
         write!(msg, "running {} on {}", line, &package.name).unwrap();
     }
-    if show_progress {
-        write!(msg, " ({}/{})", info.count, info.total).unwrap();
-    }
+    write!(msg, " ({}/{})", progress.count, progress.total).unwrap();
     info!(args.color, "{}", msg);
 
     line.exec()

tests/test.rs

@@ -186,10 +186,10 @@ fn real_manifest() {
         .output()
         .unwrap()
         .assert_success()
-        .assert_stderr_contains("running `cargo check` on member1")
-        .assert_stderr_contains("running `cargo check` on member2")
-        .assert_stderr_contains("running `cargo check` on member3")
-        .assert_stderr_contains("running `cargo check` on real");
+        .assert_stderr_contains("running `cargo check` on member1 (1/4)")
+        .assert_stderr_contains("running `cargo check` on member2 (2/4)")
+        .assert_stderr_contains("running `cargo check` on member3 (3/4)")
+        .assert_stderr_contains("running `cargo check` on real (4/4)");
 }
 
 #[test]
@@ -200,17 +200,17 @@ fn virtual_manifest() {
         .output()
         .unwrap()
         .assert_success()
-        .assert_stderr_contains("running `cargo check` on member1")
-        .assert_stderr_contains("running `cargo check` on member2");
+        .assert_stderr_contains("running `cargo check` on member1 (1/3)")
+        .assert_stderr_contains("running `cargo check` on member2 (2/3)");
 
     cargo_hack()
         .args(&["check", "--all"])
         .current_dir(test_dir("tests/fixtures/virtual"))
         .output()
         .unwrap()
         .assert_success()
-        .assert_stderr_contains("running `cargo check` on member1")
-        .assert_stderr_contains("running `cargo check` on member2");
+        .assert_stderr_contains("running `cargo check` on member1 (1/3)")
+        .assert_stderr_contains("running `cargo check` on member2 (2/3)");
 }
 
 #[test]