From dee6e9a0ea062f88441be1101bf9c42ec31d341f Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sat, 17 Oct 2020 07:56:44 +0900 Subject: [PATCH] Exclude features specified by --features flag from feature combinations --- CHANGELOG.md | 2 ++ src/cli.rs | 80 ++++++++++++++++++++++++++++++----------------- src/main.rs | 2 +- src/package.rs | 4 ++- src/process.rs | 4 +-- tests/test.rs | 85 +++++++++++++++++++++++++++++++++++--------------- 6 files changed, 117 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 147ec320..802b3932 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 ff4da679..9e92e708 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 ... - pub(crate) skip: Vec, /// --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>, - /// --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 pub(crate) depth: Option, + /// --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 ... + pub(crate) skip: Vec, + /// --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 ... pub(crate) features: Vec, @@ -283,9 +289,12 @@ pub(crate) fn args(coloring: &mut Option) -> Result> { 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) -> Result> { } 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) -> Result> { .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) -> Result> { ($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 "); - parse_opt!(depth, false, "--depth", "--depth "); - parse_opt!(color, true, "--color", "--color "); + parse_opt!(manifest_path, "--manifest-path", "--manifest-path "); + parse_opt!(depth, "--depth", "--depth "); + parse_opt!(color, "--color", "--color "); parse_multi_opt!(package, false, true, "--package", "--package ..."); parse_multi_opt!(package, false, true, "-p", "--package ..."); @@ -416,6 +418,7 @@ pub(crate) fn args(coloring: &mut Option) -> Result> { 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) -> Result> { "--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) -> Result> { 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::>(), @@ -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 67ce0c71..da2dab8e 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 3fc31a2c..c74a6a0d 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 880f1193..2dfbc11d 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. diff --git a/tests/test.rs b/tests/test.rs index 0a112b09..bf492e15 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"); +}