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

Cargo feature migrations #3146

Closed

Conversation

Ericson2314
Copy link
Contributor

Extend Cargo to allow some limited forms of migrations of feature sets to be expressed.
This will allow new features to be added that make existing functionality optional without causing needless breakage.

Rendered


This second use-case is really important — in fact, perhaps more important.
The key thing aspect about features to realize is that the ultimate *reason* they are used is not to control a crate's interface (exports), but its *dependencies* (imports).
All things equal, no one minds if a crate provides some extra functionality they don't need.
Copy link
Member

Choose a reason for hiding this comment

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

All things are not equal; I care about build time.

Copy link
Contributor Author

@Ericson2314 Ericson2314 Jul 7, 2021

Choose a reason for hiding this comment

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

I changed to "a little". Please bear with me :) in now way does this proposal cause regressions to the first case, so it shouldn't cause an increase in build times. I just merely want to point out my opinion that the relative support for the latter use-case is too small given its relative importance.

Thanks!

Co-authored-by: mbartlett21 <29034492+mbartlett21@users.noreply.github.com>
@kornelski
Copy link
Contributor

kornelski commented Jul 15, 2021

The syntax with cfg expressions and versions has a lot of moving parts. I have a different suggestion that I think achieves the same goal, but has less syntax surface:

  1. I think default-features = false doesn't do its job well, and is the root cause of needless complexity here, so it should be bypassed/replaced. For backwards compatibility, this could be done by adding an ability to define default features that remain default regardless of default-features = false (this could be new features key, or maybe tied to an edition if deprecating default-features entirely).

  2. Add syntax to explicitly exclude a named feature from defaults that get enabled, e.g. features = ["ignore:foo"] or features = ["~foo"] or no-default-features = ["foo"]. This is distinct from globally disabling features, which can't be done in Cargo. It's simply an equivalent of removing a named feature from this one dep's default features set.

This solves the problem with default-features = false removing future features users haven't heard of.

If a dep has [features] default = ["foo"], then user can disable it with no-default-features = ["foo"], and when dep changes to [features] default = ["foo", "bar"], then no-default-features = ["foo"] still does what it did previously. If users of the new version want to disable both features, they can change it to no-default-features = ["foo", "bar"] to explicitly say which features they don't want.

This is also a big usability improvement. Currently, if a crate has many default features and you don't want just one of them, it's verbose and fragile:

[features]
default = ["foo", "bar", "baz", "serde", "std"]

and you don't want foo, you have to write this, and risk disabling future default features:

dep = { default-features = false, features = ["bar", "baz", "serde", "std"] }

but I'm proposing migrating users to this:

dep = { no-default-features = ["foo"] }

Thanks!

Co-authored-by: teor <teor@riseup.net>
@Ericson2314
Copy link
Contributor Author

@kornelski I have tried to address the opt-out approach, and why I considered but rejected it, in the alternative section. I also agree default-features = false bad and proposed (optionally) deprecating it.

@nagisa
Copy link
Member

nagisa commented Jul 15, 2021

I don't see default-features = false being deprecated, especially given the fact that there are a fair number of crates using features in a non-additive way in the first place, and a subset of them specifying one of the mutually exclusive features as a default feature.


I've gotten into a habit of checking, and disabling all default features in my libraries. As the RFC says:

The key thing aspect about features to realize is that the ultimate reason they are used is not to control a crate's interface (exports), but its dependencies (imports).

I don't want to be on the hook for unknowingly enabling some feature and therefore a dependency that some binary down the line could avoid.

What this RFC proposes would effectively force downstream libraries to be maintained and updated every time a crate it depends on adds a feature.

I do want to also nit here that I've seen a fair share of crates attempting to vary their API using features. A most prominent example that comes to mind is a choice of the TLS implementation used by default (which tends to leak into the public API with e.g. hyper).

@djc
Copy link
Contributor

djc commented Jul 15, 2021

I do think @kornelski's proposed direction is better than the proposal in this RFC. IMO the proposed RFC adds a lot of complexity. Responding to the alternatives section from the RFC:

First of all, this complicates the "additive" features mental model. We've worked very hard to teach people how features are additive: it should always be safe to add more features. We can't give that up across-the-board without destroying feature resolution, so it's important that "without necessarily features x or y" doesn't mean the crate will for sure not get x or y, but just that they are not requesting them. I worry this will be very subtle and hard to teach.

That's not a change from the status quo. Obviously this part is a trade-off, but renegotiating the idea of additivity seems pretty fundamental and likely out of scope for whatever is used to solve the present use case.

Second of all, there is an especially unintuitive case of the former where a non-opted-out default feature depends on an opted-out feature. Consider this example:

Consumer depends on "default features without foo".

bar is another default

bar depends on foo.

In this case, the not-mentioned feature still drags in the opted-out feature. This might happen because the user simply forgot to include both features. It might also happen because the problematic bar -> foo dependency only exists in a newer version, and not the one the user was using when they wrote the dependency spec. Finally, it might be that that bar itself, and thus necessarily also the bar -> foo dependency, is newer than the dependency spec. In any case, the result is that all build plans using a problematic version of the package have the explicitly opted-out feature foo, which is liable to confuse the user even more.

This works as designed. If after an update there are new dependencies. This is also not much different from today: if I disable the default features from my dependency but re-enable foo, that might pull in bar in the new release.

One might think there is a fix making the out-out a hard rejection, so such that no plan is allowed to have those features. But that creates other problems, namely that those same sneaky new deps would rule out all plans causing solving to fail. Moreover, this sort of "negative reasoning" undermines the entire "additive" comparability story Cargo features are supposed to have. This will make the overall Cargo solving brittle, and also make it impossible to express the situation where one is in fact, agnostic to whether some unneeded feature is enabled for some other consumer's sake.1

I do think a solution to this problem that might improve related issues is to have some configuration for forcibly opting out of features at the binary level rather than the library level, provided a way to configure this at the workspace level is included as well (for scalability).

Finally, and is a matter of taste, I find writing down features that I don't need poor UX. We say "pay for what you use" in Rust, but writing down features that we, by definition, don't care about means cluttering our minds and Cargo.tomls. I would only want to propose negative reasoning as an absolute last resort.

On the other hand, today we have to exhaustively write down features that are enabled by default if we want to disable only one of the features that are enabled by default, which is also cluttering our minds and Cargo.tomls. IMO it clutters my mind more because I now will have to keep track over time if I haven't missed features which ought to be enabled by default but which I by accident disabled because I wanted to disable some other things.

1: N.B. I do think is useful to be able assert some features aren't enabled. but I that such "global reasoning" should just be allowed in the workspace root, and not dependency crates, for sake of modularity / compositionality.

@ghost
Copy link

ghost commented Jul 15, 2021

Hi, I have literally zero experience in this area, but I did have an idea I wanted to explore as I was following along reading these threads. If this doesn't contribute to the discussion or completely misses the mark, please feel free to mark off topic. I don't want to disrupt the thread. I had to lookup the cargo book to even know what the current syntax for features was. :)

Idea

Have default features become versioned by a simple incrementing number, and have downstream opt out of defaults using that version number. Any time a new feature is added that gates pre-existing functionality, the defaults version number would increment (like a semver bump). The "negative" feature would be marked to indicate that it is such, along with the version where it was introduced. Purely additive features would be added to the existing default features sets without a version bump.

With this information, pre-existing functionality can be put behind a feature gate and then enabled by default, without breaking pre-existing crates that had opted out of default features. In this case, the pre-existing crate will get the new negative feature that is on by default without explicitly requesting it, thus preserving backwards compatibility. Any new additive features that are enabled by default would not be picked up by a pre-existing crate that had opted out of defaults.

This works by having downstream crates that opted out of default features still implicitly enable negative features from a higher versioned defaults set. So opting out of default features at "v0" will still bring in any later negative features that were added at "v1", "v2", etc.

This doesn't enable anything fancy or try to improve default features generally, it's just doing the bare minimum needed for negative features like this to be added backwards compatibly. "Negative" feature is a bit of a misnomer too because they still behave like normal additive features, but I wasn't sure what to call them. They're just features that would be breaking changes to introduce.

Purely mock-up not-actually-valid syntax

Default features versioned sets:

default = ["ico", "webp"]
default_1 = ["ico", "webp", "negative_feature1"]
default_1 = ["ico", "webp", "negative_feature1", "additive_feature2"]
default_2 = ["ico", "webp", "negative_feature1", "additive_feature2", "negative_feature3"]

Opting out of default features:

default-features = false
default-features-1 = false
default-features-2 = false

Identifying features as gating pre-existing functionality:

[features]
negative_feature1 = []
negative_feature1_negative_default_at = 1
additive_feature2 = []
negative_feature3 = []
negative_feature3_negative_default_at = 2

Scenario 1: An upstream crate wants to gate pre-existing functionality behind a new feature flag: "negative_feature1". We're on status quo Rust upstream and downstream, so we'll call our defaults version 0.

  • Upstream: Bump the defaults version to 1 by creating a new "default_1" set with the new feature enabled. Mark the feature as being a negative feature introduced at version 1 with a "negative_feature1_negative_default_at = 1" entry.

  • Downstream, normal defaults: There's been no opt out of default features, so just use the latest default set: "default_1".

  • Downstream, opted-out defaults: Because downstream has "default-features = false" it will only get the features it requested. But in addition it will now also get any negative features from future versions, in this case "negative_feature1" from "default_1". The downstream crate may in fact not need this feature, but including the feature ensures backwards compatibility is preserved. The new feature can then be explicitly opted out of, if desired.

  • Downstream, opted-out defaults and wants to opt out of new feature: If downstream doesn't actually require the pre-existing functionality that is being gated, it can now upgrade to disable the new feature. Change "default-features = false" to "default-features-1 = false" and now "negative_feature1" will no longer be implicitly enabled as it is no longer from a higher version. This is an optional step that only needs to be done if downstream wants to take advantage of a new "slimming down" capability that didn't exist before.

Scenario 2: An upstream crate wants to gate new functionality behind a new feature flag that's enabled by default.

In this scenario nothing about the status quo changes. Add the new feature to the existing default features sets without bumping the version number. Any downstream crates that didn't opt out of defaults will pick it up, and any downstream crates that did opt out of defaults won't pick it up. Because it's a purely additive feature, downstream won't break if it doesn't pick it up.

@Ericson2314
Copy link
Contributor Author

@nagisa

I don't see default-features = false being deprecated...

That's fine, it's a purely optional step.

I've gotten into a habit of checking, and disabling all default features in my libraries.

Woo! I like to hear that :).

I do want to also nit here that I've seen a fair share of crates attempting to vary their API using features. A most prominent example that comes to mind is a choice of the TLS implementation used by default (which tends to leak into the public API with e.g. hyper).

I will revise the motivation a bit. The latter bits were focusing more on that second case of why to use features, and not trying to make blanket statements about how they are used in all cases.

In this specific one,

I don't want to be on the hook for unknowingly enabling some feature and therefore a dependency that some binary down the line could avoid.

What this RFC proposes would effectively force downstream libraries to be maintained and updated every time a crate it depends on adds a feature.

I don't understand this? This is definitely not my intent. Perhaps i have made some mistake.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 17, 2021

Can we get a rigorous definition of "base version of the dependency spec"? What does that mean for all of the semver operators X leading zeros X prerelease versions?

Does it matter how the migrations from old versions change over time? E.g. how the migrations from 1.1 -> 1.2 -> 1.3 might not match the migrations from 1.1 -> 1.3 directly?

As the person that understands Cargos dependency resolver the best, I am not shore. And this needs to be thought about very carefully. The fact that different versions can have different requirements is what makes Dependency resolution NP-Complete. This may make the problem O( numbers of packages considered ) harder to solve and explain, or it may be a surface level problem that has no theoretical impact. I will think about it.

Public and private dependencies

are all challenging problems that are amendable to this sort of analysis. ... that there is some sort of holistic approach for both deciding on a fundamental level what we want from Cargo

The problem with "Public and private dependencies" is not the thery. It is fairly clear what we want it to mean. It is implementation that is the problem. In a SAT/SMT reduction of Dependency resolution it adds O( (numbers of packages considered) ^ 2) terms to the problem. More importantly in the Resolver as implemented it takes common use cases from sub second time to multiple hours time. There are a lot of changes to the Resolver that we can not stabilize because of limitations to performance or error messages even though we have a clear understanding of exactly what behavior we want and why.

@Ericson2314
Copy link
Contributor Author

@Eh2406

Can we get a rigorous definition of "base version of the dependency spec"? What does that mean for all of the semver operators X leading zeros X prerelease versions?

Good question! I do not know all the myriad types of version specifiers off hand. If you don't mind, I rather punt on these details until we've established whether we want to do anything like this at all. There is always an escape hatch of the user specifying a base version explicitly if it would be ambiguous (e.g. if there isn't a unique lowest pre-release version matched by the spec). Is that OK with you?

As the person that understands Cargos dependency resolver the best, I am not sure...I will think about it.

Thanks! I am very curious about this.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 18, 2021

Good question! I do not know all the myriad types of version specifiers off hand.

The new Semver 1.0 crate makes this much easier to deal with! Here is the list of ops. Thanks to everything being pub in the new versions of Semver, we can make a fn base_version(req: &VersionReq) -> Version anyware we want. I think all the details will need to be documented at some point. But you are right, we can punt for now.

Does it matter how the migrations from old versions change over time? E.g. how the migrations from 1.1 -> 1.2 -> 1.3 might not match the migrations from 1.1 -> 1.3 directly? (Note that multi-step migrations might be possible if Cargo combines depenency specs to avoid duplicate crates / public dep violations mid-solving.) The "path dependency" of migrating different ways could make for unexpected behavior as the solver tries different versions.

Having taken a walk, I now think it is a surface level problem that has no theoretical impact. The thing I remembered is that we already need to do computation to figure out what the set of features from the Dependency object mean when looking at a particular package version (here), now the code needs to figure out what the features mean for this package version in light of the "base version".

Prior art

Do other package managers have a system like this? How do they deal with the stability hazards of moving code to opt in flags?

@epage
Copy link
Contributor

epage commented Oct 10, 2022

While we haven't written up and RFC yet, there are some of us interested in @kornelski idea. For now, it is being discussed in rust-lang/cargo#3126.

@epage
Copy link
Contributor

epage commented Oct 10, 2022

. If you don't mind, I rather punt on these details until we've established whether we want to do anything like this at all

For me, the RFC's ambiguity around versions vs version requirements and how "base versions" work makes it hard for me to give a clear answer. My gut says that this RFC is a bad idea from my vague understanding of this RFC but I can't fully articulate the thought and make sure I'm responding to your idea instead of some other idea I created when trying to read this.

In case this can short-circuit some discussion, I'll share my thoughts based on my interpretation of the RFC: I feel like this RFC is trying to make decisions of what is happening based on the user's version requirement based on rules in the package in the lockfile.

The most immediate concern I would have is that modifying a version requirement to a compatible version would be a breaking change because you are now missing the migrated features. If we had cargo upgrade merged, then the RFC could say "apply the migration directly to the Cargo.toml on upgrade" but I don't think we should say "you have to use cargo upgrade to avoid breaking changes between compatible versions". That leaves out hand edits, editor integration, Dependabot, etc.

@Ericson2314
Copy link
Contributor Author

@epage that's a fair point. Tightening bounds would have new unanticipated effects.

@Ericson2314
Copy link
Contributor Author

I'm going to close this as I have a new simpler idea I like better as a first step.

@Ericson2314
Copy link
Contributor Author

#3347 Here's a new attempt on this problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants