refactor(toml): Decouple logic from schema #13080

epage · 2023-11-30T16:51:15Z

What does this PR try to resolve?

This tries to decouple cargo logic from the schemas so we can split the schemas out into a separate crate for #12801

Profile layering was one of the few pieces of logic I kept in the schema, assuming its general enough / decoupled enough from cargo policies.

How should we test and review this PR?

Each step is broken down into its own commit for easier analysis

Additional information

rustbot · 2023-11-30T16:51:22Z

r? @weihanglo

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

weihanglo

Mostly LGTM. Thank you!

src/cargo/util/toml/mod.rs

weihanglo · 2023-12-01T16:29:16Z

src/cargo/util/toml/schema.rs

@@ -670,6 +670,98 @@ pub struct TomlProfile {
    pub trim_paths: Option<TomlTrimPaths>,
 }

+impl TomlProfile {
+    /// Overwrite self's values with the given profile.
+    pub fn merge(&mut self, profile: &Self) {


Profile layering was one of the few pieces of logic I kept in the schema, assuming its general enough / decoupled enough from cargo policies.

How is thing general enough? Do we have any possible use case outside cargo?

From what I understand of profiles, the merging is inherent to their definition, so if someone wants to do process the manifest, they'll need it.

I know its a bit dubious about why this when other things don't get included. The biggest motivation is that this is tightly coupled to the struct definition and keeping them close is important to avoid forgetting to merge new fields. The reason I felt this as justifiable was that it is very direct without special logic.

so if someone wants to do process the manifest, they'll need it.

I can hardly find a case for purely merging profile without any profile inheritance logic involved.

keeping them close is important to avoid forgetting to merge new fields

However, I understand and agree to this reason. We can always iterate later on. Thank you!

weihanglo · 2023-12-01T16:42:48Z

One thing worth calling out is that free functions sometimes are hard to find. By grouping methods under an item I can get a better auto-completion experience whey typing things.<tab>.

epage · 2023-12-01T16:54:30Z

One thing worth calling out is that free functions sometimes are hard to find. By grouping methods under an item I can get a better auto-completion experience whey typing things.<tab>.

This is a given if we still want to do #12801. A lot of this logic is too tightly coupled to the internals of cargo to be able to split out without a lot of other work first (if ever)

weihanglo · 2023-12-01T17:21:17Z

@bors r+

Thank you again for the hard work!

bors · 2023-12-01T17:21:20Z

📌 Commit 9bb7c97 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-12-01T18:48:39Z

⌛ Testing commit 9bb7c97 with merge b97cffe...

bors · 2023-12-01T19:22:47Z

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

Update cargo 25 commits in 26333c732095d207aa05932ce863d850fb309386..58fb23140972092a12f7011d17a7db1d99e3eacf 2023-11-28 20:07:39 +0000 to 2023-12-02 14:15:16 +0000 - test(install): use TCP connection instead of thread sleep (rust-lang/cargo#13099) - test(mdman): Switch to snapbox (rust-lang/cargo#13098) - Include declared list of features in fingerprint for `-Zcheck-cfg` (rust-lang/cargo#13012) - chore(deps): update compatible (rust-lang/cargo#13083) - chore(ci): Always update gix packages together (rust-lang/cargo#13093) - chore(deps): update rust crate windows-sys to 0.52 (rust-lang/cargo#13089) - refactor(toml): Decouple logic from schema (rust-lang/cargo#13080) - Have cargo add --optional <dep> create a <dep> = "dep:<dep> feature (rust-lang/cargo#13071) - Add `--public` for `cargo add` (rust-lang/cargo#13046) - chore(deps): update rust crate toml_edit to 0.21.0 (rust-lang/cargo#13088) - chore(deps): update rust crate rusqlite to 0.30.0 (rust-lang/cargo#13087) - test(trim-paths): exercise with real world debugger (rust-lang/cargo#13091) - Fixed uninstall a running binary failed on Windows (rust-lang/cargo#13053) - chore(deps): update rust crate itertools to 0.12.0 (rust-lang/cargo#13086) - Add more options to registry test support. (rust-lang/cargo#13085) - Don't filter on workspace members when scraping doc examples (rust-lang/cargo#13077) - Remove the outdated comment (rust-lang/cargo#13076) - fix(resolver): Remove unused public-deps error handling (rust-lang/cargo#13036) - Fixes error count display is different when there's only one error left (rust-lang/cargo#12484) - fix: reorder `--remap-path-prefix` flags for `-Zbuild-std` (rust-lang/cargo#13065) - remove jobserver env var in some tests (rust-lang/cargo#13072) - doc: clarify different target has different set of `CARGO_CFG_*` values (rust-lang/cargo#13069) - docs: remove review capacity notice in PR template (rust-lang/cargo#13070) - chore(deps): update rust crate openssl to 0.10.60 [security] (rust-lang/cargo#13068) - fix(resolver): De-prioritize no-rust-version in MSRV resolver (rust-lang/cargo#13066) r? ghost

Update cargo 27 commits in 26333c732095d207aa05932ce863d850fb309386..623b788496b3e51dc2f9282373cf0f6971a229b5 2023-11-28 20:07:39 +0000 to 2023-12-02 18:10:03 +0000 - docs(book): make old title anchorable (rust-lang/cargo#13102) - Revert "chore(deps): update rust crate openssl to 0.10.60 [security]" (rust-lang/cargo#13101) - test(install): use TCP connection instead of thread sleep (rust-lang/cargo#13099) - test(mdman): Switch to snapbox (rust-lang/cargo#13098) - Include declared list of features in fingerprint for `-Zcheck-cfg` (rust-lang/cargo#13012) - chore(deps): update compatible (rust-lang/cargo#13083) - chore(ci): Always update gix packages together (rust-lang/cargo#13093) - chore(deps): update rust crate windows-sys to 0.52 (rust-lang/cargo#13089) - refactor(toml): Decouple logic from schema (rust-lang/cargo#13080) - Have cargo add --optional <dep> create a <dep> = "dep:<dep> feature (rust-lang/cargo#13071) - Add `--public` for `cargo add` (rust-lang/cargo#13046) - chore(deps): update rust crate toml_edit to 0.21.0 (rust-lang/cargo#13088) - chore(deps): update rust crate rusqlite to 0.30.0 (rust-lang/cargo#13087) - test(trim-paths): exercise with real world debugger (rust-lang/cargo#13091) - Fixed uninstall a running binary failed on Windows (rust-lang/cargo#13053) - chore(deps): update rust crate itertools to 0.12.0 (rust-lang/cargo#13086) - Add more options to registry test support. (rust-lang/cargo#13085) - Don't filter on workspace members when scraping doc examples (rust-lang/cargo#13077) - Remove the outdated comment (rust-lang/cargo#13076) - fix(resolver): Remove unused public-deps error handling (rust-lang/cargo#13036) - Fixes error count display is different when there's only one error left (rust-lang/cargo#12484) - fix: reorder `--remap-path-prefix` flags for `-Zbuild-std` (rust-lang/cargo#13065) - remove jobserver env var in some tests (rust-lang/cargo#13072) - doc: clarify different target has different set of `CARGO_CFG_*` values (rust-lang/cargo#13069) - docs: remove review capacity notice in PR template (rust-lang/cargo#13070) - chore(deps): update rust crate openssl to 0.10.60 [security] (rust-lang/cargo#13068) - fix(resolver): De-prioritize no-rust-version in MSRV resolver (rust-lang/cargo#13066)

refactor: Clarify PackageId constructor names ### What does this PR try to resolve? From #13080 > I would love to see eithe rename this function or have a doc comment on it. It always puzzle me what pure means. From my experience, `new` is more normally a name for more direct construction when there are alternatives ### How should we test and review this PR? ### 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.

epage added 15 commits November 21, 2023 20:43

refactor(toml): Inline TomlPackage::to_package_id

d8b9b99

refactor(toml): Inline TomlLintLevel::flag

a774d8b

refactor(toml): Inline TomlDetailedDependency::update_optional

b4489e3

refactor(toml): Inline TomlDetailedDependency::add_features

97c9a17

refactor(toml): Inline TomlDetailedDependency::resolve_path

89bfc4d

refactor(toml): Make profile validation free methods

6e187f8

refactor(toml): Move Profile layering to be part of schema API

1ed0efc

refactor(toml): Group inheritance code

3cb3130

refactor(toml): Make InheritableField::inherit_with a free function

3047a41

refactor(toml): Make InheritableLints::inherit_with a free function

e48befe

refactor(toml): Make InheritableDependency::inhert_with a free function

d32e1f9

refactor(toml): Make Detailed Dep's to_dependency a free function

94395d4

refactor(toml): Make Dep's to_dependency a free function

8ff20c7

refactor(toml): Make Dep's to_dependency_split a free function

89f8f5c

refactor(toml): Make rest of TomlManifest logic free functions

9bb7c97

rustbot assigned weihanglo Nov 30, 2023

rustbot added A-manifest Area: Cargo.toml issues A-profiles Area: profiles A-workspaces Area: workspaces Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2023

epage force-pushed the toml branch from ac5b461 to eb9272f Compare November 30, 2023 16:58

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

epage force-pushed the toml branch from eb9272f to 9bb7c97 Compare December 1, 2023 16:30

weihanglo approved these changes Dec 1, 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 1, 2023

bors merged commit b97cffe into rust-lang:master Dec 1, 2023
20 checks passed

This was referenced Dec 1, 2023

Ignore errors when declaring a registry on path dependency #12807

Draft

Implement support for base paths (RFC 3529) #12974

Closed

epage deleted the toml branch December 1, 2023 19:27

epage mentioned this pull request Dec 1, 2023

Official API for parsing Cargo.tomls schema #12801

Closed

20 tasks

weihanglo mentioned this pull request Dec 2, 2023

Update cargo rust-lang/rust#118541

Merged

ehuss added this to the 1.76.0 milestone Dec 6, 2023

epage mentioned this pull request Dec 6, 2023

refactor: Clarify PackageId constructor names #13123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(toml): Decouple logic from schema #13080

refactor(toml): Decouple logic from schema #13080

epage commented Nov 30, 2023

rustbot commented Nov 30, 2023

weihanglo left a comment

weihanglo Dec 1, 2023

epage Dec 1, 2023

weihanglo Dec 1, 2023

weihanglo commented Dec 1, 2023

epage commented Dec 1, 2023

weihanglo commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

refactor(toml): Decouple logic from schema #13080

refactor(toml): Decouple logic from schema #13080

Conversation

epage commented Nov 30, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Nov 30, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Dec 1, 2023

Choose a reason for hiding this comment

epage Dec 1, 2023

Choose a reason for hiding this comment

weihanglo Dec 1, 2023

Choose a reason for hiding this comment

weihanglo commented Dec 1, 2023

epage commented Dec 1, 2023

weihanglo commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023