refactor(toml): Pull out the schema #12911

epage · 2023-11-02T18:49:45Z

What does this PR try to resolve?

On its own, this PR is a net negative for readability / complexity. It moves all of the serde types related to manifest from toml/mod.rs to toml/schema.rs, leaving a lot of the functions and some trait implementations back in toml/mod.rs (some basic functions that made sense for the type on their own were also moved).

So why do this? This is an ooch towards having the schema broken out into a separate package (#12801). To do this, we need to

Identify what all types need to be put in the package. This refactor highlights the dependence on RustVersion and PackageIdSpec
Highlights what functionality we need to find a new home for

Follow up PRs would

Find better homes for the logic in toml/mod.rs, moving us away from having impl schema::Type blocks.
Pull out a src/cargo/util_semver package to own PartialVersion (at least) as prep for making a cargo-util-semver package (refactor(util): Pull out mod util_semver #12940)
Move RustVersion to manifest-toml/schema.rs, deciding what functionality needs to move with the type
Move or copy PackageIdSpec into manfest-toml/schema.rs, deciding what functionality remain in core/ and what moves over
Move toml/schema.rs to src/cargo/util_schema
Actually make cargo-util-semver and cargo-util-manifest-schema packages

So why do this now? This is a big change! By being incremental

Reduce churn for me and others
Be easier to review
Collect feedback as we go on the whole plan to avoid more painful changes later

We can back this out if needed but the further we go, the more painful it will be.

How should we test and review this PR?

Additional information

Remaining steps - Decouple `toml/mod.rs` functionality from `toml/schema.rs` - Pull out `core/` types referenced by `toml/schema.rs`

rustbot · 2023-11-02T18:49:51Z

r? @ehuss

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

ehuss · 2023-11-03T03:07:47Z

Just to confirm, this just moves things, there aren't any other changes of substance?

If so, it sounds good to me. However, I'd like to defer to @weihanglo to see what they would like.

r? @weihanglo

epage · 2023-11-03T11:15:15Z

Correct. The most visible changes are for cargo-as-a-library users

Types moved
More types are pub
Fields are pub

weihanglo

The module name doesn't catch my eye. toml::schema looks pretty genernal that we might also want config.toml to be inside. I don't think that's the better direction, but is it in the plan in your mind?

epage · 2023-11-06T14:45:20Z

Thats been a problem for util::toml for a while :)

This doesn't try to solve it. I laid out my plan within the PR description. Currently, I have the package name as cargo-util-schema but tht does remind me that I meant it to be cargo-util-manifest-schema

EDIT: PR description is updated

weihanglo

While I left two comments about the visibility of seemingly internal only fields, I am going to merge this to unblock stuff.

weihanglo · 2023-11-06T16:28:35Z

src/cargo/util/toml/schema.rs

+
+    /// Provide a helpful error message for a common user error.
+    #[serde(rename = "cargo-features", skip_serializing)]
+    pub _invalid_cargo_features: Option<InvalidCargoFeatures>,


What is the use case of making this public?

That depends on if we want this to be #[non_exhaustive] or directly constructable. For now, I was going with directly constructable.

weihanglo · 2023-11-06T16:29:54Z

src/cargo/util/toml/schema.rs

+    /// This is here to provide a way to see the "unused manifest keys" when deserializing
+    #[serde(skip_serializing)]
+    #[serde(flatten)]
+    pub unused_keys: BTreeMap<String, toml::Value>,


This seems like for emitting a better warning only. Do we want to pub this?

Callers might want to collect unused keys

See above about #[non_exhaustive]

weihanglo · 2023-11-06T16:59:20Z

Correct. The most visible changes are for cargo-as-a-library users
* Types moved

* More types are pub

* Fields are pub

Worth noting that, making fields and types pub doesn't mean they are stable. There is still no strong stability guarantee for Cargo as a library, but this PR is a path forward that goal.

@bors r+

bors · 2023-11-06T16:59:23Z

📌 Commit 9a1bbc9 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-11-06T17:00:31Z

⌛ Testing commit 9a1bbc9 with merge 1efd40e...

refactor(toml): Pull out the schema ### What does this PR try to resolve? On its own, this PR is a net negative for readability / complexity. It moves all of the serde types related to manifest from `toml/mod.rs` to `toml/schema.rs`, leaving a lot of the functions and some trait implementations back in `toml/mod.rs` (some basic functions that made sense for the type on their own were also moved). So why do this? This is an ooch towards having the schema broken out into a separate package (#12801). To do this, we need to - Identify what all types need to be put in the package. This refactor highlights the dependence on `RustVersion` and `PackageIdSpec` - Highlights what functionality we need to find a new home for Follow up PRs would - Find better homes for the logic in `toml/mod.rs`, moving us away from having `impl schema::Type` blocks. - Pull out a `src/cargo/util_semver` package to own `PartialVersion` (at least) as prep for making a `cargo-util-semver` package - Move `RustVersion` to `manifest-toml/schema.rs`, deciding what functionality needs to move with the type - Move or copy `PackageIdSpec` into `manfest-toml/schema.rs`, deciding what functionality remain in `core/` and what moves over - Move `toml/schema.rs` to `src/cargo/util_schema` - Actually make `cargo-util-semver` and `cargo-util-manifest-schema` packages So why do this now? This is a big change! By being incremental - Reduce churn for me and others - Be easier to review - Collect feedback as we go on the whole plan to avoid more painful changes later We *can* back this out if needed but the further we go, the more painful it will be. ### How should we test and review this PR? ### Additional information

bors · 2023-11-06T17:17:21Z

💔 Test failed - checks-actions

epage · 2023-11-06T17:19:41Z

Blocked on #12921

weihanglo · 2023-11-06T19:48:08Z

@bors retry

bors · 2023-11-06T19:49:17Z

⌛ Testing commit 9a1bbc9 with merge 4300dd7...

bors · 2023-11-06T20:25:28Z

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

Update cargo 7 commits in 65e297d1ec0dee1a74800efe600b8dc163bcf5db..7046d992f9f32ba209a8079f662ebccf9da8de25 2023-11-03 20:56:31 +0000 to 2023-11-08 03:24:57 +0000 - fix: Report more detailed semver errors (rust-lang/cargo#12924) - Fix some broken links in the man pages (rust-lang/cargo#12929) - Add better error message when it can not find the search section (rust-lang/cargo#12865) - Bug 12920 (rust-lang/cargo#12923) - Update link in environment-variables.md (rust-lang/cargo#12922) - refactor(toml): Pull out the schema (rust-lang/cargo#12911) - tests: Remove plugin tests (rust-lang/cargo#12921) r? ghost

refactor(toml): Simplify code to make schema split easier ### What does this PR try to resolve? This is a follow up to #12911 and is prep for #12801. ### How should we test and review this PR? ### Additional information

epage added 2 commits November 1, 2023 20:54

feat(toml): Publically expose all schema types

a15c377

refactor(toml): Pull out schema

9a1bbc9

Remaining steps - Decouple `toml/mod.rs` functionality from `toml/schema.rs` - Pull out `core/` types referenced by `toml/schema.rs`

rustbot assigned ehuss Nov 2, 2023

epage force-pushed the manifest-schema branch from 11bcc2b to 547f7d9 Compare November 2, 2023 18:57

rustbot added the A-unstable Area: nightly unstable support label Nov 2, 2023

rustbot assigned weihanglo and unassigned ehuss Nov 3, 2023

epage force-pushed the manifest-schema branch from 547f7d9 to 9a1bbc9 Compare November 3, 2023 22:00

weihanglo reviewed Nov 6, 2023

View reviewed changes

weihanglo approved these changes Nov 6, 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 Nov 6, 2023

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

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 Nov 6, 2023

bors merged commit 4300dd7 into rust-lang:master Nov 6, 2023
20 checks passed

epage deleted the manifest-schema branch November 6, 2023 21:06

weihanglo mentioned this pull request Nov 8, 2023

Update cargo rust-lang/rust#117699

Merged

This was referenced Nov 8, 2023

Official API for parsing Cargo.tomls schema #12801

Closed

refactor(toml): Simplify code to make schema split easier #12948

Merged

ehuss added this to the 1.75.0 milestone Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(toml): Pull out the schema #12911

refactor(toml): Pull out the schema #12911

epage commented Nov 2, 2023 •

edited

Loading

rustbot commented Nov 2, 2023

ehuss commented Nov 3, 2023

epage commented Nov 3, 2023

weihanglo left a comment

epage commented Nov 6, 2023 •

edited

Loading

weihanglo left a comment

weihanglo Nov 6, 2023

epage Nov 6, 2023

weihanglo Nov 6, 2023

epage Nov 6, 2023

weihanglo commented Nov 6, 2023

bors commented Nov 6, 2023

bors commented Nov 6, 2023

bors commented Nov 6, 2023

epage commented Nov 6, 2023

weihanglo commented Nov 6, 2023

bors commented Nov 6, 2023

bors commented Nov 6, 2023

refactor(toml): Pull out the schema #12911

refactor(toml): Pull out the schema #12911

Conversation

epage commented Nov 2, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Nov 2, 2023

ehuss commented Nov 3, 2023

epage commented Nov 3, 2023

weihanglo left a comment

Choose a reason for hiding this comment

epage commented Nov 6, 2023 • edited Loading

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Nov 6, 2023

Choose a reason for hiding this comment

epage Nov 6, 2023

Choose a reason for hiding this comment

weihanglo Nov 6, 2023

Choose a reason for hiding this comment

epage Nov 6, 2023

Choose a reason for hiding this comment

weihanglo commented Nov 6, 2023

bors commented Nov 6, 2023

bors commented Nov 6, 2023

bors commented Nov 6, 2023

epage commented Nov 6, 2023

weihanglo commented Nov 6, 2023

bors commented Nov 6, 2023

bors commented Nov 6, 2023

epage commented Nov 2, 2023 •

edited

Loading

epage commented Nov 6, 2023 •

edited

Loading