Skip to content

Add support for new feature syntax (RFC 3143)#3867

Merged
bors merged 3 commits into
rust-lang:masterfrom
ehuss:features-v2
Dec 30, 2021
Merged

Add support for new feature syntax (RFC 3143)#3867
bors merged 3 commits into
rust-lang:masterfrom
ehuss:features-v2

Conversation

@ehuss

@ehuss ehuss commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

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 called features2 to prevent older versions of cargo from breaking on them. Additionally, it sets a new version field called v to indicate that this new field is present, which prevents versions starting at 1.51 from selecting those versions.

@rust-highfive

rust-highfive commented Aug 25, 2021

Copy link
Copy Markdown

r? @smarnach

(rust-highfive has picked a reviewer for you, use r? to override)

Turbo87
Turbo87 reviewed Aug 26, 2021
View reviewed changes
Comment thread src/git.rs
@Turbo87 Turbo87 added A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works labels Aug 27, 2021
jtgeibel
jtgeibel reviewed Sep 3, 2021
View reviewed changes
Comment thread src/git.rs
pksunkara
pksunkara reviewed Oct 14, 2021
View reviewed changes

@pksunkara pksunkara left a comment

Copy link
Copy Markdown

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

Comment thread src/models/krate.rs
@bors

bors commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #4096) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss ehuss mentioned this pull request Dec 21, 2021
Closed
3 tasks
@djc

djc commented Dec 21, 2021

Copy link
Copy Markdown
Contributor

Gentle ping? I think this should be labeled as waiting on review again?

jhpratt
jhpratt reviewed Dec 22, 2021
View reviewed changes

@jhpratt jhpratt left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/models/krate.rs Outdated
@Turbo87

Turbo87 commented Dec 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

FWIW we discussed this on Discord a few days ago: https://discord.com/channels/442252698964721669/448525639469891595/920823792631152691

the tl;dr is that we show features on the UI these days and we need to verify that this still works correctly with this new syntax

@ehuss

ehuss commented Dec 23, 2021

Copy link
Copy Markdown
Contributor Author

the tl;dr is that we show features on the UI these days and we need to verify that this still works correctly with this new syntax

I'm not sure how you'd like to verify that. I could add something that did a database query in the features_version_2 test if you'd like.

The feature values themselves don't seem to be displayed in the UI:

image

I can confirm from the API that it is there:

curl http://localhost:4200/api/v1/crates/foo | jq .versions[0].features
{
  "f1": [
    "dep:bar"
  ],
  "f2": [
    "bar?/bar-feat"
  ]
}

And via psql:

cargo_registry=# select * from versions;
 id | crate_id |  num  |         updated_at         |         created_at         | downloads |                   features                   | yanked | license | crate_size | published_by
----+----------+-------+----------------------------+----------------------------+-----------+----------------------------------------------+--------+---------+------------+--------------
  1 |        1 | 0.1.0 | 2021-12-22 23:49:25.411573 | 2021-12-22 23:49:25.411573 |         0 | {}                                           | f      | MIT     |        702 |            1
  2 |        2 | 0.1.0 | 2021-12-22 23:51:35.513816 | 2021-12-22 23:51:35.513816 |         0 | {"bar-feat": []}                             | f      | MIT     |        719 |            1
  4 |        1 | 0.2.0 | 2021-12-22 23:56:47.374767 | 2021-12-22 23:56:47.374767 |         0 | {"f1": ["dep:bar"], "f2": ["bar?/bar-feat"]} | f      | MIT     |        772 |            1
(3 rows)

Turbo87
Turbo87 approved these changes Dec 23, 2021
View reviewed changes

@Turbo87 Turbo87 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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 :)

@Turbo87

Turbo87 commented Dec 30, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Dec 30, 2021

Copy link
Copy Markdown
Contributor

📌 Commit 2b4430c has been approved by Turbo87

@bors

bors commented Dec 30, 2021

Copy link
Copy Markdown
Contributor

⌛ Testing commit 2b4430c with merge 91a2cd4...

@bors

bors commented Dec 30, 2021

Copy link
Copy Markdown
Contributor

☀️ Test successful - checks-actions
Approved by: Turbo87
Pushing 91a2cd4 to master...

@bors bors merged commit 91a2cd4 into rust-lang:master Dec 30, 2021
@Turbo87

Turbo87 commented Dec 30, 2021

Copy link
Copy Markdown
Member

rebased, merged and deployed. thanks for working on this @ehuss!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtgeibel jtgeibel jtgeibel left review comments
@Turbo87 Turbo87 Turbo87 approved these changes
+2 more reviewers
@pksunkara pksunkara pksunkara left review comments
@jhpratt jhpratt jhpratt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ehuss @rust-highfive @bors @djc @Turbo87 @jtgeibel @pksunkara @jhpratt @smarnach