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

Issue:
Exclude features specified by --features flag from feature combinations
Features specified by --features flag is always passed to cargo.

https://github.com/taiki-e/cargo-hack/blob/45cafc0134b2b1034765b8acc548d3d2fa66124a/tests/test.rs#L533

Pull request description:
Exclude features specified by --features flag from feature combinations
Fixes #60

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 147ec32..802b393 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,8 @@ This project adheres to [Semantic Versioning](https://semver.org).
 
 * Add `--skip-all-features` flag. See [#42][42] for details.
 
+* Fix an issue where using `--features` with `--each-feature` or `--feature-powerset` together would result in the same feature combination being performed multiple times.
+
 [42]: https://github.com/taiki-e/cargo-hack/pull/42
 
 ## [0.3.14] - 2020-10-10
diff --git a/src/cli.rs b/src/cli.rs
index ff4da67..9e92e70 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -182,8 +182,6 @@ pub(crate) struct Args {
     pub(crate) each_feature: bool,
     /// --feature-powerset
     pub(crate) feature_powerset: bool,
-    /// --skip <FEATURES>...
-    pub(crate) skip: Vec<String>,
     /// --no-dev-deps
     pub(crate) no_dev_deps: bool,
     /// --remove-dev-deps
@@ -194,17 +192,25 @@ pub(crate) struct Args {
     pub(crate) ignore_unknown_features: bool,
     /// --optional-deps [DEPS]...
     pub(crate) optional_deps: Option<Vec<String>>,
-    /// --skip-no-default-features
-    pub(crate) skip_no_default_features: bool,
-    /// --skip-all-features
-    pub(crate) skip_all_features: bool,
     /// --clean-per-run
     pub(crate) clean_per_run: bool,
-    /// -v, --verbose, -vv
-    pub(crate) verbose: bool,
     /// --depth <NUM>
     pub(crate) depth: Option<usize>,
 
+    /// --no-default-features
+    pub(crate) no_default_features: bool,
+    /// -v, --verbose, -vv
+    pub(crate) verbose: bool,
+
+    // Note: These values are not always exactly the same as the input.
+    // Error messages should not assume that these options have been specified.
+    /// --skip <FEATURES>...
+    pub(crate) skip: Vec<String>,
+    /// --skip-no-default-features
+    pub(crate) skip_no_default_features: bool,
+    /// --skip-all-features
+    pub(crate) skip_all_features: bool,
+
     // flags that will be propagated to cargo
     /// --features <FEATURES>...
     pub(crate) features: Vec<String>,
@@ -283,9 +289,12 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
     let mut skip_no_default_features = false;
     let mut skip_all_features = false;
     let mut clean_per_run = false;
-    let mut verbose = false;
     let mut depth = None;
 
+    let mut verbose = false;
+    let mut no_default_features = false;
+    let mut all_features = false;
+
     let res = (|| -> Result<()> {
         while let Some(arg) = args.next() {
             // stop at `--`
@@ -306,20 +315,14 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
             }
 
             macro_rules! parse_opt {
-                ($opt:ident, $propagate:expr, $pat:expr, $help:expr) => {
+                ($opt:ident, $pat:expr, $help:expr) => {
                     if arg == $pat {
                         if $opt.is_some() {
                             return Err(multi_arg($help, subcommand.as_ref()));
                         }
                         let next =
                             args.next().ok_or_else(|| req_arg($help, subcommand.as_ref()))?;
-                        if $propagate {
-                            $opt = Some(next.clone());
-                            leading.push(arg);
-                            leading.push(next);
-                        } else {
-                            $opt = Some(next);
-                        }
+                        $opt = Some(next);
                         continue;
                     } else if arg.starts_with(concat!($pat, "=")) {
                         if $opt.is_some() {
@@ -330,9 +333,6 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
                             .nth(1)
                             .ok_or_else(|| req_arg($help, subcommand.as_ref()))?;
                         $opt = Some(next.to_string());
-                        if $propagate {
-                            leading.push(arg);
-                        }
                         continue;
                     }
                 };
@@ -383,13 +383,15 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
                 ($flag:ident) => {
                     if mem::replace(&mut $flag, true) {
                         return Err(multi_arg(&arg, subcommand.as_ref()));
+                    } else {
+                        continue;
                     }
                 };
             }
 
-            parse_opt!(manifest_path, false, "--manifest-path", "--manifest-path <PATH>");
-            parse_opt!(depth, false, "--depth", "--depth <NUM>");
-            parse_opt!(color, true, "--color", "--color <WHEN>");
+            parse_opt!(manifest_path, "--manifest-path", "--manifest-path <PATH>");
+            parse_opt!(depth, "--depth", "--depth <NUM>");
+            parse_opt!(color, "--color", "--color <WHEN>");
 
             parse_multi_opt!(package, false, true, "--package", "--package <SPEC>...");
             parse_multi_opt!(package, false, true, "-p", "--package <SPEC>...");
@@ -416,6 +418,7 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
                     if let Some(arg) = workspace.replace(arg) {
                         return Err(multi_arg(&arg, subcommand.as_ref()));
                     }
+                    continue;
                 }
                 "--no-dev-deps" => parse_flag!(no_dev_deps),
                 "--remove-dev-deps" => parse_flag!(remove_dev_deps),
@@ -430,9 +433,16 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
                     "--ignore-non-exist-features was removed, use --ignore-unknown-features instead"
                 ),
                 // allow multiple uses
-                "--verbose" | "-v" | "-vv" => verbose = true,
-                _ => leading.push(arg),
+                "--verbose" | "-v" | "-vv" => {
+                    verbose = true;
+                    continue;
+                }
+                "--no-default-features" => no_default_features = true,
+                "--all-features" => all_features = true,
+                _ => {}
             }
+
+            leading.push(arg);
         }
 
         Ok(())
@@ -517,6 +527,12 @@ pub(crate) fn args(coloring: &mut Option<Coloring>) -> Result<Option<Args>> {
     if each_feature && feature_powerset {
         bail!("--each-feature may not be used together with --feature-powerset");
     }
+    if all_features && each_feature {
+        bail!("--all-features may not be used together with --each-feature");
+    }
+    if all_features && feature_powerset {
+        bail!("--all-features may not be used together with --feature-powerset");
+    }
 
     if subcommand.is_none() {
         if leading.iter().any(|a| a == "--list") {
@@ -546,6 +562,9 @@ For more information try --help
         )
     }
 
+    skip_no_default_features |= no_default_features;
+    skip.extend_from_slice(&features);
+
     Ok(Some(Args {
         leading_args: leading,
         trailing_args: args.collect::<Vec<_>>(),
@@ -558,20 +577,23 @@ For more information try --help
         workspace: workspace.is_some(),
         each_feature,
         feature_powerset,
-        skip,
         no_dev_deps,
         remove_dev_deps,
         ignore_private,
         ignore_unknown_features,
         optional_deps,
-        skip_no_default_features,
-        skip_all_features,
         clean_per_run,
         depth,
 
+        no_default_features,
+        verbose,
+
+        skip,
+        skip_no_default_features,
+        skip_all_features,
+
         features,
         color,
-        verbose,
     }))
 }
 
diff --git a/src/main.rs b/src/main.rs
index 67ce0c7..da2dab8 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -134,7 +134,7 @@ fn exec_on_package(
         Ok(())
     } else {
         let mut line = line.clone();
-        line.features(args, package);
+        line.append_features_from_args(args, package);
 
         line.arg("--manifest-path");
         line.arg(&package.manifest_path);
diff --git a/src/package.rs b/src/package.rs
index 3fc31a2..c74a6a0 100644
--- a/src/package.rs
+++ b/src/package.rs
@@ -134,7 +134,9 @@ pub(crate) fn exec(
         exec_cargo(args, package, &mut line, info, true)?;
     }
 
-    line.arg("--no-default-features");
+    if !args.no_default_features {
+        line.arg("--no-default-features");
+    }
 
     if !args.skip_no_default_features {
         // run with no default features if the package has other features
diff --git a/src/process.rs b/src/process.rs
index 880f119..2dfbc11 100644
--- a/src/process.rs
+++ b/src/process.rs
@@ -66,8 +66,7 @@ impl<'a> ProcessBuilder<'a> {
         }
     }
 
-    /// (chainable) Adds `--features` flag to the args list.
-    pub(crate) fn features(&mut self, args: &Args, package: &Package) -> &mut Self {
+    pub(crate) fn append_features_from_args(&mut self, args: &Args, package: &Package) {
         if args.ignore_unknown_features {
             self.append_features(args.features.iter().filter(|f| {
                 if package.features.get(*f).is_some()
@@ -86,7 +85,6 @@ impl<'a> ProcessBuilder<'a> {
         } else if !args.features.is_empty() {
             self.append_features(&args.features);
         }
-        self
     }
 
     /// (chainable) Adds `arg` to the args list.


Test patch:
diff --git a/tests/test.rs b/tests/test.rs
index 0a112b0..bf492e1 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -527,27 +527,25 @@ fn each_feature() {
 
     // with other feature
     cargo_hack()
-        .args(&["check", "--each-feature", "--features=a"])
+        .args(&["check", "--each-feature", "--features", "a"])
         .current_dir(test_dir("tests/fixtures/real"))
         .output()
         .unwrap()
         .assert_success()
-        .assert_stderr_contains("running `cargo check --features a` on real (1/6)")
+        .assert_stderr_contains("running `cargo check --features a` on real (1/5)")
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a` on real (2/6)",
+            "running `cargo check --no-default-features --features a` on real (2/5)",
         )
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,a` on real (3/6)",
+            "running `cargo check --no-default-features --features a,b` on real (3/5)",
         )
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,b` on real (4/6)",
+            "running `cargo check --no-default-features --features a,c` on real (4/5)",
         )
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,c` on real (5/6)",
+            "running `cargo check --no-default-features --all-features --features a` on real (5/5)",
         )
-        .assert_stderr_contains(
-            "running `cargo check --no-default-features --all-features --features a` on real (6/6)",
-        );
+        .assert_stderr_not_contains("--features a,a");
 }
 
 #[test]
@@ -587,37 +585,28 @@ fn feature_powerset() {
 
     // with other feature
     cargo_hack()
-        .args(&["check", "--feature-powerset", "--features=a"])
+        .args(&["check", "--feature-powerset", "--features", "a"])
         .current_dir(test_dir("tests/fixtures/real"))
         .output()
         .unwrap()
         .assert_success()
-        .assert_stderr_contains("running `cargo check --features a` on real (1/10)")
-        .assert_stderr_contains("running `cargo check --no-default-features --features a` on real (2/10)")
-        .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,a` on real (3/10)",
-        )
-        .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,b` on real (4/10)",
-        )
+        .assert_stderr_contains("running `cargo check --features a` on real (1/6)")
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,c` on real (6/10)",
+            "running `cargo check --no-default-features --features a` on real (2/6)",
         )
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,a,b` on real (5/10)",
+            "running `cargo check --no-default-features --features a,b` on real (3/6)",
         )
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,a,c` on real (7/10)",
+            "running `cargo check --no-default-features --features a,c` on real (4/6)",
         )
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,b,c` on real (8/10)",
+            "running `cargo check --no-default-features --features a,b,c` on real (5/6)",
         )
         .assert_stderr_contains(
-            "running `cargo check --no-default-features --features a,a,b,c` on real (9/10)",
+            "running `cargo check --no-default-features --all-features --features a` on real (6/6)",
         )
-        .assert_stderr_contains(
-            "running `cargo check --no-default-features --all-features --features a` on real (10/10)",
-        );
+        .assert_stderr_not_contains("--features a,a");
 }
 
 #[test]
@@ -1165,3 +1154,47 @@ fn verbose() {
             SEPARATOR
         ));
 }
+
+#[test]
+fn propagate() {
+    cargo_hack()
+        .args(&["check", "--features", "a"])
+        .current_dir(test_dir("tests/fixtures/real"))
+        .output()
+        .unwrap()
+        .assert_success()
+        .assert_stderr_contains("--features a");
+
+    cargo_hack()
+        .args(&["check", "--no-default-features"])
+        .current_dir(test_dir("tests/fixtures/real"))
+        .output()
+        .unwrap()
+        .assert_success()
+        .assert_stderr_contains("--no-default-features");
+
+    cargo_hack()
+        .args(&["check", "--all-features"])
+        .current_dir(test_dir("tests/fixtures/real"))
+        .output()
+        .unwrap()
+        .assert_success()
+        .assert_stderr_contains("--all-features");
+
+    cargo_hack()
+        .args(&["check", "--color", "auto"])
+        .current_dir(test_dir("tests/fixtures/real"))
+        .output()
+        .unwrap()
+        .assert_success()
+        .assert_stderr_contains("`cargo check --color auto`");
+
+    // --verbose does not be propagated
+    cargo_hack()
+        .args(&["check", "--verbose"])
+        .current_dir(test_dir("tests/fixtures/real"))
+        .output()
+        .unwrap()
+        .assert_success()
+        .assert_stderr_not_contains("--verbose");
+}