implement package feature unification #15684

mladedav · 2025-06-19T21:31:57Z

What does this PR try to resolve?

Implements another part of feature unification (#14774, rfc). The workspace option was implemented in #15157, this adds the package option.

How to test and review this PR?

The important change is changing WorkspaceResolve so it can contain multiple ResolvedFeatures. Along with that, it also needs to know which specs those features are resolved for. This was used in several other places:

cargo fix - from what I understand, the location is reached only with --edition. I think it should be ok to disallow using cargo fix --edition when someone already uses this feature.
building std - it should be safe to assume std is not using this feature so I just unwrap there. I'm not sure if some attempt to later feature unification would be better.
cargo tree - I just use the first feature set. This is definitely not ideal, but I'm not entirely sure what's the correct solution here. Printing multiple trees? Disallowing this, forcing users to select only one package?

Based on comments in #15157 I've added tests first with selected feature unification and then changed that after implementation. I'm not sure if that's how you expect the tests to be added first, if not, I can change the history.

I've expanded the test checking that this is ignored for cargo install although it should work the same way even if it is not ignored (selected and package are the same thing when just one package is selected).

rustbot · 2025-06-19T21:32:03Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage · 2025-06-19T21:43:17Z

src/doc/src/reference/unstable.md

@@ -1860,7 +1860,7 @@ Specify which packages participate in [feature unification](../reference/feature
 * `selected`: Merge dependency features from all packages specified for the current build.
 * `workspace`: Merge dependency features across all workspace members,
  regardless of which packages are specified for the current build.
-* `package` _(unimplemented)_: Dependency features are considered on a package-by-package basis,
+* `package`: Dependency features are considered on a package-by-package basis,


This PR implements it as projects instead of package. What was the motivation for that?

It was a mistake but where I got the wrong name, I really can't say. Fixed everywhere, hopefully.

epage · 2025-06-19T21:44:04Z

tests/testsuite/feature_unification.rs

Would be good to have a cargo tree test as well

and cargo fix

I've added a cargo tree test, I'll just have to fixup history so that it's added before the feature.

That said, I think it might need a bit of design work because using package and selected feature unification has almost identical outuput, but in the former each tree should be taken on its own while in the latter the user should be aware that the features are unified.

For example in one simplified case where a depends on common/a and b depends on common/b; package outputs:

common v0.1.0 ([ROOT]/foo/common) a v0.1.0 ([ROOT]/foo/a) └── common feature "a" └── common v0.1.0 ([ROOT]/foo/common) b v0.1.0 ([ROOT]/foo/b) └── common feature "b" └── common v0.1.0 ([ROOT]/foo/common)

while selected currently outputs:

a v0.1.0 ([ROOT]/foo/a) └── common feature "a" └── common v0.1.0 ([ROOT]/foo/common) b v0.1.0 ([ROOT]/foo/b) └── common feature "b" └── common v0.1.0 ([ROOT]/foo/common) common v0.1.0 ([ROOT]/foo/common)

and for completeness, workspace outputs:

a v0.1.0 ([ROOT]/foo/a) └── common feature "a" └── common v0.1.0 ([ROOT]/foo/common) b v0.1.0 ([ROOT]/foo/b) └── common feature "b" └── common v0.1.0 ([ROOT]/foo/common) common v0.1.0 ([ROOT]/foo/common)

I'm not sure what would be the ideal behavior here though, we probably don't want to have both features coming out of both packages in selected because people often use this to find out what package turned on a given feature. But I don't think we should keep it as is either. Maybe adding an edge that signifies a feature but somehow marks it as coming from unification elsewhere?

For example something like this?

a v0.1.0 ([ROOT]/foo/a) └── common feature "a" └── common v0.1.0 ([ROOT]/foo/common) └── common feature "b" (unified) └── common v0.1.0 ([ROOT]/foo/common) b v0.1.0 ([ROOT]/foo/b) └── common feature "b" └── common v0.1.0 ([ROOT]/foo/common) └── common feature "a" (unified) └── common v0.1.0 ([ROOT]/foo/common) common v0.1.0 ([ROOT]/foo/common)

Should there be some kind of proposal for these changes? And in that case should I just fixup history with the current behavior or wait for the outcome of that? Or is it fine to make the changes as part of this PR or a different one but without any larger proposal than just the PR?

Should there be some kind of proposal for these changes? And in that case should I just fixup history with the current behavior or wait for the outcome of that? Or is it fine to make the changes as part of this PR or a different one but without any larger proposal than just the PR?

We can clean this up and get it merged and leave it as an Open Question on the tracking issue to have the discussion run independent of this PR

Ok, tree tests are added now as well as one fix test. I did not know what to test for though so I just took a random fix test from edition 2021 to 2024 since the relevant change should break just a specific upgrade from 2018. If you want to test for something different, let me know.

epage · 2025-06-19T21:46:06Z

src/cargo/ops/fix/mod.rs

@@ -588,7 +588,15 @@ fn check_resolver_change<'gctx>(
            feature_opts,
        )?;

-        let diffs = v2_features.compare_legacy(&ws_resolve.resolved_features);
+        if ws_resolve.specs_and_features.len() != 1 {
+            bail!(r#"Cannot fix edition when using `feature-unification = "projects"`."#);


Cargo error messages should start with a lower case letter

epage · 2025-06-19T21:48:20Z

src/cargo/ops/fix/mod.rs

@@ -588,7 +588,15 @@ fn check_resolver_change<'gctx>(
            feature_opts,
        )?;

-        let diffs = v2_features.compare_legacy(&ws_resolve.resolved_features);
+        if ws_resolve.specs_and_features.len() != 1 {
+            bail!(r#"Cannot fix edition when using `feature-unification = "projects"`."#);


cargo fix - from what I understand, the location is reached only with --edition. I think it should be ok to disallow using cargo fix --edition when someone already uses this feature.

This should just be limited to projects with edition = "2018" which should be fine

epage · 2025-06-19T21:50:07Z

src/cargo/ops/resolve.rs

-        target_data,
-        force_all_targets,
-    )?;
+    for specs in individual_specs {


So we're creating independent resolves for each package. I take it we can then leave merging of these graphs to a future possibility to further optimize this?

Just independent ResolvedFeatures, it seems everything works with just a single targeted_resolve, though I did not expect that since the field is documeted with "The narrowed resolve, with the specific features enabled." Then from those independent unit graphs are constructed and those are then merged into a single build context.

I probably don't understand the whole process well enough, but I don't see a way to merge anything earlier?

epage · 2025-06-19T21:51:47Z

src/cargo/ops/tree/mod.rs

+    // TODO build multiple graphs, one for each resolved features set.
+    let resolved_features = &ws_resolve


cargo tree - I just use the first feature set. This is definitely not ideal, but I'm not entirely sure what's the correct solution here. Printing multiple trees? Disallowing this, forcing users to select only one package?

I see this as no different than cargo tree --workspace where there will be multiple root packages. --edges features (ro whatever its called) will be needed to fully understand it.

True, I got confused, this part is fine. I think that just signalling to the user whether features are unified between the trees or not is a new problem.

epage · 2025-06-19T21:53:10Z

Thanks for moving this forward!

epage · 2025-06-20T20:30:54Z

Feel free to edit the commits as needed

tests/testsuite/feature_unification.rs

…resolve

mladedav · 2025-06-23T06:59:08Z

I've also just added a test with weak dependencies, which works. I tried to also add namespaced dep: features, but those seem to not be unified (#15694) so I'm ignoring that, but I guess that might also go towards the feature unification's unresolved questions.

…icatin

rustbot assigned weihanglo Jun 19, 2025

rustbot added A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation Command-fix Command-tree S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2025

mladedav force-pushed the dm/unification-package branch from ad48f18 to e94564d Compare June 19, 2025 21:41

epage reviewed Jun 19, 2025

View reviewed changes

mladedav changed the title ~~implement projects feature unification~~ implement package feature unification Jun 19, 2025

mladedav force-pushed the dm/unification-package branch from e94564d to c6090d5 Compare June 19, 2025 21:53

mladedav force-pushed the dm/unification-package branch from c6090d5 to e5a3134 Compare June 20, 2025 20:13

mladedav commented Jun 20, 2025

View reviewed changes

tests/testsuite/feature_unification.rs Outdated Show resolved Hide resolved

mladedav force-pushed the dm/unification-package branch from e5a3134 to 90a6e11 Compare June 21, 2025 20:45

mladedav added 3 commits June 23, 2025 08:55

tests: add future package feature unification tests

ec4ed98

refactor(resolve): allow multiple resolved feature sets in workspace …

dec2130

…resolve

feat: add the package feature unification option

30e4c51

mladedav force-pushed the dm/unification-package branch 2 times, most recently from b10f5d9 to 2457810 Compare June 23, 2025 06:59

mladedav added 2 commits June 23, 2025 09:33

tests: update package feature unification tests to use correct unif…

40532ca

…icatin

docs: update unstable docs on feature unificatin

d28a30f

mladedav force-pushed the dm/unification-package branch from 2457810 to d28a30f Compare June 23, 2025 07:33

		// TODO build multiple graphs, one for each resolved features set.
		let resolved_features = &ws_resolve

implement package feature unification #15684

Are you sure you want to change the base?

implement package feature unification #15684

Uh oh!

Conversation

mladedav commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How to test and review this PR?

Uh oh!

rustbot commented Jun 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Jun 19, 2025

Uh oh!

epage commented Jun 20, 2025

Uh oh!

Uh oh!

mladedav commented Jun 23, 2025

Uh oh!

Uh oh!

mladedav commented Jun 19, 2025 •

edited

Loading