RFC: Give users control over feature unification

[epage]

This adds resolver.feature-unification to .cargo/config.toml to allow workspace unfication (cargo-workspace-hack) or per-package unification (cargo hack).

Rendered

[joshtriplett]

Thank you so much for turning the RustConf discussions into RFC prose so quickly!

[joshtriplett]

I'm going to go ahead and start the asynchronous process of checking for consensus or concerns on this idea. People should feel free to read and consider at their leisure, and we can always discuss it further; just want to give a place for folks who have had a chance to review it to register either consensus or concerns or both.

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[RalfJung]

The main situation I encounter this with is a workspace that has a single package, but the different cargo targets have different dependencies and that affects feature resolution in a way that cargo build vs cargo test leads to rebuilds. Will this RFC help there? If yes, then the text should be clarified, because it seems to talk entirely about packages but not about targets within a package. If not -- what would it take to help in that situation?

[epage]

Thanks for calling this out! I had considered this but forgot it and didn't want to hold it up until I could remember.

[epage]

@sunshowers says cargo-hakari will unify normal and dev-dependencies. Platforms are not unified.

[epage]

@sunshowers says that is also unifies the features that workspace member features enable

[epage]

Some cases with unifying dev-dependencies include

• fail being used for dev-dependencies but not production
• Tests enabling std but not wanting them for normal build
• Some use case related to private keys where the impl Clone is only provided on debug builds and not release builds

There are likely other cases of this same sort of problem

[RalfJung]

In Miri's case it's dev-dependencies enabling different features for libc or some other fundamental crate, which means that even after cargo test, doing cargo build does an almost complete re-build of Miri -- that's waiting time I'd rather avoid. :)

[epage]

I've summarized this under "Future possibilities"

[weiznich]

I'm not a team member of any of the relevant teams here at all, but I nevertheless want to raise some concerns. The proposed text is very light on details what should happen regarding feature unification between host and target crates, especially for shared dependencies of them. The last time the cargo team performed changed on how feature unification worked with the 2021 edition they broke existing crates in an incompatible way. (The argumentation for that essentially was: It's required for other use-cases to work, which is really not that great if you part of the number of crates that broke due to this.)
I raised similar concerns back then a bit later in the process. It was ignored back then, so I figured that it might be worth to raise this just in the beginning this time. I would be very grateful if some of the official team members could list this as concern in the FCP comment.

As additional observation: The resolver = "2" feature back then was already forced onto upstream crates, this now introduces another potentially equally breaking feature in another location again controlled by downstream crates. I'm not 100% sure if it's really a good idea to have controls for the resolver behavior in various different places and also I'm still not sure if it's the correct solution to introduce these potential upstream package breaking changes at all.

That written: It would be surely helpful to have some sort of control over how feature unification works, but maybe that should not be done at a workspace level, but rather be up to control of who puts there the actual dependency? That would allow me as a crate author of some inter-dependent crates to correctly control how cargo treats features for these interconnected crates.

[epage]

@dlight

Could this be added into the workspace's Cargo.toml rather than .cargo/config.toml? Or is this intended to be applied to all workspaces inside a directory?

(moved here for easier to follow thread)

[epage]

We do mention manifest support in future possibilities.

Also a config doesn't require it to apply to all workspaces. It depends on where you put your config, where your workspaces are, and what directory you are in.

[kpreid]

I have some suggestions regarding spelling, punctuation, or rewording for clarity. The following suggested edits are not intended to change the meaning of the RFC but only make it easier to read.

[kpreid]

Suggested change

	* `package`: dependency features are only enabled for each package, preferring duplicate builds of dependencies to when different feature sets are selected
	* `package`: dependency features are only enabled for each package, preferring duplicate builds of dependencies when different feature sets are selected by their dependents

[kpreid]

Suggested change

	Unlike feature unification, dependency versions are alwayd unified across the
	Unlike feature unification, dependency versions are currently always unified across the

[kornelski]

If I understand correctly, currently Cargo (or rustc?) has cache per profile per target per crate, and if the crate's active features change, this invalidates the cached build for the crate.

How about dividing build caches further per crate per set of features?

[sunshowers]

@kornelski per-feature caching is already the case. I don't consider its performance characteristics acceptable in large projects personally, which is why I maintain hakari.

[RalfJung]

@epage btw you can commit a bunch of github suggestions at once if you do it in the "files changed" view. Avoids tons of small commits and some amount of notification spam. :)

[epage]

The minimum bar for this feature is parity with cargo-hakari and cargo-hack. Even if things are done suboptimally (more likely in the cargo-hack case), this proposal is fine with that so long as we can improve it over time which I hope we'll be able to.

[weiznich]

As I had this discussion with @epage privately for quite some time already. I think it's at least reasonable to publicly raise this concern here again that this is a potential breaking change for upstream crates of the workspace.

To have an explicit example where this happens:

cargo new broken
cd broken
cat "[workspace]" >> Cargo.toml

cargo new crate_a
(cd crate_a && cargo add diesel -F postgres)
cargo new crate_b
(cd crate_b && cargo add --build diesel -F sqlite)
(cd crate_b && cargo check)

That setup currently work. If this feature is stabilized and you change the resolver.feature-unification to workspace the build will fail. You can currently observe this by executing cargo check -p crate_a -p crate_b or by building in the workspace root, but please not that this broken behavior becomes then more wide spread as it then also affects builds in both folders.

Now the argumentation of @epage is that this is no problem as the user of the workspace opt into this behavior and it is no breaking change because of this. While this might be technically correct, this completely skips over potential social aspects. Just to raise a rhetorical question: What happens if you see a compiler error that some of your dependencies failed to build? Most of you go straight to the offending crates repo and fill an issue there. That means having such a change, possibly advice as "this can speed up your build" (as done by the RFC), will cause that quite a few people go and fill issues with popular crates (like e.g. diesel, where I know that it will be affected as there is now way to currently solve the underlying problem in a better way). That will cause even more work for maintainers that are already overworked.

I would really appropriate if one of the members of the cargo team could tag this as official concern.

[epage]

I would really appropriate if one of the members of the cargo team could tag this as official concern.

That workspace unification can fail with mutually exclusive features (which I'm assuming postgres and sqlite are) is already called out in the Drawbacks section of the RFC.

potential breaking change

This is aggressively strong language as breaking changes are when expected behavior changes between releases with no other intervention. That is not the case with this RFC.

Now the argumentation of @epage is that this is no problem as the user of the workspace opt into this behavior and it is no breaking change because of this.

That comment was specifically about defining breaking changes and not in whether that case is a problem for this RFC or not.

Users can currently control package unification by

• What directory they run commands in
• The combination of --package, --exclude, and --workspace flags they use, alon with multiple invocations

This RFC is another manifestation of the situation users are already in. If cargo check --workspace is broken for you today, then cargo check --config "resolver.feature-unfification = "workspace" will be just as broken.

On top of that, as already called out in the RFC, Cargo does not officially support mutually exclusive features.

However, resolver.feature-unification = "package" will make things easier for people who have packages that enable mutually exclusive features.

[weiznich]

That workspace unification can fail with mutually exclusive features (which I'm assuming postgres and sqlite are) is already called out in the Drawbacks section of the RFC.

These features are not exclusive. You can build diesel with both features enabled. I already explained the underlying issue in rust-lang/cargo#14415, it's totally unrelated to mutable exclusive features. For others: The main issue here is inconsistent feature unification between host and target dependencies around proc macro crates. Cargo currently only builds one version of the proc-macro crate for both to be used host and the target dependencies with the super set of features for host and target dependencies. In this particular case the proc-macro crate needs to generate feature specific code in the "main" crate, which has different feature sets between host and target, which in turn leads to compilation errors as the proc-macro generates code for disabled features. It's not like cargo is alone to blame for this problem, as proc-macros need to live in an external crate for other reasons.

This is aggressively strong language as breaking changes are when expected behavior changes between releases with no other intervention. That is not the case with this RFC.

Can you point to the intervention that happens from the author of diesel? At least I cannot see anything in the provided steps that happens due to some action by the diesel author, but in the end diesel code is broken. If you cannot point to that location, it is breaking something, so it's a potential breaking change.

This RFC is another manifestation of the situation users are already in. If cargo check --workspace is broken for you today, then cargo check --config "resolver.feature-unfification = "workspace" will be just as broken.

That's all fine, nevertheless it breaks a command invocation in a new location. I'm not sure how other than "breaking" I would call this new behavior. Or do you argue that this does not break anything?

On top of that, as already called out in the RFC, Cargo does not officially support mutually exclusive features.

However, resolver.feature-unification = "package" will make things easier for people who have packages that enable mutually exclusive features.

As mentioned before: This issue is totally not about mutually exclusive features, so these arguments are not relevant for the discussion.

---

Finally: I still do not see how you address my main concern around the potential social impact on authors of such crates that are broken by this change. Your argumentation only includes either arguments about mutually exclusive features (which is not even relevant for the underlying problem, or even the example) and something that at least for me sounds like: No that's no problem at all without providing some evidence. The counter point to the later argument is: I as author of such a crate say: Yes it's a problem. So either you claim that my experience and reasoning is not real/relevant or your just choose to ignore it. Both is highly concerning.

I know you all have many things to do, but please take the time to understand the problem and read the relevant context. I do currently not have the impression that this happened, given the not relevant arguments about mutually exclusive features.

[sunshowers]

@weiznich I'm deeply sympathetic to your concern. At the same time, I believe the current state of feature unification in Cargo is truly and utterly unacceptable for larger projects. I've put my money where my mouth is — I've invested significant time and energy maintaining a tool (hakari) which does feature unification via a workspace-hack package for you. And hakari, while being a positive experience overall, leads to some major complications in practice that wouldn't happen with native support for unification in Cargo.

I think on balance it's just really difficult for me to get to the point where those concerns are outweighed by Diesel's.

[eric-seppanen]

The broken example also doesn't build from the top of the workspace if the workspace Cargo.toml contains resolver = "2".

If a workspace already fails to build from the workspace root, and I add a setting that requests "always unify features as though I'm building from the workspace root", then it seems like I got what I requested.

A related question: Should this feature-unification setting only be allowed in workspaces using resolver v2 (or higher)?

[weiznich]

@sunshowers Thanks for your response. To be clear here: I don't ask for canceling this feature, I just want to make sure that the cargo team is fully aware of the consequences and makes a conscious decision here. At least I personally feel that this requires more than one team member essentially stating: "That's no problem".

I also believe that there is much that can be done to find a middle ground between having this feature and not having it. For example the RFC could make it clearer that it might break stuff and it's on cargo to communicate that this breakage might have happened due to non-default resolver settings. Or that the wording around this feature shouldn't be "Setting xyz will improve build performance without consequences" (I am deliberately exaggerating here).

On a more fundamental level this RFC could also seek to introduce settings for crates to deliberately say: Never unify features in this or that way so that I as a crate author can opt out of these problems. It's not like there are no solutions here that allows to get both. There are certainly also different solutions to the underlying problem, it would just take someone to address them, which is something that I see as task for the person that proposes this change.
After all the RFC process is the place where such discussions are supposed to happen. Finally at least in the past: We need this now, so lets rush it was not a good argument to get a RFC landed.

I think on balance it's just really difficult for me to get to the point where those concerns are outweighed by Diesel's.

While I totally can understand that conclusion I just want to clarify here again: You basically say, we need this for omicron/ a similar project? which has how many users? Is that really more important than diesel which has also quite a lot of users?
The point here: It's possibly not good to argue with: Popular Project xyz needs that, let's break a different also popular project abc for that in some cases, because where do you want to draw the line? Which project is more popular/important? After all the default is: Do not break existing code/workflows, at least not without issuing a major release!
Again: I'm not saying that this shouldn't happen, just that there needs to be a clear decision process that is aware of the potential consequences and that carefully looks at alternatives.

[sunshowers]

Really appreciate your perspective @weiznich.

While I totally can understand that conclusion I just want to clarify here again: You basically say, we need this for omicron/ a similar project? which has how many users? Is that really more important than diesel which has also quite a lot of users?

Every single Rust project I maintain with more than like 4 crates in it (including nextest) uses hakari. Medium or large projects that aren't using hakari or some kind of similar workspace-hack unification are wasting massive amounts of developer time and resources. (Hakari makes common build workflows up to twice as fast! We usually invest large amounts of effort towards even a 5% improvement in compile times, and there's a 2x improvement just sitting there waiting to happen.)

Not having a combinatorial explosion of features while building dependencies starts paying off very very early.

The point here: It's possibly not good to argue with: Popular Project xyz needs that, let's break a different also popular project abc for that in some cases, because where do you want to draw the line? Which project is more popular/important?

Having run hakari in production for years across a variety of setups (including Diesel within Omicron), I can say with some confidence that feature unification just hasn't caused issues in practice. Hakari does have a config mechanism to exclude particular crates from unification, but I haven't had to reach for that in a while.

After all the default is: Do not break existing code/workflows, at least not without issuing a major release!

I appreciate this perspective and I'd be hesitant to support this change if the improvements are minor. But they're not minor -- I believe that feature unification might be the single most impactful improvement to Rust compile times since 1.0 -- and I think it's worth making the ecosystem adapt to it.

[Diggsey]

@weiznich rust-lang/cargo#14415 is definitely a problem that needs to be fixed. The change in this RFC adds a new way to "expose" that problem. But the problem already existed, and it was already possible to run into it, no?

It sounds like your concern is not a concern about the merit of this change in isolation, but assuming we do want this change to go through whether that should be blocked on some improvement to rust-lang/cargo#14415 or not.

[weiznich]

Thanks for your responses.

Medium or large projects that aren't using hakari or some kind of similar workspace-hack unification are wasting massive amounts of developer time and resources. (Hakari makes common build workflows up to twice as fast! We usually invest large amounts of effort towards even a 5% improvement in compile times, and there's a 2x improvement just sitting there waiting to happen.)

If that's so widespread it shouldn't be too hard to provide some exact numbers that say: "We measure a speedup of x in this cases. You can reproduce this by doing y." I do not doubt that this is the case, it would just make the discussion much more transparent than: Someone says this might speedup xyz.

Having run hakari in production for years across a variety of setups (including Diesel within Omicron), I can say with some confidence that feature unification just hasn't caused issues in practice. Hakari does have a config mechanism to exclude particular crates from unification, but I haven't had to reach for that in a while.

I wouldn't go as far as confidently stating that this will not cause issues in practice, as at least I see real uses cases that might have a setup as shown in the "broken" example, although I agree that these are likely not common. That's the reason why I repeatably wrote that I don't think the RFC should be cancelled because of that, it only needs to make sure that the communication around the feature is careful. In my opinion it needs to at least communicate:

• That it is possible that things break if you change this settings
• Ideally cargo needs to detect that this setting is not the default and put up a rather explicit message that this is likely the issue is caused by this setting and that the user should try it without that setting. Given that cargo has access to the dependency tree it can even try to detect setups like the example. I would expect that the majority of such issues is around feature unification between shared host/target dependencies. It should be possible to just detect this case.
• Maybe consider not promoting this as the "ultimate" performance booster for compile times, as that might result in people coming to crates that cannot support this and complain about that as it "costs" performance. (That wouldn't be necessary if cargo is able to reliable detect the problem on it's own)
• Possibly it also helps to change the scope of the RFC and directly address the outlined issue as part of the RFC. Yes that will require more work, but it would result in a better feature.

I appreciate this perspective and I'd be hesitant to support this change if the improvements are minor. But they're not minor -- I believe that feature unification might be the single most impactful improvement to Rust compile times since 1.0 -- and I think it's worth making the ecosystem adapt to it.

Again: On a fundamental level I need to disagree here. A breaking change is a breaking change, no matter how useful it is or not. If you believe that it is really that useful and it is worth a breaking change you should be at least honest and say: Let's bump the major version. My main point here is: It's up to the RFC author to provide reasonable evidence that this is not a breaking change. Given the provided example and the communication from the author I must say that I cannot see that evidence yet.
On a practical level I agree in so far that this change (given proper communication/documentation/implementation) might be a worthwhile improvement and not a breaking change.

It sounds like your concern is not a concern about the merit of this change in isolation, but assuming we do want this change to go through whether that should be blocked on some improvement to rust-lang/cargo#14415 or not.

Yes, my concern around this is not only around this particular change. As written before: I'm not requesting that this isn't accepted. My main concern here is around the ability of the cargo team to decide whether or not something is a breaking change, given the history of rust-lang/cargo#14415 (which was a breaking change, with a very similar scope). Given how the team ignored my concerns back then I just want to make sure that this time this issue:

• Is registered as official concern
• Discussed with proper arguments
• Maybe some adjustments are made to make it clearer that changing this setting is entirely up to the user that changes it and that there are situations where certain settings may break things.
• Ideally: Concentrating on fixing regressions before introducing new features

[pacak]

If that's so widespread it shouldn't be too hard to provide some exact numbers that say

Those are figures I got from a sizeable workspace with no hack, a different hack manager and something similar to hackari:

https://crates.io/crates/cargo-hackerman#user-content-hackerman-vs-no-hack-vs-single-hack-crate

Example is somewhat artificial, but it shows 2x difference in total compilation time because dependencies are recompiled multiple times with different sets of features.

Lack of feature unification hurts workspaces that use nix to compile dependencies even worse - I saw cases where a single crate was compiled 20-25 times, with total compilation time increasing from 10-15 minutes to 1-2 hours on a CI machine. No exact measurements, but the order of magnitude is correct.