Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Part 6 of RFC2906 - Switch the inheritance source from workspace to… #10564

Merged
merged 1 commit into from
Apr 13, 2022
Merged

Part 6 of RFC2906 - Switch the inheritance source from workspace to… #10564

merged 1 commit into from
Apr 13, 2022

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented Apr 13, 2022 •
edited
Loading

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563

This PR focuses on switching the inheritance source from workspace to workspace.package. The RFC specified workspace but it was decided that it should be changed to workspace.package per epage's comment.

  • Moved fields that can be inherited to package: Option<InheritableFields> in TomlWorkspace
    • Making package Option<InheritableFields> was done to remove duplication of types and better signify what fields you can inherit from in one place
  • Added #[serde(skip)] to ws_root and dependencies in InheritableFields since they will never be present when deserializing and we don't want them present when serializing
  • Added a method to update ws_root and dependencies in InheritableFields
    • Added for ws_root since it will never be present in a Cargo.toml and is needed to resolve relative paths
    • Added for dependencies since they are under workspace.dependencies not workspace.package.dependencies
  • workspace.dependencies was left out of workspace.package to better match package and package.dependencies
  • Updated error messages from workspace._ to workspace.package._
  • Updated unstable.md to reflect change to workspace.package
  • Updated tests to use workspace.package

Remaining implementation work for the RFC

@rust-highfive
Copy link

r? @ehuss

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2022
@Muscraft
Copy link
Member Author

r? @epage

@rust-highfive rust-highfive assigned epage and unassigned ehuss Apr 13, 2022
@epage epage mentioned this pull request Apr 13, 2022
Closed
12 tasks
epage
epage reviewed Apr 13, 2022
View reviewed changes
src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Apr 13, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Apr 13, 2022

📌 Commit 494eb29 has been approved by epage

@bors 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 Apr 13, 2022
@bors
Copy link
Contributor

bors commented Apr 13, 2022

⌛ Testing commit 494eb29 with merge dba5baf...

@bors
Copy link
Contributor

bors commented Apr 13, 2022

☀️ Test successful - checks-actions
Approved by: epage
Pushing dba5baf to master...

@bors bors merged commit dba5baf into rust-lang:master Apr 13, 2022
@Muscraft Muscraft mentioned this pull request Apr 13, 2022
Merged
@ehuss ehuss mentioned this pull request Apr 14, 2022
Merged
bors added a commit that referenced this pull request Apr 14, 2022
@bors
Auto merge of #10565 - Muscraft:rfc2906-part7, r=epage
ec83f8d 
Part 7 of RFC2906 - Add support for inheriting `exclude` and `include`

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563
Part 6: #10564

This PR focuses on adding support for inheriting `include` and `exclude`. While they were not in the original RFC it was decided that they should be added per [epage's comment](#8415 (comment)).
- Changed `include` and `exclude` into `Option<MaybeWorkspace<Vec<String>>>` inside `TomlProject`
- Added `include` and `exclude` to `InheritbaleFields`
- Updated tests to verify `include` and `exclude` are inheriting correctly

Remaining implementation work for the RFC
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations, as needed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 14, 2022
@matthiaskrgr
Rollup merge of rust-lang#96031 - ehuss:update-cargo, r=ehuss
bfe6f87 
Update cargo

11 commits in e2e2dddebe66dfc1403a312653557e332445308b..dba5baf4345858c591517b24801902a062c399f8
2022-04-05 17:04:53 +0000 to 2022-04-13 21:58:27 +0000
- Part 6 of RFC2906 - Switch the inheritance source from `workspace` to… (rust-lang/cargo#10564)
- Part 5 of RFC2906 - Add support for inheriting `rust-version` (rust-lang/cargo#10563)
- Add support for rustc --check-cfg well known names and values (rust-lang/cargo#10486)
- Reserve filename `Cargo.toml.orig` in `cargo package` (rust-lang/cargo#10551)
- Retry command invocation with argfile (rust-lang/cargo#10546)
- Add a progress indicator for `cargo clean` (rust-lang/cargo#10236)
- Ensure host units don't depend on Docscrape units, fixes rust-lang/cargo#10545 (rust-lang/cargo#10549)
- Fix docs: Bindeps env vars are passed to build script at runtime (rust-lang/cargo#10550)
- Part 4 of RFC2906 - Add support for inheriting `readme` (rust-lang/cargo#10548)
- Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` (rust-lang/cargo#10538)
- Bump to 0.63.0, update changelog (rust-lang/cargo#10544)
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2022
@bors
Auto merge of rust-lang#96031 - ehuss:update-cargo, r=ehuss
e371eeb 
Update cargo

11 commits in e2e2dddebe66dfc1403a312653557e332445308b..dba5baf4345858c591517b24801902a062c399f8
2022-04-05 17:04:53 +0000 to 2022-04-13 21:58:27 +0000
- Part 6 of RFC2906 - Switch the inheritance source from `workspace` to… (rust-lang/cargo#10564)
- Part 5 of RFC2906 - Add support for inheriting `rust-version` (rust-lang/cargo#10563)
- Add support for rustc --check-cfg well known names and values (rust-lang/cargo#10486)
- Reserve filename `Cargo.toml.orig` in `cargo package` (rust-lang/cargo#10551)
- Retry command invocation with argfile (rust-lang/cargo#10546)
- Add a progress indicator for `cargo clean` (rust-lang/cargo#10236)
- Ensure host units don't depend on Docscrape units, fixes rust-lang/cargo#10545 (rust-lang/cargo#10549)
- Fix docs: Bindeps env vars are passed to build script at runtime (rust-lang/cargo#10550)
- Part 4 of RFC2906 - Add support for inheriting `readme` (rust-lang/cargo#10548)
- Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` (rust-lang/cargo#10538)
- Bump to 0.63.0, update changelog (rust-lang/cargo#10544)
@Muscraft Muscraft mentioned this pull request Apr 14, 2022
Merged
bors added a commit that referenced this pull request Apr 14, 2022
@bors
Auto merge of #10568 - Muscraft:rfc2906-part8, r=epage
b75281b 
Part 8 of RFC2906 - Keep `InheritableFields` in a `LazyCell` inside `…

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563
Part 6: #10564
Part 7: #10565

This PR focuses on optimizations surrounding `InheritabeFields` and searching for a `WorkspaceRootConfig`:
- Keep `InheritableFields` in a `LazyCell` inside `to_real_manifest` so we only search for a workspace root once per `to_real_manifest`
- Changed calls for getting a workspace root to use `inherit_cell.try_borrow_with()`

[Testing Framework](https://gist.github.com/Muscraft/14f6879af27500e34584296edb468d15)
Test Results:
nest=1, runs=7, members=75, step=25
- 25 Members:
  - Optimized: 0.145s
  - Not Optimized: 0.181s
  - Normal: 0.141s
  - Percent change from Normal: 2.837%

- 50 Members
  - Optimized: 0.152s
  - Not Optimized: 0.267s
  - Normal: 0.141s
  - Percent change from Normal: 7.801%

nest=2, runs=7, members=75, step=25
- 25 Members
  - Optimized: 0.147s,
  - Not Optimized: 0.437s
  - Normal: 0.14s
  - Percent change from Normal: 5.0%

- 50 Members
  - Optimized: 0.159s
  - Not Optimized: 1.023s
  - Normal: 0.141s
  - Percent change from Normal: 12.766%

Using a `LazyCell` for `InheritableFields` brought down runtimes to more acceptable levels. It is worth noting that this is a very harsh test case and not one that represents real-world scenarios. That being said there are still some optimizations that could be done if we need to. The biggest one is making sure we only parse a `Cargo.toml` once.

Remaining implementation work for the RFC
- `cargo-add` support, see [epage's comment](#8415 (comment))
- More optimizations, as needed
@Muscraft Muscraft mentioned this pull request Apr 20, 2022
Merged
bors added a commit that referenced this pull request Apr 20, 2022
@bors
Auto merge of #10584 - Muscraft:rfc2906-part9, r=epage
1f1823c 
Prefer `key.workspace = true` to `key = { workspace = true }`

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563
Part 6: #10564
Part 7: #10565
Part 8: #10568

This PR fell out of [this discussion](#10497 (comment)) regarding if `key.workspace = true` is better than `key = { workspace = true }`.

Changes:
- Made all fields into `field.workspace = true`
- Made dependencies into `dep.workspace = true` when a they only specify `{ workspace = true }`

Remaining implementation work for the RFC
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations as needed
@Muscraft Muscraft mentioned this pull request Apr 20, 2022
Closed
@Muscraft Muscraft mentioned this pull request Apr 21, 2022
Closed
@epage epage mentioned this pull request Apr 21, 2022
Open
@ehuss ehuss added this to the 1.62.0 milestone Apr 22, 2022
This was referenced Apr 25, 2022
Merged
Merged
Merged
Merged
Merged
@Muscraft Muscraft deleted the rfc2906-part6 branch May 27, 2022 01:16
@messense messense mentioned this pull request Sep 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage approved these changes

Assignees

@epage epage

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Muscraft @rust-highfive @epage @bors @ehuss