-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Give users control over feature unification #3692
Conversation
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: 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. |
3. Resolve features | ||
4. Filter for selected packages | ||
|
||
**Features will be evaluated for each package in isolation** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out! I had considered this but forgot it and didn't want to hold it up until I could remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sunshowers says cargo-hakari
will unify normal and dev-dependencies. Platforms are not unified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sunshowers says that is also unifies the features that workspace member features enable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've summarized this under "Future possibilities"
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.) As additional observation: The 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. |
That's wrong, this feature will affect all dependencies of your own workspace. So people will (naturally) go to upstream crates and request that this or that works if the build is "suddenly" broken. That is especially an issue if the feature is promoted as something that significantly improves compile times, because then it feels for the workspace author like: I really want to improve my compile times but I cannot because of the upstream crate xyz. That will naturally result in a certain pressure towards to the maintainers of crate xyz. |
As I understand it, the problem already occurs today when you run a cargo command on the whole workspace, right? That’s not a rare thing to do: with a virtual manifest as workspace root (not the only option, but a common one), it affects pretty much every manual cargo invocation from the root directory without passing In the terms of rust-lang/cargo#14415, it seems to me that an easier workaround is for the user to change their crate(s) so they always depend on “a” with the same set of features both on the host and the target. This is not great but (when applicable) requires less ongoing effort from developers than avoiding whole-workspace invocations every time they interact with cargo. |
I don't have the feeling that anyone even tries to understand what's the underlying problem here. Yes you can hit the same problem today, as written before that's already a previous breaking change from the cargo team. In my personal opinion they shouldn't really expose more ways to do that until they started working on a fix for that previous regression, but at least @epage already dismissed that. (So much to the stability guarantees…) To write that again: My main concern is: Currently this is mostly in the line off "Well figure out your workspace crates on your own", but that changes as soon as the team actively promotes this feature as "Enabling this feature will speed up your compile times significantly", which is exactly what this RFC does as main motivation. This communication changes the expectation of users on their dependencies from "Well this won't work for me so I just change something on my side" to "This now slows down my build, you need to fix that". Please understand that this is really not a desirable situation to be in as crate maintainer. The alternative that I as crate maintainer see is to tell exactly these users to go and yell at the cargo team instead of yelling at me + explicitly document on diesels side that this setting is unsupported from our end due to the potential breakage outlined above. I'm quite sure the cargo team would prefer as well to not have that situation. Now again, I think that's not hard to address from the RFC author/cargo team. Possibilities are to tweak their communication so that:
And sure, you can all dismiss this, but again I think the rust ecosystem already has a large enough burnout problem, so there is really no need to introduce more reasons to burn out there.
That's not desirable as soon as you have native dependencies behind these feature flags. |
As I've said before, the RFC does not actually focus on this as a general "build accelerator" feature but as a "keep consistent behavior in scenarios you are already running", see Motivation
See Drawbacks which also re-iterates the performance point above. |
I'm sorry for giving this impression. I should have been much clearer about how my comment relates to what you're saying. Tacking on an afterthought (the second paragraph) which doesn't related to any of your main points didn't help. Let me try to rectify that: I'm trying to probe to what extent and in which sense the proposed I cannot comment on your other points: I'm in no position to edit the RFC, I'm not on any relevant team, and I don't know the inner lives of your users -- by which I mean, I can't and won't speculate whether/how the presence of |
@hanna-kruppe I've taken a step back end tried to reflect on all arguments given so far. I think there are two points that get mixed up all the time here. The first point is that I personally still consider the proposed change a breaking change. I've gave an example above that works to build in a certain directory. It wouldn't work anymore if the new setting is set. Now that can be set as explicit argument, but it can also be set "implicitly" by the environment, e.g. by a global environment variable or by a "os level" setting (e.g. in This brings me to my second more important point. I think we all agree on the point that this might expose the "existing breakage" on more locations. As previously explained I do not see that all users necessarily understand the potential impact of this new feature unification setting, especially in such settings as described above. As long as this would be yet another obscure cargo setting, even that would likely be fine. I personally fear that the cargo team is going to promote this as "Enable this setting and get faster builds" feature, which to be fair might be even true for most setups. But if they do that, it suddenly shifts from being yet another obscure settings that is not used often to something that might be popular (as rain pointed out above, for example). That certainly will lead to pressure on the maintainers of crates that cannot support this for the reasons outlined above. That's why I'm asking for an addition to the RFC that explicitly states that preventing this is the responsibility of the cargo maintainers. I don't even think that this is much of a burden for them, as it's mostly a communication/documentation thing, although ideally they also would emit a message for candidates of such failures that just point to the corresponding documentation. The alternative is to not promote this feature as something that improves performance and turn it into yet another obscure setting, which would be even less work. I personally see the second point and the proposed resolution as important. I still believe that this might be a breaking change, but I see that others might disagree there and I even agree that this edge-case is probably not worth blocking the whole feature over. At least as long as the second point is addressed in a meaningful way. |
I feel you're attributing too much importance to what the |
If I'm reading this RFC correctly, this will only affect feature unification for direct dependencies of packages in the workspace and won't affect the feature sets for indirect dependencies. This means that if a workspace package uses the dependency |
It's always easier for maintainers if they can just point to the official documentation and say: It's written there, that's expected behaviour. That official documentation is something the cargo team can clearly provide. The second suggestion I made was to include this information in the emitted error messages, which would make this information even more prominent for users. For me that doesn't sounds like something where the cargo team does not have any influence, but quite the opposite: They can provide helpful responses without much effort.
As pointed out that is not correct. As soon as you unify features of direct dependencies you change the compile feature resolution for all dependencies. |
|
That's does not even touch any of my concerns. It says the following:
For the example provided there are no mutually exclusive features. The features in question are additive by all common definitions that I'm aware of. You keep saying that this won't work for mutually exclusive features, but I really do not see how that's even the point of the discussion. Nobody claimed that. So can you please point out where exactly the drawback section talks about the case presented above? Additionally I do not even see any mention around the potential social pressure there. Do I miss something or do you still claim that this won't be a problem. If the later is the case: On which foundation do you believe you can claim that and dismiss my personal experience with previous similar features? At this point I have the feeling that you either do not want think about all implications or that you mix up what mutually exclusive features are and what not . Both would be highly concerning for a team member in the cargo team.
The critical mode is the
For me that reads like it will not only unify features for direct dependencies, but for all dependencies of the workspace. It still won't unify features between host and target dependencies, but it will unify them for each of those on it's own. Exactly that behavior is what would be problematic with the example provided above. |
For me, the RFC section you quoted reads like features will always be unified across the top-level workspace like they would be with |
This "workspaces of dependencies" concept you're talking about doesn't really exist. Any given crate either is a member of the workspace that the user is working in right now, or it's not. In the most common case where non-workspace-members come from the crates.io index, they're self-contained and the rest of the workspace that it may have been developed in isn't even available. Path and git dependencies presumably may have to refer to their respective workspace Cargo.toml to resolve fields that are inherited from the workspace, but I don't think Cargo pays very much attention to those other workspaces afterwards. In any case, this has nothing to do with Diesel being affected by rust-lang/cargo#14415. As you can see from the minimal example in #3692 (comment), the relevant feature unification is between two members of the "top-level" workspace depending on |
I think you're both right: Regardless of this new setting: Cargo will unify the features of all compilation units (direct and indirect dependencies). The only thing this setting changes is which crates are included in this set of compilation units. To me @Freax13' interpretation sounds more accurate though. In the case of mutually exclusive features you of course can only use
There also is literally no way a dependency with mutually exclusive features could fix this. You also cannot compile diesel with I do not think it's feasible for the error/warning message to contain this information, as there is no "official" way to mark two features as mutually exclusive. But there is still the option to output a The only other way I can see for this would be to either:
Personally I think this is fine as long as |
@DragonDev1906 I'm not sure why you bring up mutually exclusive features again. As already written several times before: In the provided example are no mutually exclusive features so at least my concern is not around mutually exclusive features.
Exactly this is wrong. @epage Claimed that without any providing any evidence. You can compile diesel with So yes your statements are correct for mutually exclusive features, but this discussion is about another build error that occurs without mutually exclusive features. |
…utually exclusive features is just an example
@epage Thanks for updating the drawback section, it now reads at least remotely like it covers the presented case. Would it be possible to get an official response from the cargo team to my concern that crate maintainers could be the target of considerable social pressure due to the need to support the workspace unification variant for promised build speedups although that's not possible due to one of the listed drawback variants? |
Is crate maintainers referring to library maintainers (i.e. dependencies) or binary maintainers (i.e. top-level projects)? If it's about library maintainers, I still don't get how this feature makes the situation worse and how a maintainer would possibly fix this. Can you provide an example of a repo that would be affected and how it could be fixed? |
It refers to the library/dependency crates.
This RFC describes as motivation for the workspace unification variant that this might speed up builds. My main fear here is that maintainers of a library crate can end up in a situation where some of their users demand to support this specific build mode in their setup, while that's technically not possible. Given how heated discussion around build times can become that might result in considerable pressure to theses maintainers, without giving them a way to solve the underlying problem. The current situation is I'm so far different that this issue is not seen as something that's related to build times. To summarise that: It's more a concern around the social impact, rather than a hard technical issue, although this new feature can result in failed builds in additional locations.
For a affected repo: See the example provided before For how to fix it: It's not possible to fix this to my knowledge, which is exactly the point I'm trying to make. |
I think it's worth reiterating out that the Personally, I'm not worried about workspace feature unification across a top-level project breaking a build. I would be concerned if workspace feature unification across the workspace of a dependency breaks a build, but it seems to me like we are all in agreement that this won't happen, because features are not unified across the workspace of a dependency. |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
Thanks everyone for the comments! To track further discussion, subscribe to the tracking issue here: rust-lang/cargo#14774 |
This adds
resolver.feature-unification
to.cargo/config.toml
to allow workspace unfication (cargo-workspace-hack) or per-package unification (cargo hack
).Rendered