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.

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

Implement weak dependency features. #8818

Merged
merged 2 commits into from
Nov 4, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Implement weak dependency features.
  • Loading branch information
ehuss committed Nov 4, 2020
commit 9ffcf69093fe82a16a29d71101be80e9c1cefa5b
1 change: 1 addition & 0 deletions src/bin/cargo/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Available unstable (nightly-only) flags:
-Z doctest-xcompile -- Compile and run doctests for non-host target using runner config
-Z terminal-width -- Provide a terminal width to rustc for error truncation
-Z namespaced-features -- Allow features with `dep:` prefix
-Z weak-dep-features -- Allow `dep_name?/feature` feature syntax

Run with 'cargo -Z [FLAG] [SUBCOMMAND]'"
);
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ pub struct CliUnstable {
pub rustdoc_map: bool,
pub terminal_width: Option<Option<usize>>,
pub namespaced_features: bool,
pub weak_dep_features: bool,
}

fn deserialize_build_std<'de, D>(deserializer: D) -> Result<Option<Vec<String>>, D::Error>
Expand Down Expand Up @@ -464,6 +465,7 @@ impl CliUnstable {
"rustdoc-map" => self.rustdoc_map = parse_empty(k, v)?,
"terminal-width" => self.terminal_width = Some(parse_usize_opt(v)?),
"namespaced-features" => self.namespaced_features = parse_empty(k, v)?,
"weak-dep-features" => self.weak_dep_features = parse_empty(k, v)?,
_ => bail!("unknown `-Z` flag specified: {}", k),
}

Expand Down
4 changes: 4 additions & 0 deletions src/cargo/core/resolver/dep_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,10 @@ impl Requirements<'_> {
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)?,
};
Ok(())
Expand Down
175 changes: 136 additions & 39 deletions src/cargo/core/resolver/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@ impl FeatureOpts {
if let ForceAllTargets::Yes = force_all_targets {
opts.ignore_inactive_targets = false;
}
if unstable_flags.weak_dep_features {
// Force this ON because it only works with the new resolver.
opts.new_resolver = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibility is to return an error here if the new resolver isn't enabled. Especially if the flag is intended to be tied to a manifest flag, we'll pobably want to return a first-class error instead of having a silent opt-in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new resolver is intended to be 100% identical to the old one, assuming none of the other options are enabled (like decouple_host_deps). The way I viewed this is that this is just an internal implementation detail that should be invisible and irrelevant to users. I considered using -Z features=weak as the CLI interface, but the parsed flags aren't available from Config or Summary where they are needed. I could special-case it, but it seemed simpler just to have a single separate flag.

The long-term stabilization plan here is that we just turn this on for everyone (set new_resolver and weak_dep_features to TRUE by default, and eventually just remove them). There will be no opt-in to it, since it should be backwards compatible. The other resolver options will remain tied to the resolver = "2" opt-in, but technically that will just turn on the other options (decouple_host_deps and friends).

Does that make sense? It does seem a bit confusing from the perspective of how it is implemented, but from the perspective of the user, it is more about opting-in to backwards-incompatible changes. The resolver opt-in is about the user's perception of things being different, and less about how it is actually implemented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right sorry I keep equating "new resolver" with all the new features (like decouple_host_deps). I forgot that the new resolver also implements the old logic! In that case this seems fine and agreed this should be a silent switch!

}
Ok(opts)
}
}
Expand Down Expand Up @@ -311,6 +315,15 @@ pub struct FeatureResolver<'a, 'cfg> {
/// This has to be separate from `FeatureOpts.decouple_host_deps` because
/// `for_host` tracking is also needed for `itarget` to work properly.
track_for_host: bool,
/// `dep_name?/feat_name` features that will be activated if `dep_name` is
/// ever activated.
///
/// 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)>>,
}

impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
Expand Down Expand Up @@ -353,6 +366,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
activated_dependencies: HashMap::new(),
processed_deps: HashSet::new(),
track_for_host,
deferred_weak_dependencies: HashMap::new(),
};
r.do_resolve(specs, requested_features)?;
log::debug!("features={:#?}", r.activated_features);
Expand Down Expand Up @@ -399,6 +413,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
for_host: bool,
fvs: &[FeatureValue],
) -> CargoResult<()> {
log::trace!("activate_pkg {} {}", pkg_id.name(), for_host);
// 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.
Expand Down Expand Up @@ -446,56 +461,28 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
for_host: bool,
fv: &FeatureValue,
) -> CargoResult<()> {
log::trace!("activate_fv {} {} {}", pkg_id.name(), for_host, fv);
match fv {
FeatureValue::Feature(f) => {
self.activate_rec(pkg_id, for_host, *f)?;
}
FeatureValue::Dep { dep_name } => {
// Mark this dependency as activated.
self.activated_dependencies
.entry((pkg_id, self.opts.decouple_host_deps && for_host))
.or_default()
.insert(*dep_name);
// Activate the optional dep.
for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) {
for (dep, dep_for_host) in deps {
if dep.name_in_toml() != *dep_name {
continue;
}
let fvs = self.fvs_from_dependency(dep_pkg_id, dep);
self.activate_pkg(dep_pkg_id, dep_for_host, &fvs)?;
}
}
self.activate_dependency(pkg_id, for_host, *dep_name)?;
}
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix,
weak,
} => {
// Activate a feature within a dependency.
for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) {
for (dep, dep_for_host) in deps {
if dep.name_in_toml() != *dep_name {
continue;
}
if dep.is_optional() {
// Activate the dependency on self.
let fv = FeatureValue::Dep {
dep_name: *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
// name.
self.activate_rec(pkg_id, for_host, *dep_name)?;
}
}
// Activate the feature on the dependency.
let fv = FeatureValue::new(*dep_feature);
self.activate_fv(dep_pkg_id, dep_for_host, &fv)?;
}
}
self.activate_dep_feature(
pkg_id,
for_host,
*dep_name,
*dep_feature,
*dep_prefix,
*weak,
)?;
}
}
Ok(())
Expand All @@ -509,6 +496,12 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
for_host: bool,
feature_to_enable: InternedString,
) -> CargoResult<()> {
log::trace!(
"activate_rec {} {} feat={}",
pkg_id.name(),
for_host,
feature_to_enable
);
let enabled = self
.activated_features
.entry((pkg_id, self.opts.decouple_host_deps && for_host))
Expand Down Expand Up @@ -539,6 +532,110 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
Ok(())
}

/// Activate a dependency (`dep:dep_name` syntax).
fn activate_dependency(
&mut self,
pkg_id: PackageId,
for_host: bool,
dep_name: InternedString,
) -> CargoResult<()> {
// Mark this dependency as activated.
let save_for_host = self.opts.decouple_host_deps && for_host;
self.activated_dependencies
.entry((pkg_id, save_for_host))
.or_default()
.insert(dep_name);
// Check for any deferred features.
let to_enable = self
.deferred_weak_dependencies
.remove(&(pkg_id, for_host, dep_name));
// Activate the optional dep.
for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) {
for (dep, dep_for_host) in deps {
if dep.name_in_toml() != dep_name {
continue;
}
if let Some(to_enable) = &to_enable {
for (dep_feature, dep_prefix) 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)?;
}
}
let fvs = self.fvs_from_dependency(dep_pkg_id, dep);
self.activate_pkg(dep_pkg_id, dep_for_host, &fvs)?;
}
}
Ok(())
}

/// Activate a feature within a dependency (`dep_name/feat_name` syntax).
fn activate_dep_feature(
&mut self,
pkg_id: PackageId,
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) {
for (dep, dep_for_host) in deps {
if dep.name_in_toml() != dep_name {
continue;
}
if dep.is_optional() {
let save_for_host = self.opts.decouple_host_deps && for_host;
if weak
&& !self
.activated_dependencies
.get(&(pkg_id, save_for_host))
.map(|deps| deps.contains(&dep_name))
.unwrap_or(false)
{
// This is weak, but not yet activated. Defer in case
// something comes along later and enables it.
log::trace!(
"deferring feature {} {} -> {}/{}",
pkg_id.name(),
for_host,
dep_name,
dep_feature
);
self.deferred_weak_dependencies
.entry((pkg_id, for_host, dep_name))
.or_default()
.insert((dep_feature, dep_prefix));
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
// name.
self.activate_rec(pkg_id, for_host, dep_name)?;
}
}
// Activate the feature on the dependency.
let fv = FeatureValue::new(dep_feature);
self.activate_fv(dep_pkg_id, dep_for_host, &fv)?;
}
}
Ok(())
}

/// Returns Vec of FeatureValues from a Dependency definition.
fn fvs_from_dependency(&self, dep_id: PackageId, dep: &Dependency) -> Vec<FeatureValue> {
let summary = self.resolve.summary(dep_id);
Expand Down
54 changes: 45 additions & 9 deletions src/cargo/core/summary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,11 @@ impl Summary {

/// Returns an error if this Summary is using an unstable feature that is
/// not enabled.
pub fn unstable_gate(&self, namespaced_features: bool) -> CargoResult<()> {
pub fn unstable_gate(
&self,
namespaced_features: bool,
weak_dep_features: bool,
) -> CargoResult<()> {
if !namespaced_features {
if self.inner.has_namespaced_features {
bail!(
Expand All @@ -101,6 +105,22 @@ impl Summary {
)
}
}
if !weak_dep_features {
for (feat_name, features) in self.features() {
for fv in features {
if matches!(fv, FeatureValue::DepFeature{weak: true, ..}) {
bail!(
"optional dependency features with `?` syntax are only \
allowed on the nightly channel and requires the \
`-Z weak-dep-features` flag on the command line\n\
Feature `{}` had feature value `{}`.",
feat_name,
fv
);
}
}
}
}
Ok(())
}

Expand Down Expand Up @@ -293,7 +313,7 @@ fn build_feature_map(
);
}
}
DepFeature { dep_name, .. } => {
DepFeature { dep_name, weak, .. } => {
// Validation of the feature name will be performed in the resolver.
if !is_any_dep {
bail!(
Expand All @@ -303,6 +323,12 @@ fn build_feature_map(
dep_name
);
}
if *weak && !is_optional_dep {
bail!("feature `{}` includes `{}` with a `?`, but `{}` is not an optional dependency\n\
A non-optional dependency of the same name is defined; \
consider removing the `?` or changing the dependency to be optional",
feature, fv, dep_name);
}
}
}
}
Expand Down Expand Up @@ -346,6 +372,10 @@ pub enum FeatureValue {
/// 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.
weak: bool,
},
}

Expand All @@ -360,10 +390,16 @@ impl FeatureValue {
} else {
(dep, false)
};
let (dep, weak) = if let Some(dep) = dep.strip_suffix('?') {
(dep, true)
} else {
(dep, false)
};
FeatureValue::DepFeature {
dep_name: InternedString::new(dep),
dep_feature: InternedString::new(dep_feat),
dep_prefix,
weak,
}
}
None => {
Expand Down Expand Up @@ -393,13 +429,13 @@ impl fmt::Display for FeatureValue {
DepFeature {
dep_name,
dep_feature,
dep_prefix: true,
} => write!(f, "dep:{}/{}", dep_name, dep_feature),
DepFeature {
dep_name,
dep_feature,
dep_prefix: false,
} => write!(f, "{}/{}", 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)
}
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1082,11 +1082,20 @@ fn validate_required_features(
target_name
);
}
FeatureValue::DepFeature { weak: true, .. } => {
anyhow::bail!(
"invalid feature `{}` in required-features of target `{}`: \
optional dependency with `?` is not allowed in required-features",
fv,
target_name
);
}
// Handling of dependent_crate/dependent_crate_feature syntax
FeatureValue::DepFeature {
dep_name,
dep_feature,
dep_prefix: false,
weak: false,
} => {
match resolve
.deps(summary.package_id())
Expand Down
Loading