refactor: Pull PackageIdSpec into schema#13128

epage · 2023-12-06T22:27:48Z

What does this PR try to resolve?

This removes one of the remaining ties util_schemas has on the rest of cargo as part of #12801.

How should we test and review this PR?

This is broken up into small commits

Additional information

This is a step towards moving `PackageIdSpec` into `schema` as manifests need it as part of rust-lang#12801. While I didn't take this approach in rust-lang#13080, that was largely how core these functions are / how pervasive their use is.

rustbot · 2023-12-06T22:27:53Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

weihanglo

Looks good to me. Just some questions that we should deal with later.

weihanglo · 2023-12-08T04:04:50Z

src/cargo/util_schemas/mod.rs

 //! Any logic for getting final semantics from these will likely need other tools to process, like
 //! `cargo metadata`.

+pub mod core;


Although it is just moving things around, core is not too meaningful as a name.

Got a better idea?

I considered common and misc but figured those were the same or worse

I considered the root but then that gets messy

I considered manifest but these are general beyond that

Or remove the entire core and expose every type to root level?

Thats the "messy" part. I feel like it'd more help users reading the docs to not have a large dumping ground in the root of types that are secondary to what they are doing.

weihanglo · 2023-12-08T04:43:56Z

src/cargo/core/package_id_spec.rs

+        I: IntoIterator<Item = PackageId>;
+}
+
+impl PackageIdSpecQuery for PackageIdSpec {


I like the name, though naming is hard, and cargo doesn't have a naming convention for this "extension" like trait. Do you have a better idea?

Most extension traits have are <type>Ext which is weird if there might be multiple (e.g. ResultExt) so I tend to use more specific names.

weihanglo · 2023-12-08T04:52:35Z

src/cargo/core/package_id_spec.rs

+        I: IntoIterator<Item = PackageId>;
+}
+
+impl PackageIdSpecQuery for PackageIdSpec {


Should we have a simple doc for this? For example

std::os::unix::ffi::OsStrExt

anyhow::Context

itertools::Itertools (not exactly an extension trait)

weihanglo · 2023-12-08T04:52:51Z

src/cargo/core/package_id_spec.rs

+        I: IntoIterator<Item = PackageId>;
+}
+
+impl PackageIdSpecQuery for PackageIdSpec {


Do we want to "seal" this trait?

We can but if we start caring about that for the cargo crate, we likely have a bigger audit to do for API stability.

weihanglo

I am not seeing my comments as blockers for this PR, so feel free to r=me if you want to proceed.

epage · 2023-12-08T20:49:02Z

@bors r=weihanglo

bors · 2023-12-08T20:49:05Z

📌 Commit d1b1cb1 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-12-08T20:55:32Z

⌛ Testing commit d1b1cb1 with merge a1449ee...

bors · 2023-12-08T21:31:21Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing a1449ee to master...

Update cargo 12 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..6feabf94773286928b7d73d0d313c0c5562b66af 2023-12-06 02:29:23 +0000 to 2023-12-08 22:38:37 +0000 - fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114) - Don't rely on mtime to test changes (rust-lang/cargo#13143) - refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128) - fix: Print rustc messages colored on wincon (rust-lang/cargo#13140) - Add a windows manifest file (rust-lang/cargo#13131) - Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132) - re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130) - fix bash completion in directory with spaces (rust-lang/cargo#13126) - test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129) - fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125) - re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117) - refactor: Clarify PackageId constructor names (rust-lang/cargo#13123) r? ghost

Update cargo 13 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..66ad359b408ccdf867cceb30cc2dff1ed4afd82d 2023-12-06 02:29:23 +0000 to 2023-12-09 12:30:01 +0000 - chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144) - fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114) - Don't rely on mtime to test changes (rust-lang/cargo#13143) - refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128) - fix: Print rustc messages colored on wincon (rust-lang/cargo#13140) - Add a windows manifest file (rust-lang/cargo#13131) - Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132) - re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130) - fix bash completion in directory with spaces (rust-lang/cargo#13126) - test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129) - fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125) - re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117) - refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)

Update cargo 20 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..1aa9df1a5be205cce621f0bc0ea6062a5e22a98c 2023-12-06 02:29:23 +0000 to 2023-12-12 14:52:31 +0000 - crates-io: Add support for other 2xx HTTP status codes (rust-lang/cargo#13158) - Remove the deleted feature test_2018_feature from the test (rust-lang/cargo#13156) - refactor(schema): Remove reliance on cargo types (rust-lang/cargo#13154) - fix(toml)!: Disallow `[lints]` in virtual workspaces (rust-lang/cargo#13155) - Limit exported-private-dependencies lints to libraries (rust-lang/cargo#13135) - chore: update to gix-index@0.27.1 (rust-lang/cargo#13148) - Update curl-sys to bring in curl 8.5.0 (rust-lang/cargo#13147) - chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144) - fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114) - Don't rely on mtime to test changes (rust-lang/cargo#13143) - refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128) - fix: Print rustc messages colored on wincon (rust-lang/cargo#13140) - Add a windows manifest file (rust-lang/cargo#13131) - Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132) - re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130) - fix bash completion in directory with spaces (rust-lang/cargo#13126) - test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129) - fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125) - re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117) - refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)

This is a partial revert of 2b2502f These were pulled out with the idea of being a place to house `PartialVersion` for `util_schemas` to use. Since then, we went with a `util_schemas::core` in rust-lang#13128, meaning we can hold off on creating yet another crate for us to manage.

epage added 5 commits December 6, 2023 14:56

refactor(schema): Pull SourceKind out into schemas

f3999d5

refactor: Pull out constructor for PackageIdSpec

294c385

refactor: Move Id to Spec converter

33d1d78

refactor: Pull PackageIdSpec into schema

d1b1cb1

rustbot assigned weihanglo Dec 6, 2023

epage force-pushed the schema branch from 89132e8 to 3380998 Compare December 6, 2023 22:33

weihanglo reviewed Dec 8, 2023

View reviewed changes

epage force-pushed the schema branch from 3380998 to d1b1cb1 Compare December 8, 2023 17:48

weihanglo approved these changes Dec 8, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2023

bors merged commit a1449ee into rust-lang:master Dec 8, 2023

bors mentioned this pull request Dec 8, 2023

fix: only examine base URL when matching package #13110

Closed

weihanglo mentioned this pull request Dec 9, 2023

Update cargo rust-lang/rust#118765

Merged

epage mentioned this pull request Dec 13, 2023

Official API for parsing Cargo.tomls schema #12801

Closed

20 tasks

ehuss added this to the 1.76.0 milestone Mar 2, 2024

Conversation

epage commented Dec 6, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Dec 6, 2023

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

epage commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments