rust-lang
RFC: Give users control over feature unification
This adds resolver.feature-unification to .cargo/config.toml to allow workspace unfication (cargo-workspace-hack) or per-package unification (cargo hack).
Thank you so much for turning the RustConf discussions into RFC prose so quickly!
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
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Eh2406
- [x] @Muscraft
- [ ] @arlosi
- [x] @ehuss
- [x] @epage
- [x] @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.
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.
@weiznich build/host feature unification was originally out of scope. This was left to be read between the lines in how we described the processes changes. We only changed when package filtering happened but still resolved features the same way. In learning more about what cargo-hakari supports, this has been added to the Unresolved Questions.
Either way, this feature is opt-in only. We are not changing default behavior on stable and not changing this through Edition-as-opt-in.
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.
This is currently being driven by the use cases where this is needed at the workspace level. Dependency-influenced feature unification would need someone to drive discussion. A concern from some corners is about dependencies controlling dependency resolution too much. For example, having a library's lockfile influence the workspaces lockfile is debatable. However, they do influence dependency resolution through version constraints. The question is where dependency-driven feature unification would fall between these two cases.
@weiznich It is already the case today that users have some control over this: if you build at the root of a workspace, you get one resolution, and if you build in a crate subdirectory, you get a different resolution. This already surprises people today. People also use approaches like cargo-workspace-hack or cargo-hakari. One of the goals of this is to switch from "give me one or the other of those depending on how I build" to "give me enough control to say I always want one or the other".
The RFC seems clear to me, and would address the issue I was concerned about: dependency feature jitter and frequent rebuilds when moving between crates in an "application-style" workspace. :thumbsup:
cargo-hakari maintainer here. I absolutely support this work in general -- hakari was always meant to be a stop-gap solution.
Going down the list of hakari's config options:
Most of the options are artifacts of the workspace-hack approach not relevant to a native implementation in Cargo.
traversal-excludes is for packages that have mutually exclusive features. It's reasonable to focus on Cargo's own implementation for them.
final-excludes is for crates like fail, which let you turn on features that have behavior changes. (In fail's case the falipoints feature causes failures to randomly be inserted, which is useful for chaos engineering.) I think this use case is worth considering, but maybe there are other workarounds here such as telling users to not do feature unification for releases.
unify-target-host is, I think, quite important. As mentioned in the text of the RFC, for Oxide's control plane, ensuring that builds were unified across the target and host platforms led to a pretty substantial improvement in build times. But there are use cases where that is not desired as well, so this may have to key off a config option (like in hakari).
(...) I think this use case is worth considering, but maybe there are other workarounds here such as telling users to not do feature unification for releases.
This is a good point that I hadn't thought of. It seems reasonable to ask for a "production release build of the application" to be built using resolver.feature-unification = "selected" even in a workspace that's configured for "developer skipping around between application component crates" (resolver.feature-unification = "workspace"). For my environment, I think it would be sufficient to document how to override this on the CLI.
@sunshowers I've summarized unify-target-host under "Future possibilities". I have some open questions about what cargo hakari does to help fill in that section some more.
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?
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?
@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.
@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. :)
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.
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.
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--workspaceflags 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.
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 https://github.com/rust-lang/cargo/issues/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.
@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.
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)?
@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.
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.
@weiznich https://github.com/rust-lang/cargo/issues/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 https://github.com/rust-lang/cargo/issues/14415 or not.
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 https://github.com/rust-lang/cargo/issues/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 https://github.com/rust-lang/cargo/issues/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
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.
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.
It will take a bit of time to measure this, but the speedup is absolutely real and is certainly not a "might". But before I invest my time into this, I would like an assurance from you that you will respect the effort that I put in.
On a fundamental level I need to disagree here. A breaking change is a breaking change, no matter how useful it is or not.
No, this is not a breaking change. The sense of breaking that matters here is a strict sense, and this is very clearly non-breaking. Existing workspaces will continue to compile with no changes—it is explicitly opt-in.
Since it is not a breaking change in the strict sense, it can be evaluated as an engineering decision based on the merits. This change would make life better for the vast majority of people who opt into it, but unfortunately result in more work for some. That is just part of life as an engineer.
It will take a bit of time to measure this, but the speedup is absolutely real and is certainly not a "might". But before I invest my time into this, I would like an assurance from you that you will respect the effort that I put in.
You are free to do whatever you want. After all I'm not a member of the cargo team, so strictly speaking my comments here are "not relevant". I merely point out that the RFC only mentions that there are speedups, but does not provide numbers or evidence for that. Coming from a scientific background that's just bad practice as you "claim" something without providing at least some evidence.
Please note: I do not doubt the speedups, it's just bad practice to make such claims without having numbers.
No, this is not a breaking change. The sense of breaking that matters here is a strict sense, and this is very clearly non-breaking. Existing workspaces will continue to compile with no changes—it is explicitly opt-in.
I'm sorry, but I need to disagree here again. You are in so far correct that this is no breaking change for the workspace owner itself as they can opt into this feature. The matter is not so clear for any crate that is a dependency of said workspace, as their authors do not opt into something as far as I can read out of the RFC and their code can break due to this setting. Given that I find it not useful that you (and others) claim this is clearly not breaking as you opt in to this behavior. I might be wrong here, but in this case again: Please point out for the broken example workspace described above: At which point I as diesel author opt into this behavior? If you cannot do that: Please point to a RFC or other official team decision that states it's OK to break code of dependencies.
Now you might still argue that this is not a breaking change for other reasons, e.g. because it's already broken in different, but similar situations. That's a different argument to made. I honestly do not see a complete evaluation of that argument yet.
Since it is not a breaking change in the strict sense, it can be evaluated as an engineering decision based on the merits. This change would make life better for the vast majority of people who opt into it, but unfortunately result in more work for some. That is just part of life as an engineer.
Again it seems like you just claim things here by saying it's more work for other people (in this case me). Let's put it the other way around: What exactly can I as diesel author do to fix this situation so that this breakage does not happen? As outlined in https://github.com/rust-lang/cargo/issues/14415 it's not possible to fix that in diesel due to fundamental limitations in how feature unification currently works. If you disagree with that I'm looking forward to see your solution. From that point of view you do not only expect me to spend more work for you, but you literally request me to do something that's not possible for all I know. Again that does not feel like a honest argument.
Finally to take up the argument: The broken example is already broken if you compile both crates from the workspace root. To play the devils advocate here: If it's already broken from there, that means you don't need to have the suggested feature as you already can have the same behavior by issuing the right command from the right place. At that point: What's the reason for having a setting for this at all? After all you can get the "same" speedup today without that setting (and without any helper tool), by just carefully writing the build/test command.
On a more fundamental side: I spend some more time thinking about whether or not that could be useful for my use cases. While doing that I noticed another rather fundamental point that I do not see addressed or discussed yet. I wouldn't call this necessarily a blocker, but I personally would see that as serve limitation of the proposed feature.
For our internal code we have a large workspace that includes client and server code, to share some types between both. The server code depends on diesel with various database backends enabled, which in turn pulls in various native libraries. The client code also uses diesel, but for different use cases and only with the sqlite backend enabled. Now the big argument for this feature is: It speeds up compile times, which I do not question in its own. You cite CI run times here as critical point. Now consider the described workspace, for test runs we do care about compile times as we want to have test results as quickly as possible, so you would enable the resolver.feature-unification = "workspace" flag for that. For release builds things are completely different: We do care much more about binary size there, so we want to build the client dependencies with a minimal set of features, especially to not include the native dependencies pulled in by diesel on server side, so we would want to enable resolver.feature-unification = "package" there. I would expect that there are quite a large number of workspaces out there with a similar setup (having client + server code in the same workspace + that care about different things for different compilation modes). That makes me wonder whether it's really a good idea to have this a workspace level setting or if this feature should be a command line flag instead. The later would make it much easier to choose different behavior for different situations.
This is RFC is not for workspace-level config (Cargo.toml [workspace]) but environment/transient config (.cargo/config.toml) which includes CLI support (--config) and environment variable support. Putting it in manifests is a future possibility.
What's the reason for having a setting for this at all? After all you can get the "same" speedup today without that setting (and without any helper tool), by just carefully writing the build/test command.
This relevant text in the RFC is:
If a user is building an application, they may be jumping around the application's components which are packages within the workspace. The final artifact is the same but Cargo will select different features depending on which package they are currently building, causing build churn for the same set of dependencies that, in the end, will only be used with the same set of features. (...)
In case that wasn't clear, it's describing a situation where a workspace contains a large number of crates, that aren't meant to be published on their own, but are sub-components of a big monolithic application. During development, it's fairly common for an individual developer to be editing only a small number of crates, and that developer may hop between workspace crate directories running e.g. cargo check. Today, switching between crate directories (or running -p some-component) triggers a unique feature unification, which in large projects (thousands of dependencies) can mean minutes of waiting. This is what the RFC means by "build churn". "Just build the whole workspace" doesn't resolve the problem because it also involves a lot of waiting, depending on where in the dependency graph changes were made.
This situation has led to various "hacks" (see the RFC for links) that are a bit unsatisfying and consume developer time that might be put to better use.
There is no setting today, and no carefully written command, that allows this developer to say "build just crate X, while using the feature unification that would have been used, if I were building the whole application".
Also note that this situation doesn't appear in other build systems, which leads to pressure to move away from Cargo toward systems that always build components the same way.
In case that wasn't clear, it's describing a situation where a workspace contains a large number of crates, that aren't meant to be published on their own, but are sub-components of a big monolithic application.
Really, the user is asking "build package X as if its dependency of Y". I've added this to the Alternatives and why we aren't proposing that at this time and instead going with this.