Tracking Issue for workspace `feature-unification`

ehuss commented 4 weeks ago

Summary

RFC: #3692 Implementation: TODO Documentation: TODO

Adds the resolver.feature-unification configuration option to control how features are unified across a workspace.

Unresolved Issues

[ ] Bikeshed the config option naming.

Future Extensions

Support in manifests
Dependency version unification
Unify features in other settings

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

weiznich commented 4 weeks ago

Given that my concerns on the RFC were ignored I will repeat them here as well.

This feature can result in build failures in upstream dependency crates outside of the workspace if the workspace feature unification is used.

The charge team should make clear in the document and communication around this feature that:

This can happen and might not be fixable due to other deficiencies in cargo and the wider rust ecosystem.
This is entirely up to the user that sets this setting and not to the upstream crate
Ideally these information should be presented by cargo as part of the compiler error message

Also as I personal note I want to emphasise again that I'm highly disappointed how the cargo team seems to handle concerns and suggestions to resolve these concerns. I'm also disappointed how the team decided to (not) communicate their decision. For me that doesn't look like a team that's interested in outside contributions. For me personally that means that I will likely not contribute to cargo anymore, including taking care of concerns from the cargo team for potential concerns for features I design/work on. I also cannot recommend other persons to contribute to this team anymore. That effort is likely better spend somewhere else.

weiznich commented 4 weeks ago

Whoever marked my comment as duplicate: Could you point to the "duplicated" comment in this thread?

Also could you point to an official answer to these suggestions from the cargo team and not from individual team members? I'm really not sure what you are expecting to happen if you just keep pretending to ignore meaningful suggestions.

epage commented 3 weeks ago

As you said, this was repeating content from the RFC discussion. I gave some replies in the RFC. The Cargo team saw the discussion and signed off on the RFC without raising concerns. This was re-iterated by Eric in the thread you opened on Zulip, see https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/Offical.20team.20response.20to.20concerns.20raised.20in.20RFC-3692/near/479039846

As this has come up before and is re-litigating the RFC, I collapsed it to get it out of the way of the discussion for implementing the RFC. It is a big help to be able to have Issue discussions more focused using tools like that. Some other teams go to the extreme of locking Tracking Issues, reserving them solely for status updates.

weiznich commented 3 weeks ago

@epage I still do not see where exactly my concerns about the potential social impact are addressed. Can you point to the exact location, including an explanation why it's not possible to have this slight adjustment to the wording to make clear that the communication around this feature needs to be aware of these edge cases. Given how other parts of the rust project usually try to have the best possible diagnostic for error cases I really cannot understand why you and the cargo team continues to just ignore these suggestions.

All I got so far is reiterating why you don't consider this a breaking change. I got your point there the first time, I just disagree with it. Even that does not mean that I'm the opinion that you shouldn't do this feature, which is why I literally trying to suggest for more than a month now that this likely can be resolved by just having that diagnostic/documentation note explicit in there.

epage commented 2 weeks ago

As for implementing this, I think its ok to implement each config value (and maybe stabilize) separately.

For all of this, the top-level function for where all of this lives is https://github.com/rust-lang/cargo/blob/e858736b561805af163ad9dbe0dbd30aa889701f/src/cargo/ops/resolve.rs#L137-L269

Keep in mind that we have two resolvers (dependency, feature) and multiple phases

Resolve dependencies for the lockfile (resolve_with_registry)
Resolve dependencies to strip out unrelated packages, unspecified features (resolve_with_previous)
Resolve features (FeatureResolver::resolve)

To make workspace work, we'll likely need to duplicate the logic from (2) to be able to run it after feature resolver so that we can resolve features for unselected packages. The problem with outright moving the logic from the dependency resolver to the feature resolver is that, under various circumstances, we allow Cargo to combine (1) and (2) so that dependencies don't need to be vendored or that we can skip downloading git dependencies for the sake of deciding not to use them. This is what the other two cases at the beginning of resolve_ws_with_opts are about.

The big risk is that we do the filtering differently between the the dependency resolver and the feature resolver.

As for package, a hacky way to implement that is to do back-to-back compilations. The downside is the redundant work. Ideally, FeatureResolver::resolve could just handle this. The main question I have is how well will it scale to have these additional disjoint feature graphs. There might need to be work done to handle that.

weiznich commented 2 weeks ago

@epage I'm still waiting on your response to my question above.

I'm also highly disappointed that the cargo team just does not seem to care about good diagnostics at all.

rust-lang / cargo