-
Notifications
You must be signed in to change notification settings - Fork 609
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
Add support for new feature syntax (RFC 3143) #3867
Conversation
r? @smarnach (rust-highfive has picked a reviewer for you, use r? to override) |
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.
Looks good except for a minor concern
☔ The latest upstream changes (presumably #4096) made this pull request unmergeable. Please resolve the merge conflicts. |
Gentle ping? I think this should be labeled as waiting on review 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.
I can't attest to the validity of the changes as a whole; I'm not familiar with the internals. The parsing of the feature seems correct. Just one minor nit on style.
FWIW we discussed this on Discord a few days ago: https://discord.com/channels/442252698964721669/448525639469891595/920823792631152691 the tl;dr is that we show |
I'm not sure how you'd like to verify that. I could add something that did a database query in the The feature values themselves don't seem to be displayed in the UI: I can confirm from the API that it is there:
And via psql:
|
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.
The feature values themselves don't seem to be displayed in the UI
you're absolutely right. now that I finally had time to take a detailed look at the RFC and PR changes, I agree that the frontend shouldn't be affected by this. since the split between features
and features2
only happens for the index, but is otherwise kept in its merged form I don't see any issues with it.
nice work! and sorry for the huge review delay... 😞
@pietroalbini @jtgeibel unless you have any objections I plan to merge this on 2021-12-30 (one week from now), or earlier if you both signal your approval until then :)
Co-authored-by: Jacob Pratt <jacob@jhpratt.dev>
@bors r+ |
📌 Commit 2b4430c has been approved by |
☀️ Test successful - checks-actions |
rebased, merged and deployed. thanks for working on this @ehuss! |
This adds support for the new feature syntax described in RFC 3143. There are two new feature values that are allowed in the
[features]
table: weak dependencies (foo?/feat-name
) and namespaced dependencies (dep:bar
). When these style of features are detected, they are placed in the index in a separate field calledfeatures2
to prevent older versions of cargo from breaking on them. Additionally, it sets a new version field calledv
to indicate that this new field is present, which prevents versions starting at 1.51 from selecting those versions.