implement package feature unification

[mladedav]

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 --edition (from 2018 to 2021) - 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]

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]

Thanks for moving this forward!

[epage]

Feel free to edit the commits as needed

[mladedav]

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.

[mladedav]

I had to also narrow down CliFeatures passed to the feature resolver because otherwise cargo check -p a -p b -F a,b complains that a does not have feature named b. The same code already ran with all selected packages and features so if there were unknown features, it would fail.

However, simply narrowing it down does not work for resolver v1 because there are tests like other_member_from_current which check edge cases regarding a feature of an unselected package inside cwd enabling a feature of a selected package. Since resolver v1 will not be using package feature unification, I just pass all cli features as before and only narrow it down for package feature unification.

I've added a test for this but added it just to the commit that's supposed to fix the tests. I think I've done three different things regarding splitting tests into commits by now since this is adding a feature so behavior of tests before and after the addition may be either behavior with feature-unification = "selected" or it could be that it just didn't compile and now it does so if you want me to fixup the test commits, feel free to tell me what's optimal.

[epage]

Per usual, would prefer tests and test cases to be added in that initial commit. Whatever it does (nothing, change behavior, start passing, start erroring), seeing that change of state through the diffs is very helpful for reviewing and demonstrating what the behavior is.

[mladedav]

Ok, I've added all the tests with feature-unification = "selected" and change them later to package.

[epage]

From the --features docs

Space or comma separated list of features to activate. Features of workspace members may be enabled with package-name/feature-name syntax. This flag may be specified multiple times, which enables all specified features.

The way this is worded, it makes it sound like you can specify a feature of a dependency so long as its a workspace member. Whether its intended or not, you can even specify features for transitive dependencies so long as all of the activated deps are already in your Cargo.lock

@ehuss wanted to double check my reading of those docs and if you had any additional ideas about the problem of CARGO_RESOLVER_FEATURE_UNIFICATION=package cargo check -p a -F a/a -p b -F b/b trying to activate the b/b feature when b isn't in the resolve.

[mladedav]

It does work that way, I've added a test case just now to the cli features tests having cargo check -p a -F a/a,common/b because members_with_features already contains the a feature and also a DepFeature for common/b, both for package a. If that's what you mean.

Arguably that reading could even allow cargo check -p a -F b/b even if b is not a dependency of a (the quoted docs don't mention dependencies at all) but that's rejected regardless of feature unification settings.

[epage]

So if I'm understanding correctly, the narrowed features are preserving that existing behavior

Didn't help that I read through this multiple times before it clicked what you are doing with this code block. You had already divided up features by workspace member, so now you are getting the ones only relevant for the selected workspace members to apply to resolution.

What would this then do with CARGO_RESOLVER_FEATURE_UNIFICATION=package cargo check -p a -F transitive/feature? I'm assuming it silently ignores transitive/feature as it wasn't in members_with_features and so doesn't get forwarded on to the feature resolver.

That case isn't officially supported but it would at least be good to call out if the behavior deviates from regular cargo behavior in the tracking issue. We can then decide how we want to handle that case more generally.

[mladedav]

Do you mean something like this? It fails for both package and selected so I assume not, but then I don't understand what case do you mean.

Test case

#[cargo_test]
fn transitive_feature() {
    Package::new("child", "1.0.0")
        .add_dep(&Dependency::new("grand_child", "1.0"))
        .publish();
    Package::new("grand_child", "1.0.0")
        .feature("a", &[])
        .file(
            "lib.rs",
            r#"
            #[cfg(not(feature = "a"))]
            compile_error!("missing feature");
            "#,
        )
        .publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                version = "0.1.0"
                edition = "2024"

                [dependencies]
                chile = "1.0"
            "#,
        )
        .file("src/lib.rs", "")
        .build();

    p.cargo("check -F grand_child/a")
        .arg("-Zfeature-unification")
        .masquerade_as_nightly_cargo(&["feature-unification"])
        .env("CARGO_RESOLVER_FEATURE_UNIFICATION", "selected")
        .with_status(101)
        .with_stderr_data(str![[r#"
[ERROR] the package 'foo' does not contain this feature: grand_child/a

"#]])
        .run();

    p.cargo("check -F grand_child/a")
        .arg("-Zfeature-unification")
        .masquerade_as_nightly_cargo(&["feature-unification"])
        .env("CARGO_RESOLVER_FEATURE_UNIFICATION", "package")
        .with_status(101)
        .with_stderr_data(str![[r#"
[ERROR] the package 'foo' does not contain this feature: grand_child/a

"#]])
        .run();
}

[epage]

Looks like I over-estimated what is supported. The case I'm talking about is limited to direct dependencies:

#[cargo_test]
fn non_member_features() {
    Package::new("child", "1.0.0")
        .add_dep(&Dependency::new("grand_child", "1.0"))
        .feature("a", &[])
        .file(
            "src/lib.rs",
            r#"
            #[cfg(feature = "a")]
            compile_error!("`a` is active");
            "#,
        )
        .publish();
    Package::new("grand_child", "1.0.0")
        .feature("a", &[])
        .file(
            "src/lib.rs",
            r#"
            #[cfg(feature = "a")]
            compile_error!("`a` is active");
            "#,
        )
        .publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                version = "0.1.0"
                edition = "2024"

                [dependencies]
                child = "1.0"
            "#,
        )
        .file("src/lib.rs", "")
        .build();

    p.cargo("check -F child/a")
        .with_status(101)
        .with_stderr_data(str![[r#"
[UPDATING] `dummy-registry` index
[LOCKING] 2 packages to latest Rust 1.87.0 compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] grand_child v1.0.0 (registry `dummy-registry`)
[DOWNLOADED] child v1.0.0 (registry `dummy-registry`)
[CHECKING] grand_child v1.0.0
[CHECKING] child v1.0.0
[ERROR] `a` is active
 --> [ROOT]/home/.cargo/registry/src/-[HASH]/child-1.0.0/src/lib.rs:3:13
  |
3 |             compile_error!("`a` is active");
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[ERROR] could not compile `child` (lib) due to 1 previous error

"#]])
        .run();

    p.cargo("check -F grand_child/a")
        .with_status(101)
        .with_stderr_data(str![[r#"
[ERROR] the package 'foo' does not contain this feature: grand_child/a

"#]])
        .run();
}

[mladedav]

I've added a test at the end trying to cover as much of this as I could with all three cases of feature unification. It seems to work the same way regardless of the option, i.e. you can set features on direct dependencies but not on grand-dependencies.

This has a consequence that Cargo.tomls are more expressive than command line, e.g. you can set default features so that a builds with common/a and b builds with common/b. If there are no default features (or features on a and b turning on the respective features), one cannot just write something like cargo check -p a -p b -F a/common/a -F b/common/b to turn the featur on only for the given root package. I would say this is correct behavior, just something to point out, because until now I believe one could theoretically just not define any features and then work around that on the command line, which is no longer the case with feature-unification = package. I'm not sure if what I wrote is comprehensible, if not, I'll try to crate full examples for illustration.

[epage]

Just need to double check on one question (#15684 (comment)) but otherwise, this looks good. Thanks for your work on this and patience through the review feedback!

[mladedav]

Thanks for your work on this and patience through the review feedback!

Honestly, this has been a blast. I wouldn't say I needed pretty much any patience with how fast you were reacting to each iteration. Thanks for the time and reviews!

[epage]

Whats the intention of this test?

[mladedav]

I tried to cover what you were talking about regarding turning on features of non-selected dependency crates inside a workspace. I thought it would be different in some cases based on feature-unification but it's consistent.

Do you want me to remove this?

[epage]

There are three cases for foo in foo/bar

1. A workspace member
2. A direct dep of a workspace member
3. An indirect dep of a workspace member

In #15684 (comment), I was talking about (2) while this test covers (1).

[mladedav]

Right, I tried to cover the first two cases in the original package_feature_unification_cli_features.

Here I tried specifically to get close to 3 but since that does not work in the general case, I tried to get closer to it by the indirect dependency being part of the workspace, but either not being selected or being excluded.

I was unsure about how some of the scenarios would (or wouldn't) work but arguably since all of the cases work the same way regardless of feature-unification, it might not belong here as it's more of a general features test. Or it might as it should be the same for all of them.

So I can either leave this as is, move it somewhere or just remove it completely.

And if you want the other cases covered more than they are in the other test, feel free to tell me.

[epage]

Right, I tried to cover the first two cases in the original package_feature_unification_cli_features.

Ok, that looks to be the case "check -p a -F a/a,outside/b".

So I can either leave this as is, move it somewhere or just remove it completely.

Unsure but we should at least make the name more clear that this is focused on activating features for specific packages only within a workspace, contracted with package_feature_unification_cli_features which focused on a combination of workspace members and registry dependencies while having a similar name of package_feature_unification_grandchild_cli_features.

[mladedav]

I've renamed the second one to feature_unification_of_cli_features_within_workspace. I didn't come up with anything more descriptive yet reasonable than package_feature_unification_cli_features for the first one so I've left that as is.

[epage]

@mladedav looks like there is a conflict?

[mladedav]

Right, I've rebased this on top of master now

[epage]

Thanks!