-
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
Cargo feature migrations #3146
Cargo feature migrations #3146
Conversation
|
||
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. |
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.
All things are not equal; I care about build time.
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 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>
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:
This solves the problem with If a dep has 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 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>
@kornelski I have tried to address the opt-out approach, and why I considered but rejected it, in the alternative section. I also agree |
I don't see I've gotten into a habit of checking, and disabling all default features in my libraries. As the RFC says:
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). |
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:
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.
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.
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).
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.
|
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. :) IdeaHave 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 syntaxDefault features versioned sets:
Opting out of default features:
Identifying features as gating pre-existing functionality:
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.
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. |
That's fine, it's a purely optional step.
Woo! I like to hear that :).
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 understand this? This is definitely not my intent. Perhaps i have made some mistake. |
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?
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
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 |
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?
Thanks! I am very curious about this. |
The new Semver 1.0 crate makes this much easier to deal with! Here is the list of ops. Thanks to everything being
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".
Do other package managers have a system like this? How do they deal with the stability hazards of moving code to opt in flags? |
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. |
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 |
@epage that's a fair point. Tightening bounds would have new unanticipated effects. |
I'm going to close this as I have a new simpler idea I like better as a first step. |
#3347 Here's a new attempt on this problem |
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