From 7089bbf2e4af35c4f01a2f50fd575cbb94cb4b3b Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Fri, 7 Oct 2022 00:23:05 +0100 Subject: [PATCH 1/4] test(bindeps): target field specified and `optional = true` coexist --- tests/testsuite/artifact_dep.rs | 55 ++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index 2a089395f2c..2c08b73d917 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" } "#, @@ -2263,3 +2263,56 @@ 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_contains( + "\ +[ERROR] environment variable `CARGO_BIN_FILE_D1` not defined +", + ) + .with_status(101) + .run(); +} From c31e0178853190e5c3c1f2bce4191e36bdbe4374 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Fri, 7 Oct 2022 01:37:12 +0100 Subject: [PATCH 2/4] fix(bindeps): target field specified and `optional = true` coexist Previously, `is_dep_activated` depends on `activated_dependencies`, which is a map of `PackageFeaturesKey` to its own activated `DepFeature`s. `PackageFeaturesKey` in feature resolver is always comprised of * A `PackageId` of a given dependency, and * A enum value that helps decoupling activated features for that dependency. Currently depending on the type of given dependency. Looking into issue 10526, it has an `activated_dependencies` of ``` { (mycrate, NormalOrDevOrArtifactTarget(None)) -> [dep:mybindep] } ``` However, this [line][1] accidentally use a parent's `pkgid` and a dependency's `FeaturesFor` to compose a key: ``` (mycrate, NormalOrDevOrArtifactTarget("x86_64-unknown-linux-gnu")) ``` That is never a valid key to query features for artifacts dependency. A valid key should be comprised of components both from the parent: ``` (mycrate, NormalOrDevOrArtifactTarget(None)) ``` Or both from the dependency itself: ``` (mybindep, NormalOrDevOrArtifactTarget("x86_64-unknown-linux-gnu")) ``` As aforementioned `activated_dependencies` only stores parent packages and their activated features. Those informations are included in `activated_features` as well, so this commit goes with the route that removes `activated_dependencies` and uses only dependency's infomation to query if itself is activated. [1]: https://github.com/rust-lang/cargo/blob/0b84a35c2c7d70df4875a03eb19084b0e7a543ef/src/cargo/core/compiler/unit_dependencies.rs#L1097-L1098 --- src/cargo/core/compiler/unit_dependencies.rs | 18 ++++++------------ src/cargo/core/resolver/features.rs | 16 ++++------------ src/cargo/ops/tree/graph.rs | 6 +----- tests/testsuite/artifact_dep.rs | 11 +++++++---- 4 files changed, 18 insertions(+), 33 deletions(-) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 9319a19e038..f6f841d11e3 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -1049,14 +1049,8 @@ 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) + 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 +1065,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 +1097,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 +1111,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..3a615683300 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -325,17 +325,10 @@ impl ResolvedFeatures { /// /// 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) + 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 @@ -475,7 +468,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } Ok(ResolvedFeatures { activated_features: r.activated_features, - activated_dependencies: r.activated_dependencies, opts: r.opts, }) } 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 2c08b73d917..0bc860906f0 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -2308,11 +2308,14 @@ fn build_with_target_and_optional() { p.cargo("build -Z bindeps -F d1 -v") .masquerade_as_nightly_cargo(&["bindeps"]) - .with_stderr_contains( + .with_stderr( "\ -[ERROR] environment variable `CARGO_BIN_FILE_D1` not defined +[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 [..] ", - ) - .with_status(101) + ) .run(); } From 9e8395904dc53cd28b30e3bd8618540fce939945 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Fri, 7 Oct 2022 01:38:09 +0100 Subject: [PATCH 3/4] doc(features2): clarify some docs --- src/cargo/core/compiler/unit_dependencies.rs | 2 ++ src/cargo/core/resolver/features.rs | 12 +++++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index f6f841d11e3..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,6 +1050,7 @@ 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) } diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 3a615683300..7a4e9a4cca9 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -56,11 +56,9 @@ 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. activated_features: ActivateMap, - /// Optional dependencies that should be built. - /// - /// The value is the `name_in_toml` of the dependencies. - activated_dependencies: ActivateMap, + /// Options that change how the feature resolver operates. opts: FeatureOpts, } @@ -321,10 +319,10 @@ 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. + /// 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)) From 2b913b664f9f8c1c5396ec18a595b98b98231117 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 26 Oct 2022 21:41:30 +0800 Subject: [PATCH 4/4] doc(features2): explain the meaning of presence in `activated_features` --- src/cargo/core/resolver/features.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 7a4e9a4cca9..9c873765612 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -57,6 +57,9 @@ 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. opts: FeatureOpts, @@ -406,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. @@ -497,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, @@ -508,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);