Auto merge of #11183 - weihanglo:issue/10526, r=ehuss

artifact deps shoud works when target field specified coexists with `optional = true`

Author: bors

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()

src/cargo/core/resolver/features.rs

@@ -56,11 +56,12 @@ type ActivateMap = HashMap<PackageFeaturesKey, BTreeSet<InternedString>>;
 
 /// 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,20 +506,24 @@ 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,
         fk: FeaturesFor,
         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);

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;
                     }
                 }

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();
+}