From 5fe27327c3c31e3c540f8c981ddd3cd0c1a1ce43 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 3 Nov 2022 15:08:30 +0000 Subject: [PATCH] Revert "Auto merge of #11183 - weihanglo:issue/10526, r=ehuss" This reverts commit d4c38af1202c8b24fce6b2ad170c520017e059b9, reversing changes made to 92d8826ed4b7dfd5dd60c75bcc86da4032b00899. --- src/cargo/core/compiler/unit_dependencies.rs | 20 ++++--- src/cargo/core/resolver/features.rs | 53 +++++++++--------- src/cargo/ops/tree/graph.rs | 6 +- tests/testsuite/artifact_dep.rs | 58 +------------------- 4 files changed, 43 insertions(+), 94 deletions(-) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index ab2c4331c..9319a19e0 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -1040,7 +1040,6 @@ impl<'a, 'cfg> State<'a, 'cfg> { } } - /// See [`ResolvedFeatures::activated_features`]. fn activated_features( &self, pkg_id: PackageId, @@ -1050,9 +1049,14 @@ impl<'a, 'cfg> State<'a, 'cfg> { features.activated_features(pkg_id, features_for) } - /// See [`ResolvedFeatures::is_activated`]. - fn is_activated(&self, pkg_id: PackageId, features_for: FeaturesFor) -> bool { - self.features().is_activated(pkg_id, features_for) + fn is_dep_activated( + &self, + pkg_id: PackageId, + features_for: FeaturesFor, + dep_name: InternedString, + ) -> bool { + self.features() + .is_dep_activated(pkg_id, features_for, dep_name) } fn get(&self, id: PackageId) -> &'a Package { @@ -1067,7 +1071,7 @@ impl<'a, 'cfg> State<'a, 'cfg> { let kind = unit.kind; self.resolve() .deps(pkg_id) - .filter_map(|(dep_id, deps)| { + .filter_map(|(id, deps)| { assert!(!deps.is_empty()); let deps: Vec<_> = deps .iter() @@ -1099,8 +1103,8 @@ impl<'a, 'cfg> State<'a, 'cfg> { // If this is an optional dependency, and the new feature resolver // did not enable it, don't include it. if dep.is_optional() { - let dep_features_for = unit_for.map_to_features_for(dep.artifact()); - if !self.is_activated(dep_id, dep_features_for) { + let features_for = unit_for.map_to_features_for(dep.artifact()); + if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) { return false; } } @@ -1113,7 +1117,7 @@ impl<'a, 'cfg> State<'a, 'cfg> { if deps.is_empty() { None } else { - Some((dep_id, deps)) + Some((id, deps)) } }) .collect() diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 9c8737656..a2dba2869 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -56,12 +56,11 @@ type ActivateMap = HashMap>; /// Set of all activated features for all packages in the resolve graph. pub struct ResolvedFeatures { - /// Map of features activated for each package. - /// - /// The presence of each key also means the package itself is activated, - /// even its associated set contains no features. activated_features: ActivateMap, - /// Options that change how the feature resolver operates. + /// Optional dependencies that should be built. + /// + /// The value is the `name_in_toml` of the dependencies. + activated_dependencies: ActivateMap, opts: FeatureOpts, } @@ -322,14 +321,21 @@ impl ResolvedFeatures { .expect("activated_features for invalid package") } - /// Asks the resolved features if the given package should be included. + /// Returns if the given dependency should be included. /// - /// One scenario to use this function is to deteremine a optional dependency - /// should be built or not. - pub fn is_activated(&self, pkg_id: PackageId, features_for: FeaturesFor) -> bool { - log::trace!("is_activated {} {features_for}", pkg_id.name()); - self.activated_features_unverified(pkg_id, features_for.apply_opts(&self.opts)) - .is_some() + /// This handles dependencies disabled via `cfg` expressions and optional + /// dependencies which are not enabled. + pub fn is_dep_activated( + &self, + pkg_id: PackageId, + features_for: FeaturesFor, + dep_name: InternedString, + ) -> bool { + let key = features_for.apply_opts(&self.opts); + self.activated_dependencies + .get(&(pkg_id, key)) + .map(|deps| deps.contains(&dep_name)) + .unwrap_or(false) } /// Variant of `activated_features` that returns `None` if this is @@ -409,14 +415,8 @@ pub struct FeatureResolver<'a, 'cfg> { /// Options that change how the feature resolver operates. opts: FeatureOpts, /// Map of features activated for each package. - /// - /// The presence of each key also means the package itself is activated, - /// even its associated set contains no features. activated_features: ActivateMap, /// Map of optional dependencies activated for each package. - /// - /// The key is the package having their dependencies activated. - /// The value comes from `dep_name` part of the feature syntax `dep:dep_name`. activated_dependencies: ActivateMap, /// Keeps track of which packages have had its dependencies processed. /// Used to avoid cycles, and to speed up processing. @@ -475,6 +475,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } Ok(ResolvedFeatures { activated_features: r.activated_features, + activated_dependencies: r.activated_dependencies, opts: r.opts, }) } @@ -506,10 +507,10 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { Ok(()) } - /// Activates a list of [`FeatureValue`] for a given package. + /// Activates [`FeatureValue`]s on the given package. /// - /// This is the main entrance into the recursion of feature activation for a package. - /// Other `activate_*` functions would be called inside this function accordingly. + /// This is the main entrance into the recursion of feature activation + /// for a package. fn activate_pkg( &mut self, pkg_id: PackageId, @@ -517,13 +518,9 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { fvs: &[FeatureValue], ) -> CargoResult<()> { log::trace!("activate_pkg {} {}", pkg_id.name(), fk); - // Cargo must insert an empty set here as the presence of an (empty) set - // also means that the dependency is activated. - // This `is_activated` behavior for dependencies was previously depends on field - // `activated_dependencies`, which is less useful after rust-lang/cargo#11183. - // - // That is, we may keep or remove `activated_dependencies` in the future - // if figuring out it can completely be replaced with `activated_features`. + // Add an empty entry to ensure everything is covered. This is intended for + // finding bugs where the resolver missed something it should have visited. + // Remove this in the future if `activated_features` uses an empty default. self.activated_features .entry((pkg_id, fk.apply_opts(&self.opts))) .or_insert_with(BTreeSet::new); diff --git a/src/cargo/ops/tree/graph.rs b/src/cargo/ops/tree/graph.rs index 46c56c9f6..20a9ca0b6 100644 --- a/src/cargo/ops/tree/graph.rs +++ b/src/cargo/ops/tree/graph.rs @@ -365,7 +365,11 @@ fn add_pkg( if dep.is_optional() { // If the new feature resolver does not enable this // optional dep, then don't use it. - if !resolved_features.is_activated(dep_id, features_for) { + if !resolved_features.is_dep_activated( + package_id, + features_for, + dep.name_in_toml(), + ) { return false; } } diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index 1ff8dafdc..6f41dd21a 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -20,7 +20,7 @@ fn check_with_invalid_artifact_dependency() { version = "0.0.0" authors = [] resolver = "2" - + [dependencies] bar = { path = "bar/", artifact = "unknown" } "#, @@ -2276,59 +2276,3 @@ fn build_script_features_for_shared_dependency() { .masquerade_as_nightly_cargo(&["bindeps"]) .run(); } - -#[cargo_test] -fn build_with_target_and_optional() { - // This is a incorrect behaviour got to be fixed. - // See rust-lang/cargo#10526 - if cross_compile::disabled() { - return; - } - let target = cross_compile::alternate(); - let p = project() - .file( - "Cargo.toml", - &r#" - [package] - name = "foo" - version = "0.0.1" - edition = "2021" - - [dependencies] - d1 = { path = "d1", artifact = "bin", optional = true, target = "$TARGET" } - "# - .replace("$TARGET", target), - ) - .file( - "src/main.rs", - r#" - fn main() { - let _b = include_bytes!(env!("CARGO_BIN_FILE_D1")); - } - "#, - ) - .file( - "d1/Cargo.toml", - r#" - [package] - name = "d1" - version = "0.0.1" - edition = "2021" - "#, - ) - .file("d1/src/main.rs", "fn main() {}") - .build(); - - p.cargo("build -Z bindeps -F d1 -v") - .masquerade_as_nightly_cargo(&["bindeps"]) - .with_stderr( - "\ -[COMPILING] d1 v0.0.1 [..] -[RUNNING] `rustc --crate-name d1 [..]--crate-type bin[..] -[COMPILING] foo v0.0.1 [..] -[RUNNING] `rustc --crate-name foo [..]--cfg[..]d1[..] -[FINISHED] dev [..] -", - ) - .run(); -}