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

Tracking Issue for workspace feature-unification #14774

Open
1 of 3 tasks
ehuss opened this issue Nov 2, 2024 · 6 comments
Open
1 of 3 tasks

Tracking Issue for workspace feature-unification #14774

ehuss opened this issue Nov 2, 2024 · 6 comments
Labels
A-features Area: features — conditional compilation C-tracking-issue Category: A tracking issue for something unstable. S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. Z-feature-unification Nightly: resolver.feature-unifiication

Comments

@ehuss
Copy link
Contributor

ehuss commented Nov 2, 2024

Summary

RFC: #3692
Implementation:

Adds the resolver.feature-unification configuration option to control how features are unified across a workspace.

Unresolved Issues

  • Bikeshed the config option naming.

Future Extensions

  • Support in manifests
  • Dependency version unification
  • Unify features in other settings

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

@ehuss ehuss added A-features Area: features — conditional compilation C-tracking-issue Category: A tracking issue for something unstable. S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. Z-feature-unification Nightly: resolver.feature-unifiication labels Nov 2, 2024
@weiznich

This comment has been minimized.

@weiznich

This comment has been minimized.

@epage

This comment has been minimized.

@weiznich

This comment has been minimized.

@epage
Copy link
Contributor

epage commented Nov 15, 2024

As for implementing this, I think its ok to implement each config value (and maybe stabilize) separately.

For all of this, the top-level function for where all of this lives is

pub fn resolve_ws_with_opts<'gctx>(
ws: &Workspace<'gctx>,
target_data: &mut RustcTargetData<'gctx>,
requested_targets: &[CompileKind],
cli_features: &CliFeatures,
specs: &[PackageIdSpec],
has_dev_units: HasDevUnits,
force_all_targets: ForceAllTargets,
dry_run: bool,
) -> CargoResult<WorkspaceResolve<'gctx>> {
let mut registry = ws.package_registry()?;
let (resolve, resolved_with_overrides) = if ws.ignore_lock() {
let add_patches = true;
let resolve = None;
let resolved_with_overrides = resolve_with_previous(
&mut registry,
ws,
cli_features,
has_dev_units,
resolve.as_ref(),
None,
specs,
add_patches,
)?;
ops::print_lockfile_changes(ws, None, &resolved_with_overrides, &mut registry)?;
(resolve, resolved_with_overrides)
} else if ws.require_optional_deps() {
// First, resolve the root_package's *listed* dependencies, as well as
// downloading and updating all remotes and such.
let resolve = resolve_with_registry(ws, &mut registry, dry_run)?;
// No need to add patches again, `resolve_with_registry` has done it.
let add_patches = false;
// Second, resolve with precisely what we're doing. Filter out
// transitive dependencies if necessary, specify features, handle
// overrides, etc.
add_overrides(&mut registry, ws)?;
for (replace_spec, dep) in ws.root_replace() {
if !resolve
.iter()
.any(|r| replace_spec.matches(r) && !dep.matches_id(r))
{
ws.gctx()
.shell()
.warn(format!("package replacement is not used: {}", replace_spec))?
}
if dep.features().len() != 0 || !dep.uses_default_features() {
ws.gctx()
.shell()
.warn(format!(
"replacement for `{}` uses the features mechanism. \
default-features and features will not take effect because the replacement dependency does not support this mechanism",
dep.package_name()
))?
}
}
let resolved_with_overrides = resolve_with_previous(
&mut registry,
ws,
cli_features,
has_dev_units,
Some(&resolve),
None,
specs,
add_patches,
)?;
(Some(resolve), resolved_with_overrides)
} else {
let add_patches = true;
let resolve = ops::load_pkg_lockfile(ws)?;
let resolved_with_overrides = resolve_with_previous(
&mut registry,
ws,
cli_features,
has_dev_units,
resolve.as_ref(),
None,
specs,
add_patches,
)?;
// Skipping `print_lockfile_changes` as there are cases where this prints irrelevant
// information
(resolve, resolved_with_overrides)
};
let pkg_set = get_resolved_packages(&resolved_with_overrides, registry)?;
let member_ids = ws
.members_with_features(specs, cli_features)?
.into_iter()
.map(|(p, _fts)| p.package_id())
.collect::<Vec<_>>();
pkg_set.download_accessible(
&resolved_with_overrides,
&member_ids,
has_dev_units,
requested_targets,
target_data,
force_all_targets,
)?;
let feature_opts = FeatureOpts::new(ws, has_dev_units, force_all_targets)?;
let resolved_features = FeatureResolver::resolve(
ws,
target_data,
&resolved_with_overrides,
&pkg_set,
cli_features,
specs,
requested_targets,
feature_opts,
)?;
pkg_set.warn_no_lib_packages_and_artifact_libs_overlapping_deps(
ws,
&resolved_with_overrides,
&member_ids,
has_dev_units,
requested_targets,
target_data,
force_all_targets,
)?;
Ok(WorkspaceResolve {
pkg_set,
workspace_resolve: resolve,
targeted_resolve: resolved_with_overrides,
resolved_features,
})
}

Keep in mind that we have two resolvers (dependency, feature) and multiple phases

  1. Resolve dependencies for the lockfile (resolve_with_registry)
  2. Resolve dependencies to strip out unrelated packages, unspecified features (resolve_with_previous)
  3. Resolve features (FeatureResolver::resolve)

To make workspace work, we'll likely need to duplicate the logic from (2) to be able to run it after feature resolver so that we can resolve features for unselected packages. The problem with outright moving the logic from the dependency resolver to the feature resolver is that, under various circumstances, we allow Cargo to combine (1) and (2) so that dependencies don't need to be vendored or that we can skip downloading git dependencies for the sake of deciding not to use them. This is what the other two cases at the beginning of resolve_ws_with_opts are about.

The big risk is that we do the filtering differently between the the dependency resolver and the feature resolver.

As for package, a hacky way to implement that is to do back-to-back compilations. The downside is the redundant work. Ideally, FeatureResolver::resolve could just handle this. The main question I have is how well will it scale to have these additional disjoint feature graphs. There might need to be work done to handle that.

@weiznich

This comment has been minimized.

github-merge-queue bot pushed a commit that referenced this issue Feb 11, 2025
### 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.
github-merge-queue bot pushed a commit that referenced this issue Feb 15, 2025
### What does this PR try to resolve?

Follow-up of <#15157>.
Add missing docs for <#14774>.

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

```
mdbook serve src/doc
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features Area: features — conditional compilation C-tracking-issue Category: A tracking issue for something unstable. S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. Z-feature-unification Nightly: resolver.feature-unifiication
Projects
Status: No status
Development

No branches or pull requests

3 participants