Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow requiring "at least one feature" #3347
Allow requiring "at least one feature" #3347
Changes from 3 commits
5f064ba
7b90ea3
4c452e1
c37e11b
6168951
1c25cd9
fcf2cc5
bfc3105
9e61631
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It would be nice if this could provide a way for existing crates to move over to the new system without breaking compatibility. Perhaps add a
no-feature-selected = "feature"
key that allows to specify the single feature that is activated when no others are?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 don't think it breaks compatibility. Because the solver knows to reject plans where the no-feature dep is mis-resovled, there are no issues. Not all downstream uses can upgrade, but all that can will.
In general I think it's good to think of version bumps as the residual of thing that the plan solver doesn't already know how to detect. Bumping a version bound is saying "sorry there is a problem I don't know how to directly say what it is, so I am going to insert this barrier instead".
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 don't exactly understand your language here. Do you mean that cargo will not update a crate's dependency to a version with
at-least-one-feature = true
, if the depending (not dependent) crate does not specify a feature for the dependency?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.
Exactly! Thanks for saying that more clearly :)
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.
That still sounds like a breaking change upgrade, even if you are protecting people from doing the upgrade.
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.
Periodically trying to address concerns in the comments and then opening new threads I think is a good way to keep the conversation manual. Perhaps my "
at-least-one-feature = true
is not a breaking change" explanation is all wrong, but I think the first-order problem is the RFC originally neglected to discuss the issue at all. Now at is at least discussed, and so problems with that discussion (and I think it's better to poke holes in the RFC version than the version in the comments) are to me follow-up concerns.All that said, if you still rather continue the conversation OK I will accept it.
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.
For me, the topic is breaking changes and spreading that out across multiple threads can make that harder to follow.
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.
OK Then, I'll keep it here.
I went with
MSRV
because it is an example of something we used to not have, so it is nice to illustrate the point about "residual" breakage. But it not the only option. (Also once rust-lang/cargo#9930 is added I don't think the "build environment" argument makes sense. Version bounds (only a lower bound because we assume there is no breaking changes in rustc) that effect version solving are bona fide dependencies, are they not?)Is increasing a dependency bound by a narrow version a breaking change? I think the answer is also "no". Anyone stuck on the old version (private deps make this a bit harder to imagine, but public deps reveal it) will be unable to upgrade, but no breakage will occur.
Moreover, we can call this a breaking change, and that a breaking change, but does anyone benefit from bumping the version number? No. Those that already couldn't upgrade still can't. And those that previously could upgrade now also can't. "breaking" nomenclature aside, the practical difference is simply to rule at more perfectly good build plans.
See what I wrote in #3347 (comment). I think one thing worth taking from @tbu-'s RFC's is basically a one-off migration/ret-conning of
default-features = false
. Then we side-step is controversy entirely. The restriction in this PR ensures that the "one off" approach is sufficient --- since going forward there won't be newdefault-features = false
users we don't have to worry about that cropping up again.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.
MSRV is a very different case that, just because we want to improve the experience for it, does not mean we take it as a license to do similar elsewhere.
I would consider it a breaking change if a crate switched from
foo = "1.0"
tofoo = "1.0;<1.3"
which is effectively what we are talking about here.This doesn't just affect "really old" crates where we can assume no one should upgrade any dependency. People upgrade dependencies piece meal. Some times they have a reason to hold back.
As I pointed out elsewhere, its not just a matter of gracefully being blocked from upgrade this dependency but being blocked from upgrading the dependency anywhere in the dependency tree. I can't depend on an unrelated crate that needs a newer version of the crate that made the switch over.
Incorporating my idea elsewhere to encourage people to use this feature via
cargo new
defaults and edition defaults at least improves this by moving this away from a "only enabled once someone needs it" to "its enabled defensively".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.
Yeah I do like that part, to be clear.
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.
How? Is it supposed to just pick a feature and enable that?
I am also confused by the paragraph on why adding
empty-features = false
is not a breaking change, which is probably related to my confusion about this sentence.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 just meant if no feature is enable than the plan is invalid, and it must throw it out. (If all plans don't enable any feature for a
empty-features = false
crate, then no plans are valid)Hehe I didn't even think of that. I suppose it is quite easy to take any such invalid plan and make it valid in this way, but yeah, ew.
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.
Oh I get it, so the reason this is semver compatible to add is that cargo will refuse to use the newer version because that would violate this condition -- so it is stuck with the old version?
That makes sense, but could be explained in a bit more detail to make it more clear.