diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 9319a19e038..ab2c4331cff 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -1040,6 +1040,7 @@ impl<'a, 'cfg> State<'a, 'cfg> { } } + /// See [`ResolvedFeatures::activated_features`]. fn activated_features( &self, pkg_id: PackageId, @@ -1049,14 +1050,9 @@ impl<'a, 'cfg> State<'a, 'cfg> { features.activated_features(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) + /// See [`ResolvedFeatures::is_activated`]. + fn is_activated(&self, pkg_id: PackageId, features_for: FeaturesFor) -> bool { + self.features().is_activated(pkg_id, features_for) } fn get(&self, id: PackageId) -> &'a Package { @@ -1071,7 +1067,7 @@ impl<'a, 'cfg> State<'a, 'cfg> { let kind = unit.kind; self.resolve() .deps(pkg_id) - .filter_map(|(id, deps)| { + .filter_map(|(dep_id, deps)| { assert!(!deps.is_empty()); let deps: Vec<_> = deps .iter() @@ -1103,8 +1099,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 features_for = unit_for.map_to_features_for(dep.artifact()); - if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) { + let dep_features_for = unit_for.map_to_features_for(dep.artifact()); + if !self.is_activated(dep_id, dep_features_for) { return false; } } @@ -1117,7 +1113,7 @@ impl<'a, 'cfg> State<'a, 'cfg> { if deps.is_empty() { None } else { - Some((id, deps)) + Some((dep_id, deps)) } }) .collect() diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index a2dba2869cf..9c873765612 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -56,11 +56,12 @@ type ActivateMap = HashMap>; /// Set of all activated features for all packages in the resolve graph. pub struct ResolvedFeatures { - activated_features: ActivateMap, - /// Optional dependencies that should be built. + /// Map of features activated for each package. /// - /// The value is the `name_in_toml` of the dependencies. - activated_dependencies: ActivateMap, + /// 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. opts: FeatureOpts, } @@ -321,21 +322,14 @@ impl ResolvedFeatures { .expect("activated_features for invalid package") } - /// Returns if the given dependency should be included. + /// Asks the resolved features if the given package should be included. /// - /// 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) + /// 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() } /// Variant of `activated_features` that returns `None` if this is @@ -415,8 +409,14 @@ 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,7 +475,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } Ok(ResolvedFeatures { activated_features: r.activated_features, - activated_dependencies: r.activated_dependencies, opts: r.opts, }) } @@ -507,10 +506,10 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { Ok(()) } - /// Activates [`FeatureValue`]s on the given package. + /// Activates a list of [`FeatureValue`] for a given package. /// - /// This is the main entrance into the recursion of feature activation - /// for a 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. fn activate_pkg( &mut self, pkg_id: PackageId, @@ -518,9 +517,13 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { fvs: &[FeatureValue], ) -> CargoResult<()> { log::trace!("activate_pkg {} {}", pkg_id.name(), fk); - // 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. + // 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`. 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 20a9ca0b657..46c56c9f6e8 100644 --- a/src/cargo/ops/tree/graph.rs +++ b/src/cargo/ops/tree/graph.rs @@ -365,11 +365,7 @@ 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_dep_activated( - package_id, - features_for, - dep.name_in_toml(), - ) { + if !resolved_features.is_activated(dep_id, features_for) { return false; } } diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index f63320283e5..0b58842399e 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" } "#, @@ -2275,3 +2275,59 @@ 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(); +}