refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` #11565

Muscraft · 2023-01-11T22:37:55Z

This should not be merged until after #11409

In #11523 it was noted that you could use {}.workspace = true in [patch.{}], but it would cause a panic. This panic was caused by an oversight on my part when implementing workspace inheritance. Before this PR any field that had the type TomlDependency could specify {}.workspace = true and cargo would allow it to be a TomlWorkspaceDependency. While it could be TomlWorkspaceDependency it would never be resolved since only:

Dependencies in the [dependencies], [dev-dependencies], [build-dependencies], and [target."...".dependencies] sections will support the ability to reference the [workspace.dependencies] definition of dependencies.¹

This PR makes it so that only those fields can pull from [workspace.dependencies], while still sharing TomlDependency everywhere it is needed. It does this by making MaybeWorkspace generic over both Defined and Workspace, then moving TomlWorkspaceDependency out of TomlDependency and into a MaybeWorkspace that the correct fields can use.

Closes: #11523

rfc2906-new-dependency-directives ↩

rustbot · 2023-01-11T22:37:59Z

r? @weihanglo

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

bors · 2023-01-20T19:38:19Z

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

weihanglo

Looks good! Could you do a rebase, and also add more comments on new items?

src/cargo/util/toml/mod.rs

weihanglo · 2023-01-23T16:03:37Z

src/cargo/util/toml/mod.rs

+pub trait WorkspaceInherit {
+    fn inherit_toml_table(&self) -> &str;
+    fn workspace(&self) -> bool;


Could we have docs for this trait and methods?

weihanglo · 2023-01-23T17:47:01Z

tests/testsuite/inheritable_workspace_fields.rs


            [dependencies]
            dep.workspace = true

            [workspace]
-            members = ["bar"]
+            members = []


Why does it require a member "bar" before but not after?

It used to fail before we would hit the below error below. It looks like it was an oversight on my part. It's removed so we can see the unused manifest key warning.

error: failed to parse manifest at `{}/cargo/target/tmp/cit/t4/foo/Cargo.toml` Caused by: cannot configure both `package.workspace` and `[workspace]`, only one can be specified

weihanglo · 2023-01-23T18:02:51Z

tests/testsuite/inheritable_workspace_fields.rs

+        .with_status(101)
+        .with_stderr(
+            "\
+[WARNING] [CWD]/Cargo.toml: dependency (bar) specified without providing a local path, Git repository, or version to use. This will be considered an error in future versions


These messages are a bit unclear, unfortunately. As a user, I usually fix a compilation failure starting from errors. However, the error is useless than unused manifest key warning. I don't know how to improve. Just point it out.

I don't like this either, and I am hoping my next set of changes can help improve this. I am unsure how else to make it better

src/cargo/util/toml/mod.rs

weihanglo · 2023-01-23T20:28:48Z

src/cargo/util/toml/mod.rs

+
+#[derive(Deserialize, Serialize, Clone, Debug)]
+#[serde(rename_all = "kebab-case")]
+pub struct TomlWorkspaceDependency {


Side note: we may support artifact dependencies for workspace.

What do you mean by this?

Other fields for unstable feature artifact dependencies

[workspace.dependencies] bar = { artifact = "cdylib", version = "1.0", lib = true, target = "wasm32-unknown-unknown" }

I can add those fields in if needed, but they weren't in there before I made these changes

Just filed an issue for that: #11625

weihanglo

Looks good. Thank you for the effort!

weihanglo · 2023-01-25T23:51:19Z

@bors r+

bors · 2023-01-25T23:51:21Z

📌 Commit 712b327 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-01-25T23:51:27Z

⌛ Testing commit 712b327 with merge 2ccd950...

bors · 2023-01-26T00:37:53Z

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

18 commits in 3c5af6bed9a1a243a693e8e22ee2486bd5b82a6c..e84a7928d93a31f284b497c214a2ece69b4d7719 2023-01-24 15:48:15 +0000 to 2023-01-31 22:18:09 +0000 - chore: Add autolabel for `Command-*` labels (rust-lang/cargo#11664) - Update cross test instructions for aarch64-apple-darwin (rust-lang/cargo#11663) - Make cargo install report needed features (rust-lang/cargo#11647) - docs(contrib): Remove out-of-date process step (rust-lang/cargo#11662) - Do not error for `auth-required: true` without `-Z sparse-registry` (rust-lang/cargo#11661) - Warn on commits to non-default branches. (rust-lang/cargo#11655) - Avoid saving the same future_incompat warning multiple times (rust-lang/cargo#11648) - Mention current default value in `publish.timeout` docs (rust-lang/cargo#11652) - Make cargo aware of dwp files. (rust-lang/cargo#11572) - Reduce target info rustc query calls (rust-lang/cargo#11633) - Bump to 0.70.0; update changelog (rust-lang/cargo#11640) - Enable sparse protocol in CI (rust-lang/cargo#11632) - Fix split-debuginfo support detection (rust-lang/cargo#11347) - refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` (rust-lang/cargo#11565) - book: describe how the current resolver sometimes duplicates deps (rust-lang/cargo#11604) - `cargo add` check `[dependencies]` order without considering the dotted item (rust-lang/cargo#11612) - Link CoC to www.rust-lang.org/conduct.html (rust-lang/cargo#11622) - Add more labels to triagebot (rust-lang/cargo#11621)

Update cargo 18 commits in 3c5af6bed9a1a243a693e8e22ee2486bd5b82a6c..e84a7928d93a31f284b497c214a2ece69b4d7719 2023-01-24 15:48:15 +0000 to 2023-01-31 22:18:09 +0000 - chore: Add autolabel for `Command-*` labels (rust-lang/cargo#11664) - Update cross test instructions for aarch64-apple-darwin (rust-lang/cargo#11663) - Make cargo install report needed features (rust-lang/cargo#11647) - docs(contrib): Remove out-of-date process step (rust-lang/cargo#11662) - Do not error for `auth-required: true` without `-Z sparse-registry` (rust-lang/cargo#11661) - Warn on commits to non-default branches. (rust-lang/cargo#11655) - Avoid saving the same future_incompat warning multiple times (rust-lang/cargo#11648) - Mention current default value in `publish.timeout` docs (rust-lang/cargo#11652) - Make cargo aware of dwp files. (rust-lang/cargo#11572) - Reduce target info rustc query calls (rust-lang/cargo#11633) - Bump to 0.70.0; update changelog (rust-lang/cargo#11640) - Enable sparse protocol in CI (rust-lang/cargo#11632) - Fix split-debuginfo support detection (rust-lang/cargo#11347) - refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` (rust-lang/cargo#11565) - book: describe how the current resolver sometimes duplicates deps (rust-lang/cargo#11604) - `cargo add` check `[dependencies]` order without considering the dotted item (rust-lang/cargo#11612) - Link CoC to www.rust-lang.org/conduct.html (rust-lang/cargo#11622) - Add more labels to triagebot (rust-lang/cargo#11621) r? `@ghost`

rustbot assigned weihanglo Jan 11, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2023

weihanglo reviewed Jan 23, 2023

View reviewed changes

Muscraft force-pushed the rework-toml-dependency branch from ff524ff to b753fcf Compare January 25, 2023 15:39

refactor(toml): Move TomlWorkspaceDependency out of TomlDependency

712b327

Muscraft force-pushed the rework-toml-dependency branch from b753fcf to 712b327 Compare January 25, 2023 16:51

weihanglo mentioned this pull request Jan 25, 2023

Make workspace inheritance support artifact dependencies #11625

Open

weihanglo approved these changes Jan 25, 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 Jan 25, 2023

bors merged commit 2ccd950 into rust-lang:master Jan 26, 2023

Muscraft deleted the rework-toml-dependency branch January 26, 2023 15:42

ehuss mentioned this pull request Jan 28, 2023

fix(toml): Provide a way to show unused manifest keys for dependencies #11630

Merged

weihanglo mentioned this pull request Feb 1, 2023

Update cargo rust-lang/rust#107541

Merged

ehuss added this to the 1.69.0 milestone Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` #11565

refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` #11565

Muscraft commented Jan 11, 2023

rustbot commented Jan 11, 2023

bors commented Jan 20, 2023

weihanglo left a comment

weihanglo Jan 23, 2023

weihanglo Jan 23, 2023

Muscraft Jan 25, 2023

weihanglo Jan 23, 2023

Muscraft Jan 25, 2023

weihanglo Jan 23, 2023

Muscraft Jan 25, 2023

weihanglo Jan 25, 2023

Muscraft Jan 25, 2023

weihanglo Jan 25, 2023

weihanglo left a comment

weihanglo commented Jan 25, 2023

bors commented Jan 25, 2023

bors commented Jan 25, 2023

bors commented Jan 26, 2023

refactor(toml): Move TomlWorkspaceDependency out of TomlDependency #11565

refactor(toml): Move TomlWorkspaceDependency out of TomlDependency #11565

Conversation

Muscraft commented Jan 11, 2023

Footnotes

rustbot commented Jan 11, 2023

bors commented Jan 20, 2023

weihanglo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Jan 25, 2023

bors commented Jan 25, 2023

bors commented Jan 25, 2023

bors commented Jan 26, 2023

refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` #11565

refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` #11565