From f46145312c827778482022ae7c550a98594c5dc4 Mon Sep 17 00:00:00 2001 From: Alik Aslanyan Date: Mon, 3 May 2021 13:06:34 +0400 Subject: [PATCH] Fix bug when with resolver = "1" non-virtual package was allowing unknown features --- Cargo.toml | 1 + src/cargo/core/workspace.rs | 69 +++++++++++++++++++++-------- tests/testsuite/package_features.rs | 46 ++++++++++++++----- 3 files changed, 88 insertions(+), 28 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index eaf8e8307..c7f17c030 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -67,6 +67,7 @@ clap = "2.31.2" unicode-width = "0.1.5" openssl = { version = '0.10.11', optional = true } im-rc = "15.0.0" +itertools = "0.10.0" # A noop dependency that changes in the Rust repository, it's a bit of a hack. # See the `src/tools/rustc-workspace-hack/README.md` file in `rust-lang/rust` diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 9a2d5920a..42e7dd0da 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -7,6 +7,7 @@ use std::slice; use anyhow::{bail, Context as _}; use glob::glob; +use itertools::Itertools; use log::debug; use url::Url; @@ -1071,7 +1072,7 @@ impl<'cfg> Workspace<'cfg> { if self.allows_new_cli_feature_behavior() { self.members_with_features_new(specs, cli_features) } else { - Ok(self.members_with_features_old(specs, cli_features)) + self.members_with_features_old(specs, cli_features) } } @@ -1196,7 +1197,7 @@ impl<'cfg> Workspace<'cfg> { &self, specs: &[PackageIdSpec], cli_features: &CliFeatures, - ) -> Vec<(&Package, CliFeatures)> { + ) -> CargoResult> { // Split off any features with the syntax `member-name/feature-name` into a map // so that those features can be applied directly to those workspace-members. let mut member_specific_features: HashMap> = @@ -1215,29 +1216,29 @@ impl<'cfg> Workspace<'cfg> { } => panic!("unexpected dep: syntax {}", feature), FeatureValue::DepFeature { dep_name, - dep_feature, + dep_feature: _, dep_prefix: _, weak: _, } => { - // I think weak can be ignored here. - // * With `--features member?/feat -p member`, the ? doesn't - // really mean anything (either the member is built or it isn't). - // * With `--features nonmember?/feat`, cwd_features will - // handle processing it correctly. + // Check if `dep_name` is member of the workspace or package. + // Weak can be ignored for this moment. let is_member = self.members().any(|member| member.name() == *dep_name); if is_member && specs.iter().any(|spec| spec.name() == *dep_name) { member_specific_features .entry(*dep_name) .or_default() - .insert(FeatureValue::Feature(*dep_feature)); + .insert(feature.clone()); } else { + // With `--features nonmember?/feat`, cwd_features will + // handle processing it correctly. cwd_features.insert(feature.clone()); } } } } - let ms = self.members().filter_map(|member| { + let mut result = Vec::new(); + for member in self.members() { let member_id = member.package_id(); match self.current_opt() { // The features passed on the command-line only apply to @@ -1248,13 +1249,29 @@ impl<'cfg> Workspace<'cfg> { all_features: cli_features.all_features, uses_default_features: cli_features.uses_default_features, }; - Some((member, feats)) + + // If any member specific features were passed to non-virtual package, it's error + if !member_specific_features.is_empty() { + let invalid: Vec<_> = member_specific_features + .values() + .map(|set| set.iter()) + .flatten() + .map(|feature| feature.to_string()) + .sorted() + .collect(); + + bail!( + "Member specific features with `pkg/feat` syntax are dissalowed outside of workspace with `resolver = \"1\", remove: {}", + invalid.join(", ") + ); + } + + result.push((member, feats)) } _ => { // Ignore members that are not enabled on the command-line. if specs.iter().any(|spec| spec.matches(member_id)) { - // -p for a workspace member that is not the "current" - // one. + // -p for a workspace member that is not the "current" one. // // The odd behavior here is due to backwards // compatibility. `--features` and @@ -1266,20 +1283,36 @@ impl<'cfg> Workspace<'cfg> { features: Rc::new( member_specific_features .remove(member.name().as_str()) - .unwrap_or_default(), + .unwrap_or_default() + .into_iter() + .map(|feature| match feature { + // I think weak can be ignored here. + // With `--features member?/feat -p member`, the ? doesn't + // really mean anything (either the member is built or it isn't). + FeatureValue::DepFeature { + dep_name: _, + dep_feature, + dep_prefix: false, + weak: _, + } => FeatureValue::new(dep_feature), + // Member specific features by definition contain only `FeatureValue::DepFeature` + _ => unreachable!(), + }) + .collect(), ), uses_default_features: true, all_features: cli_features.all_features, }; - Some((member, feats)) + + result.push((member, feats)) } else { // This member was not requested on the command-line, skip. - None } } } - }); - ms.collect() + } + + Ok(result) } } diff --git a/tests/testsuite/package_features.rs b/tests/testsuite/package_features.rs index dd9cd2127..bb2880cc4 100644 --- a/tests/testsuite/package_features.rs +++ b/tests/testsuite/package_features.rs @@ -215,7 +215,7 @@ fn other_member_from_current() { name = "bar" version = "0.1.0" - [features] + [features] f1 = [] f2 = [] f3 = [] @@ -273,8 +273,8 @@ fn other_member_from_current() { } #[cargo_test] -fn virtual_typo_member_feature_default_resolver() { - project() +fn feature_default_resolver() { + let p = project() .file( "Cargo.toml", r#" @@ -283,17 +283,37 @@ fn virtual_typo_member_feature_default_resolver() { version = "0.1.0" [features] - deny-warnings = [] + test = [] "#, ) - .file("src/lib.rs", "") - .build() - .cargo("check --features a/deny-warning") + .file( + "src/main.rs", + r#" + fn main() { + if cfg!(feature = "test") { + println!("feature set"); + } + } + "#, + ) + .build(); + + p.cargo("check --features testt") .masquerade_as_nightly_cargo() .with_status(101) - .with_stderr( - "[ERROR] none of the selected packages contains these features: a/deny-warning", - ) + .with_stderr("[ERROR] Package `a[..]` does not have the feature `testt`") + .run(); + + p.cargo("run --features test") + .masquerade_as_nightly_cargo() + .with_status(0) + .with_stdout("feature set") + .run(); + + p.cargo("run --features a/test") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr("[ERROR] Member specific features with `pkg/feat` syntax are dissalowed outside of workspace with `resolver = \"1\", remove: a/test") .run(); } @@ -483,6 +503,12 @@ fn resolver1_member_features() { .cwd("member2") .with_stdout("m1-feature set") .run(); + + p.cargo("check -p member1 --features member1/m2-feature") + .cwd("member2") + .with_status(101) + .with_stderr("[ERROR] Package `member1[..]` does not have the feature `m2-feature`") + .run(); } #[cargo_test] -- 2.39.5