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 all commits
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
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
193 changes: 145 additions & 48 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 All @@ -378,7 +392,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
for (member, requested_features) in &member_features {
let fvs = self.fvs_from_requested(member.package_id(), requested_features);
let for_host = self.track_for_host && self.is_proc_macro(member.package_id());
self.activate_pkg(member.package_id(), &fvs, for_host)?;
self.activate_pkg(member.package_id(), for_host, &fvs)?;
if for_host {
// Also activate without for_host. This is needed if the
// proc-macro includes other targets (like binaries or tests),
Expand All @@ -387,7 +401,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
// `--workspace`), this forces feature unification with normal
// dependencies. This is part of the bigger problem where
// features depend on which packages are built.
self.activate_pkg(member.package_id(), &fvs, false)?;
self.activate_pkg(member.package_id(), false, &fvs)?;
}
}
Ok(())
Expand All @@ -396,17 +410,18 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
fn activate_pkg(
&mut self,
pkg_id: PackageId,
fvs: &[FeatureValue],
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.
self.activated_features
.entry((pkg_id, self.opts.decouple_host_deps && for_host))
.or_insert_with(BTreeSet::new);
for fv in fvs {
self.activate_fv(pkg_id, fv, for_host)?;
self.activate_fv(pkg_id, for_host, fv)?;
}
if !self.processed_deps.insert((pkg_id, for_host)) {
// Already processed dependencies. There's no need to process them
Expand All @@ -433,7 +448,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
}
// Recurse into the dependency.
let fvs = self.fvs_from_dependency(dep_pkg_id, dep);
self.activate_pkg(dep_pkg_id, &fvs, dep_for_host)?;
self.activate_pkg(dep_pkg_id, dep_for_host, &fvs)?;
}
}
Ok(())
Expand All @@ -443,59 +458,31 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
fn activate_fv(
&mut self,
pkg_id: PackageId,
fv: &FeatureValue,
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, *f, for_host)?;
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, &fvs, dep_for_host)?;
}
}
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, &fv, for_host)?;
if !dep_prefix {
// To retain compatibility with old behavior,
// this also enables a feature of the same
// name.
self.activate_rec(pkg_id, *dep_name, for_host)?;
}
}
// Activate the feature on the dependency.
let fv = FeatureValue::new(*dep_feature);
self.activate_fv(dep_pkg_id, &fv, dep_for_host)?;
}
}
self.activate_dep_feature(
pkg_id,
for_host,
*dep_name,
*dep_feature,
*dep_prefix,
*weak,
)?;
}
}
Ok(())
Expand All @@ -506,9 +493,15 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
fn activate_rec(
&mut self,
pkg_id: PackageId,
feature_to_enable: InternedString,
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 All @@ -534,7 +527,111 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
}
};
for fv in fvs {
self.activate_fv(pkg_id, fv, for_host)?;
self.activate_fv(pkg_id, for_host, fv)?;
}
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(())
}
Expand Down
Loading