Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify weak and namespaced features. #9574

Merged
merged 1 commit into from
Jun 22, 2021
Merged

Unify weak and namespaced features. #9574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/cargo/core/resolver/dep_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,12 +405,12 @@ impl Requirements<'_> {
&mut self,
package: InternedString,
feat: InternedString,
dep_prefix: bool,
weak: bool,
) -> Result<(), RequirementError> {
// If `package` is indeed an optional dependency then we activate the
// feature named `package`, but otherwise if `package` is a required
// dependency then there's no feature associated with it.
if !dep_prefix
if !weak
&& self
.summary
.dependencies()
Expand Down Expand Up @@ -456,12 +456,11 @@ impl Requirements<'_> {
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix,
// Weak features are always activated in the dependency
// resolver. They will be narrowed inside the new feature
// resolver.
weak: _,
} => self.require_dep_feature(*dep_name, *dep_feature, *dep_prefix)?,
weak,
} => self.require_dep_feature(*dep_name, *dep_feature, *weak)?,
};
Ok(())
}
Expand Down
35 changes: 9 additions & 26 deletions src/cargo/core/resolver/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,7 @@ impl CliFeatures {
match feature {
// Maybe call validate_feature_name here once it is an error?
FeatureValue::Feature(_) => {}
FeatureValue::Dep { .. }
| FeatureValue::DepFeature {
dep_prefix: true, ..
} => {
FeatureValue::Dep { .. } => {
bail!(
"feature `{}` is not allowed to use explicit `dep:` syntax",
feature
Expand Down Expand Up @@ -441,10 +438,8 @@ pub struct FeatureResolver<'a, 'cfg> {
///
/// The key is the `(package, for_host, dep_name)` of the package whose
/// dependency will trigger the addition of new features. The value is the
/// set of `(feature, dep_prefix)` features to activate (`dep_prefix` is a
/// bool that indicates if `dep:` prefix was used).
deferred_weak_dependencies:
HashMap<(PackageId, bool, InternedString), HashSet<(InternedString, bool)>>,
/// set of features to activate.
deferred_weak_dependencies: HashMap<(PackageId, bool, InternedString), HashSet<InternedString>>,
}

impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
Expand Down Expand Up @@ -591,17 +586,9 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix,
weak,
} => {
self.activate_dep_feature(
pkg_id,
for_host,
*dep_name,
*dep_feature,
*dep_prefix,
*weak,
)?;
self.activate_dep_feature(pkg_id, for_host, *dep_name, *dep_feature, *weak)?;
}
}
Ok(())
Expand Down Expand Up @@ -675,17 +662,14 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
continue;
}
if let Some(to_enable) = &to_enable {
for (dep_feature, dep_prefix) in to_enable {
for dep_feature in to_enable {
log::trace!(
"activate deferred {} {} -> {}/{}",
pkg_id.name(),
for_host,
dep_name,
dep_feature
);
if !dep_prefix {
self.activate_rec(pkg_id, for_host, dep_name)?;
}
let fv = FeatureValue::new(*dep_feature);
self.activate_fv(dep_pkg_id, dep_for_host, &fv)?;
}
Expand All @@ -704,7 +688,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
for_host: bool,
dep_name: InternedString,
dep_feature: InternedString,
dep_prefix: bool,
weak: bool,
) -> CargoResult<()> {
for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) {
Expand Down Expand Up @@ -733,16 +716,16 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
self.deferred_weak_dependencies
.entry((pkg_id, for_host, dep_name))
.or_default()
.insert((dep_feature, dep_prefix));
.insert(dep_feature);
continue;
}

// Activate the dependency on self.
let fv = FeatureValue::Dep { dep_name };
self.activate_fv(pkg_id, for_host, &fv)?;
if !dep_prefix {
// To retain compatibility with old behavior,
// this also enables a feature of the same
if !weak {
// The old behavior before weak dependencies were
// added is to also enables a feature of the same
// name.
self.activate_rec(pkg_id, for_host, dep_name)?;
}
Expand Down
29 changes: 3 additions & 26 deletions src/cargo/core/summary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,7 @@ fn build_feature_map(
.values()
.flatten()
.filter_map(|fv| match fv {
Dep { dep_name }
| DepFeature {
dep_name,
dep_prefix: true,
..
} => Some(*dep_name),
Dep { dep_name } => Some(*dep_name),
_ => None,
})
.collect();
Expand Down Expand Up @@ -391,9 +386,6 @@ pub enum FeatureValue {
DepFeature {
dep_name: InternedString,
dep_feature: InternedString,
/// If this is true, then the feature used the `dep:` prefix, which
/// prevents enabling the feature named `dep_name`.
dep_prefix: bool,
/// If `true`, indicates the `?` syntax is used, which means this will
/// not automatically enable the dependency unless the dependency is
/// activated through some other means.
Expand All @@ -407,11 +399,6 @@ impl FeatureValue {
Some(pos) => {
let (dep, dep_feat) = feature.split_at(pos);
let dep_feat = &dep_feat[1..];
let (dep, dep_prefix) = if let Some(dep) = dep.strip_prefix("dep:") {
(dep, true)
} else {
(dep, false)
};
let (dep, weak) = if let Some(dep) = dep.strip_suffix('?') {
(dep, true)
} else {
Expand All @@ -420,7 +407,6 @@ impl FeatureValue {
FeatureValue::DepFeature {
dep_name: InternedString::new(dep),
dep_feature: InternedString::new(dep_feat),
dep_prefix,
weak,
}
}
Expand All @@ -438,14 +424,7 @@ impl FeatureValue {

/// Returns `true` if this feature explicitly used `dep:` syntax.
pub fn has_dep_prefix(&self) -> bool {
matches!(
self,
FeatureValue::Dep { .. }
| FeatureValue::DepFeature {
dep_prefix: true,
..
}
)
matches!(self, FeatureValue::Dep { .. })
}
}

Expand All @@ -458,12 +437,10 @@ impl fmt::Display for FeatureValue {
DepFeature {
dep_name,
dep_feature,
dep_prefix,
weak,
} => {
let dep_prefix = if *dep_prefix { "dep:" } else { "" };
let weak = if *weak { "?" } else { "" };
write!(f, "{}{}{}/{}", dep_prefix, dep_name, weak, dep_feature)
write!(f, "{}{}/{}", dep_name, weak, dep_feature)
}
}
}
Expand Down
18 changes: 3 additions & 15 deletions src/cargo/core/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,14 +1163,10 @@ impl<'cfg> Workspace<'cfg> {
}
}
// This should be enforced by CliFeatures.
FeatureValue::Dep { .. }
| FeatureValue::DepFeature {
dep_prefix: true, ..
} => panic!("unexpected dep: syntax {}", feature),
FeatureValue::Dep { .. } => panic!("unexpected dep: syntax {}", feature),
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix: _,
weak: _,
} => {
if dependencies.contains_key(dep_name) {
Expand Down Expand Up @@ -1283,14 +1279,10 @@ impl<'cfg> Workspace<'cfg> {
.map(|s| s.to_string())
.collect::<Vec<_>>()
}
FeatureValue::Dep { .. }
| FeatureValue::DepFeature {
dep_prefix: true, ..
} => panic!("unexpected dep: syntax {}", feature),
FeatureValue::Dep { .. } => panic!("unexpected dep: syntax {}", feature),
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix: _,
weak: _,
} => {
// Finds set of `pkg/feat` that are very similar to current `pkg/feat`.
Expand Down Expand Up @@ -1446,14 +1438,10 @@ impl<'cfg> Workspace<'cfg> {
cwd_features.insert(feature.clone());
}
// This should be enforced by CliFeatures.
FeatureValue::Dep { .. }
| FeatureValue::DepFeature {
dep_prefix: true, ..
} => panic!("unexpected dep: syntax {}", feature),
FeatureValue::Dep { .. } => panic!("unexpected dep: syntax {}", feature),
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix: _,
weak: _,
} => {
// I think weak can be ignored here.
Expand Down
6 changes: 1 addition & 5 deletions src/cargo/ops/cargo_compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1225,10 +1225,7 @@ fn validate_required_features(
))?;
}
}
FeatureValue::Dep { .. }
| FeatureValue::DepFeature {
dep_prefix: true, ..
} => {
FeatureValue::Dep { .. } => {
anyhow::bail!(
"invalid feature `{}` in required-features of target `{}`: \
`dep:` prefixed feature values are not allowed in required-features",
Expand All @@ -1248,7 +1245,6 @@ fn validate_required_features(
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix: false,
weak: false,
} => {
match resolve
Expand Down
15 changes: 7 additions & 8 deletions src/cargo/ops/tree/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,6 @@ fn add_cli_features(
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix: _,
weak,
} => {
let dep_connections = match graph.dep_name_map[&package_index].get(&dep_name) {
Expand Down Expand Up @@ -596,12 +595,12 @@ fn add_feature_rec(
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix,
// `weak` is ignored, because it will be skipped if the
// corresponding dependency is not found in the map, which
// means it wasn't activated. Skipping is handled by
// `is_dep_activated` when the graph was built.
weak: _,
// Note: `weak` is mostly handled when the graph is built in
// `is_dep_activated` which is responsible for skipping
// unactivated weak dependencies. Here it is only used to
// determine if the feature of the dependency name is
// activated on self.
weak,
} => {
let dep_indexes = match graph.dep_name_map[&package_index].get(dep_name) {
Some(indexes) => indexes.clone(),
Expand All @@ -619,7 +618,7 @@ fn add_feature_rec(
};
for (dep_index, is_optional) in dep_indexes {
let dep_pkg_id = graph.package_id_for_index(dep_index);
if is_optional && !dep_prefix {
if is_optional && !weak {
// Activate the optional dep on self.
add_feature(
graph,
Expand Down
Loading