Skip to content

Commit

Permalink
Auto merge of #11331 - weihanglo:revert-11183, r=ehuss
Browse files Browse the repository at this point in the history 
Revert #11183

This reverts commit d4c38af, reversing changes made to 92d8826.

Fixes #11330

---

The root cause is that [`map_to_features_for`] takes care of `unit_for.host_features` from parent unit, but [here] we only want to know whether the dependency is for host or not.

We could have an ad-hoc `FeaturesFor` construction there, but I'd rather revert it at this moment. The interaction between `UnitFor` and `FeaturesFor` bites me every time 😭.

After this revert, if we want to resolve #10526 again. The fastest way is doing the following:

```diff
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index 9319a19e0..14cc84941 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
`@@` -1103,7 +1103,7 `@@` 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());
+                            let features_for = FeaturesFor::from_for_host(unit_for.is_for_host());
                             if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }
```

[`map_to_features_for`]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/profiles.rs#L1043
[here]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/compiler/unit_dependencies.rs#L1102
  • Loading branch information
@bors
bors committed Nov 3, 2022
2 parents 810cbad + 5fe2732 commit 4111d42
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 94 deletions.
20 changes: 12 additions & 8 deletions src/cargo/core/compiler/unit_dependencies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,6 @@ impl<'a, 'cfg> State<'a, 'cfg> {
}
}

/// See [`ResolvedFeatures::activated_features`].
fn activated_features(
&self,
pkg_id: PackageId,
Expand All @@ -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 {
Expand All @@ -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()
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -1113,7 +1117,7 @@ impl<'a, 'cfg> State<'a, 'cfg> {
if deps.is_empty() {
None
} else {
Some((dep_id, deps))
Some((id, deps))
}
})
.collect()
Expand Down
53 changes: 25 additions & 28 deletions src/cargo/core/resolver/features.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,11 @@ type ActivateMap = HashMap<PackageFeaturesKey, BTreeSet<InternedString>>;

/// 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,
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
})
}
Expand Down Expand Up @@ -506,24 +507,20 @@ 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,
fk: FeaturesFor,
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);
Expand Down
6 changes: 5 additions & 1 deletion src/cargo/ops/tree/graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
58 changes: 1 addition & 57 deletions tests/testsuite/artifact_dep.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fn check_with_invalid_artifact_dependency() {
version = "0.0.0"
authors = []
resolver = "2"
[dependencies]
bar = { path = "bar/", artifact = "unknown" }
"#,
Expand Down Expand Up @@ -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();
}

0 comments on commit 4111d42

Please sign in to comment.