Skip to content

Commit

Permalink
feat: implement workspace feature unification (#15157)
Browse files Browse the repository at this point in the history
### What does this PR try to resolve?

Adds workspace feature unification for #14774

### How should we test and review this PR?

In a workspace that has dependencies with different activated features
depending on the packages being built, add the
`resolver.feature-unification = "workspace"` option to
`.cargo/config.toml`. Build the entire workspace with `--workspace` and
then build individual packages, ensuring no dependencies are being
recompiled.

### Additional information

Originally, the RFC and tracking issue mention some more complex changes
required in cargo's dependency/feature resolution phases in order to
support workspace feature unification. However, it seems like it also
works by just modifying the list of `PackageIdSpec`s passed to the
workspace resolver to include all workspace members, and then using the
original list of specs when generating the build units. I'm wondering if
I missed something because this change feels a bit *too* simple...

I tested it on a fairly large workspace containing about 100 packages
and didn't see any recompilations when building with different sets of
packages. I also added an integration test that verifies the correct
features are enabled. If there are any other test cases I should
include, please let me know and I'll try to add it.
  • Loading branch information
weihanglo authored Feb 11, 2025
2 parents e3fa31e + a471adb commit 321f14e
Show file tree
Hide file tree
Showing 9 changed files with 276 additions and 32 deletions.
2 changes: 2 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,7 @@ unstable_cli_options!(
direct_minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum (direct dependencies only)"),
doctest_xcompile: bool = ("Compile and run doctests for non-host target using runner config"),
dual_proc_macros: bool = ("Build proc-macros for both the host and the target"),
feature_unification: bool = ("Enable new feature unification modes in workspaces"),
features: Option<Vec<String>>,
gc: bool = ("Track cache usage and \"garbage collect\" unused files"),
#[serde(deserialize_with = "deserialize_git_features")]
Expand Down Expand Up @@ -1271,6 +1272,7 @@ impl CliUnstable {
"direct-minimal-versions" => self.direct_minimal_versions = parse_empty(k, v)?,
"doctest-xcompile" => self.doctest_xcompile = parse_empty(k, v)?,
"dual-proc-macros" => self.dual_proc_macros = parse_empty(k, v)?,
"feature-unification" => self.feature_unification = parse_empty(k, v)?,
"gc" => self.gc = parse_empty(k, v)?,
"git" => {
self.git = v.map_or_else(
Expand Down
28 changes: 23 additions & 5 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use crate::core::{
use crate::core::{EitherManifest, Package, SourceId, VirtualManifest};
use crate::ops;
use crate::sources::{PathSource, SourceConfigMap, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
use crate::util::context::FeatureUnification;
use crate::util::edit_distance;
use crate::util::errors::{CargoResult, ManifestError};
use crate::util::interning::InternedString;
Expand Down Expand Up @@ -112,7 +113,8 @@ pub struct Workspace<'gctx> {
/// and other places that use rust version.
/// This is set based on the resolver version, config settings, and CLI flags.
resolve_honors_rust_version: bool,

/// The feature unification mode used when building packages.
resolve_feature_unification: FeatureUnification,
/// Workspace-level custom metadata
custom_metadata: Option<toml::Value>,

Expand Down Expand Up @@ -246,6 +248,7 @@ impl<'gctx> Workspace<'gctx> {
requested_lockfile_path: None,
resolve_behavior: ResolveBehavior::V1,
resolve_honors_rust_version: false,
resolve_feature_unification: FeatureUnification::Selected,
custom_metadata: None,
local_overlays: HashMap::new(),
}
Expand Down Expand Up @@ -307,13 +310,20 @@ impl<'gctx> Workspace<'gctx> {
}
}
}
if let CargoResolverConfig {
incompatible_rust_versions: Some(incompatible_rust_versions),
} = self.gctx().get::<CargoResolverConfig>("resolver")?
{
let config = self.gctx().get::<CargoResolverConfig>("resolver")?;
if let Some(incompatible_rust_versions) = config.incompatible_rust_versions {
self.resolve_honors_rust_version =
incompatible_rust_versions == IncompatibleRustVersions::Fallback;
}
if self.gctx().cli_unstable().feature_unification {
self.resolve_feature_unification = config
.feature_unification
.unwrap_or(FeatureUnification::Selected);
} else if config.feature_unification.is_some() {
self.gctx()
.shell()
.warn("ignoring `resolver.feature-unification` without `-Zfeature-unification`")?;
};

Ok(())
}
Expand Down Expand Up @@ -663,6 +673,14 @@ impl<'gctx> Workspace<'gctx> {
self.resolve_honors_rust_version
}

pub fn set_resolve_feature_unification(&mut self, feature_unification: FeatureUnification) {
self.resolve_feature_unification = feature_unification;
}

pub fn resolve_feature_unification(&self) -> FeatureUnification {
self.resolve_feature_unification
}

pub fn custom_metadata(&self) -> Option<&toml::Value> {
self.custom_metadata.as_ref()
}
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::ops::{common_for_install_and_uninstall::*, FilterRule};
use crate::ops::{CompileFilter, Packages};
use crate::sources::source::Source;
use crate::sources::{GitSource, PathSource, SourceConfigMap};
use crate::util::context::FeatureUnification;
use crate::util::errors::CargoResult;
use crate::util::{Filesystem, GlobalContext, Rustc};
use crate::{drop_println, ops};
Expand Down Expand Up @@ -862,6 +863,7 @@ fn make_ws_rustc_target<'gctx>(
ws.set_resolve_honors_rust_version(Some(false));
ws
};
ws.set_resolve_feature_unification(FeatureUnification::Selected);
ws.set_ignore_lock(gctx.lock_update_allowed());
ws.set_requested_lockfile_path(lockfile_path.map(|p| p.to_path_buf()));
// if --lockfile-path is set, imply --locked
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ pub fn fix(
}
let mut ws = Workspace::new(&root_manifest, gctx)?;
ws.set_resolve_honors_rust_version(Some(original_ws.resolve_honors_rust_version()));
ws.set_resolve_feature_unification(original_ws.resolve_feature_unification());
ws.set_requested_lockfile_path(opts.requested_lockfile_path.clone());

// Spin up our lock server, which our subprocesses will use to synchronize fixes.
Expand Down
5 changes: 5 additions & 0 deletions src/cargo/ops/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ use crate::core::Workspace;
use crate::ops;
use crate::sources::RecursivePathSource;
use crate::util::cache_lock::CacheLockMode;
use crate::util::context::FeatureUnification;
use crate::util::errors::CargoResult;
use crate::util::CanonicalUrl;
use anyhow::Context as _;
Expand Down Expand Up @@ -144,6 +145,10 @@ pub fn resolve_ws_with_opts<'gctx>(
force_all_targets: ForceAllTargets,
dry_run: bool,
) -> CargoResult<WorkspaceResolve<'gctx>> {
let specs = match ws.resolve_feature_unification() {
FeatureUnification::Selected => specs,
FeatureUnification::Workspace => &ops::Packages::All(Vec::new()).to_package_id_specs(ws)?,
};
let mut registry = ws.package_registry()?;
let (resolve, resolved_with_overrides) = if ws.ignore_lock() {
let add_patches = true;
Expand Down
8 changes: 8 additions & 0 deletions src/cargo/util/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2745,6 +2745,7 @@ impl BuildTargetConfig {
#[serde(rename_all = "kebab-case")]
pub struct CargoResolverConfig {
pub incompatible_rust_versions: Option<IncompatibleRustVersions>,
pub feature_unification: Option<FeatureUnification>,
}

#[derive(Debug, Deserialize, PartialEq, Eq)]
Expand All @@ -2754,6 +2755,13 @@ pub enum IncompatibleRustVersions {
Fallback,
}

#[derive(Copy, Clone, Debug, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum FeatureUnification {
Selected,
Workspace,
}

#[derive(Deserialize, Default)]
#[serde(rename_all = "kebab-case")]
pub struct TermConfig {
Expand Down
56 changes: 29 additions & 27 deletions tests/testsuite/cargo/z_help/stdout.term.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading

0 comments on commit 321f14e

Please sign in to comment.