Skip to content
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 3283: Backward compatible default features #3283

Closed

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jun 23, 2022

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jun 23, 2022
[reference-level-explanation]: #reference-level-explanation

A new key `package.feature-bases` is introduced. This key specifies a list of
features that, augmented with `default` and `default-features-false`, are
Copy link
Contributor

@epage epage Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies new semantics to a feature name (default-features-false) that was not previously reserved. What, if any, backwards compatibility concerns do we have about this?

Comment on lines +72 to +74
[dependencies]
your_crate = { version = "5.7", base = "minimal_v1" }
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the impact on cargo add?

cargo add is about to be released on stable and with new manifest features, we should consider if they should be exposed in cargo add and how.

approach, `default-features = false` implies the feature base called
`default-features-false`.

# Reference-level explanation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the impact to the Index?

For example, see the explanation of index changes for weak and namespaced features

My main concern is that if we have a disruptive index change that we provide a better experience than we did with weak and namespaced features, see rust-lang/cargo#10623

@woshilapin
Copy link

The problem that this RFC solved can already be solved with semantic versioning, isn't it? Adding a new default-feature in a crate could be seen as a breaking change (for users having default-feature=false).

Unless I'm missing something, I feel using semantic versioning with a new major version should be mentioned in the Alternatives section at the very least.

@Lokathor
Copy link
Contributor

Lokathor commented Jul 7, 2022

yeah but you can just go turn on the feature and you're back in business, so it's not a big deal.

@tbu-
Copy link
Contributor Author

tbu- commented Jul 9, 2022

The problem that this RFC solved can already be solved with semantic versioning, isn't it? Adding a new default-feature in a crate could be seen as a breaking change (for users having default-feature=false).

Semantic versioning isn't a solution to this. Semver-incompatible changes should generally be avoided, almost at all costs. This is evidenced by the standard library which also has this very problem and will definitely not consider breaking semver to introduce a new on-by-default feature.

@Nemo157
Copy link
Member

Nemo157 commented Jul 11, 2022

This is evidenced by the standard library which also has this very problem and will definitely not consider breaking semver to introduce a new on-by-default feature.

The standard library is a special case. There are a handful of core crates in the ecosystem that really must avoid semver breaking changes, but in most cases it's entirely possible to release new major versions with the only downside being a period of extra build time because of the duplicated crates until everyone updates.

@epage
Copy link
Contributor

epage commented Jul 11, 2022

Another perspective on this RFC is that it is adding another dimension to semantic versioning. We have a breaking version number at the package.version level but now we also have breaking version number package.feature-bases level.

The intended value of this feature is allowing decomposing the breaking version number into smaller pieces to offer users some extra stability as changes are made. This is somewhat similar to the case of a crate providing a subset of the API in "core" crates and us merging #3243 to allow better composing of these crates that would normally be re-exported.

The question for me is whether it pulls its weight when considering

  • benefits of avoiding bumping the breaking version in package.version
  • Scope and impact of crates needing versioned "default/default-features-false`
  • users learning to deal with another "version"
  • users managing another "version"

feature base like `base = "minimal_v1"`.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will the cargo user that currently uses --features, --all-features, and most importantly --no-default-features interact with this new feature?

I'm assuming cargo will need a --feature-base flag.

well-formed if it is in the above-mentioned set of possible bases.

The keys `default-features` and `base` on dependencies are mutually exclusive;
`default-features = false` implies `base = "default-features-false"`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A caution with this is there will no longer be a guaranteed way of getting the minimal set of features. Users will have to know what all is happening within the bases to get a minimal set.

Copy link

@ssokolow ssokolow Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...which could cause Public Relations issues, given that you already see people complaining about build times and insufficient tooling for help identifying what to turn off because you don't need it in order to reduce build times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insufficient tooling for help identifying what to turn off because you don't need it in order to reduce build times

One small step for improving this is cargo add raises the visibility of what features are enabled by default.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

A new key `package.feature-bases` is introduced. This key specifies a list of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A draw back to this is the bases are free form and we can't do tooling to migrate users. Maybe a more concrete syntax with at least a version field would help.

If we also restricted the names to go with the versions, that might help with

  • Developers providing a consistent set of bases (minimal and default)
  • Users discovering the base they need since crates won't have different conventions
  • Discourages using bases as feature groups (if we'd want to do that)
    • If people want versioned feature groups then that might suggest we need general feature versioning and not a new "base" concept

@tbu-
Copy link
Contributor Author

tbu- commented Jul 11, 2022

The standard library is a special case. There are a handful of core crates in the ecosystem that really must avoid semver breaking changes, but in most cases it's entirely possible to release new major versions with the only downside being a period of extra build time because of the duplicated crates until everyone updates.

I disagree. Every crate should strive to maintain backward compatibility unless there's some very good reason not to. Of course, it's possible to release a new major version, but it comes with the significant disadvantage that reverse-dependencies needing to upgrade, and sometimes that traits and types from major versions are incompatible without using the semver trick.

@epage
Copy link
Contributor

epage commented Jul 11, 2022

I disagree. Every crate should strive to maintain backward compatibility unless there's some very good reason not to. Of course, it's possible to release a new major version, but it comes with the significant disadvantage that reverse-dependencies needing to upgrade, and sometimes that traits and types from major versions are incompatible without using the semver trick.

Yes, every crate should follow semver in that a breaking change bumps the major version.

Now how much crates avoid bumping major versions is dependent on scope/impact. Some crates serve as "vocabulary terms", meaning they are core to cross-crate communication, like regex, tokio, etc. These need to minimize major version bumps due to splitting the ecosystem.

However, most crates don't fall into that category get used more directly and have little interop, reducing the impact of a breaking change. nom is a great example of this. Unnecessarily burdening these crates with pressure to not bump major versions can have a major dampening effect on the Rust ecosystem from growing.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Oct 10, 2022

I would just like to point out that a feature base is the same as a feature. If we required the feature list to never be empty (e.g. if no default features had to be used with at least one feature) we wouldn't have this problem.

I'm happy to admit maybe their are simpler solutions than my old RFC (which frankly I had all but forgotten about) but it would be nice to e.g. in a future edition have a simpler Cargo.toml spec.

@joshtriplett
Copy link
Member

We've discussed this in a few @rust-lang/cargo meetings. In particular, we're especially not in favor of the idea of having the version specified in Cargo.toml affect feature resolution, as opposed to the version actually selected by the resolver.

In general, we feel like we're favoring an approach more like rust-lang/cargo#3126 rather than this approach. (That still needs some details worked out, though.)

@rfcbot close

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 1, 2022

Team member @joshtriplett has proposed to close 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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Nov 1, 2022
@tbu-
Copy link
Contributor Author

tbu- commented Nov 1, 2022

In general, we feel like we're favoring an approach more like rust-lang/cargo#3126 rather than this approach. (That still needs some details worked out, though.)

It seems to me that the approach outlined in the linked issue will not allow people (or the standard library, for that matter) to add new features for existing functionality, because it only adds a way to specify dependencies. This means that you cannot make more parts of your library optional without it being a breaking change, because people might rely on their default-features = false build compiling.

In general, when adding a new default feature to a crate, there seem to be two cases:

  1. The feature can be used (by deselecting it) to disable already-existing functionality. In this case, existing crates not knowing about the feature should keep it enabled.
  2. The feature can be used to add new functionality. In this case, existing crates trying to disable default features and not knowing about the feature should probably have it disabled as well.

The current default-features = false enables use case 2, the linked issue would enable use case 1 if default-features = false were to go away, but this will never be the case (as I understand it). This means the linked issue will not actually enable library creators to use default features in a new way. default-features = true cannot work with use case 1 (it'll result in a compiler error) and the linked issue cannot work with use case 2 (it'll result in feature bloat).

In particular, we're especially not in favor of the idea of having the version specified in Cargo.toml affect feature resolution, as opposed to the version actually selected by the resolver.

I see. I think this makes sense. I think it also implies that not both of the use cases 1 and 2 can be satisfied at the same time.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Nov 1, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 1, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔


In order to introduce this in a way that works with the existing
`default-features = false` syntax, `default-features = false` will imply a
feature base, `no-default-features`.
Copy link
Contributor

@teor2345 teor2345 Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the name was changed elsewhere, but not here?

Suggested change
feature base, `no-default-features`.
feature base, `default-features-false`.

@ijackson
Copy link

We've discussed this in a few @rust-lang/cargo meetings. In particular, we're especially not in favor of the idea of having the version specified in Cargo.toml affect feature resolution, as opposed to the version actually selected by the resolver.

This comment is very confusing to me. With the proposal here in this RFC, there is no strange dependency on the version specified in the depending package's Cargo.toml. That strange dependency is a property of #3146, not this MR.

The proposal in this MR is admirably simple and seems to resolve all the relevant issues quite well. It seems to me to be better than #3126 which is now apparently being suggested as the alternative.

I suspect there is some confusion about which proposals do what. I wonder if maybe the @rust-lang/cargo discussion was actually about #3146, not about this proposal. In any case, I think the stated rationale for closure doesn't make sense.

@epage
Copy link
Contributor

epage commented Nov 10, 2022

@ijackson while the reason listed was mixed up with #3146, we have been talking about both and our general expectation for which direction to go still applies.

I can't speak for the others but I feel this proposal adds a lot of upfront complexity for people to depend on a crate when bases are being used.

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 10, 2022

I will admit that I got the RFC discussions confused, and so was the source of the erroneous explanation. Sorry.

Being confused by what all the options are, how they interact, and what impact each one will have is not uncommon when discussing the features system. Xavier Denis and I have been discussing creating a mathematical model of the features system so as to have rigorous definitions of the proposals. If that discussion goes anywhere, this proposal should definitely be added to the list of things being modeled.

@tbu-
Copy link
Contributor Author

tbu- commented Nov 10, 2022

I can't speak for the others but I feel this proposal adds a lot of upfront complexity for people to depend on a crate when bases are being used.

I can see how it adds complexity for the people creating feature bases, creating a dependency for others. Can you describe why it makes it complicated for users of the dependencies? They can continue to use default features, or instead use a different feature base. Obviously only one that existed for the version they depend on, but that's already true for features, they can get added in later versions.

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label Nov 11, 2022
@rfcbot rfcbot added to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 11, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 11, 2022

The final comment period, with a disposition to close, 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 is now closed.

@rfcbot rfcbot added closed This FCP has been closed (as opposed to postponed) and removed disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Nov 11, 2022
@rfcbot rfcbot closed this Nov 11, 2022
@Ericson2314
Copy link
Contributor

#3347 Here's a 3rd attempt on this problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed This FCP has been closed (as opposed to postponed) finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.