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 4 of RFC2906 - Add support for inheriting readme #10548

Merged
merged 2 commits into from
Apr 8, 2022
Merged

Part 4 of RFC2906 - Add support for inheriting readme #10548

merged 2 commits into from
Apr 8, 2022

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented Apr 8, 2022

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

Part 1
Part 2
Part 3

This PR focuses on adding support for inheriting readme:

  • Added adjusting of a readme path when it is outside of the package_root
    • Copied from license-file's implementation in toml::prepare_for_publish()
  • Added copying of a readme file when it is outside of the package_root for publishing
    • Copied from license-file's implementation in cargo_package::build_ar_list()
  • Merged copying of a file outside of a package_root to reduce duplicated code and allow for other files in the future to be copied in a similar way

Remaining implementation work for the RFC

  • Path dependencies infer version directive
  • Lock workspace dependencies and warn when unused
  • Optimizations, as needed
  • Evaluate any new fields for being inheritable (e.g. rust-version)

@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 8, 2022
@Muscraft
Copy link
Member Author

Muscraft commented Apr 8, 2022

r? @epage

@rust-highfive rust-highfive assigned epage and unassigned ehuss Apr 8, 2022
@epage epage mentioned this pull request Apr 8, 2022
Closed
12 tasks
epage
epage reviewed Apr 8, 2022
View reviewed changes
Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/cargo/ops/cargo_package.rs Show resolved Hide resolved
src/cargo/ops/cargo_package.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Show resolved Hide resolved
epage
epage approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@epage
Copy link
Contributor

epage commented Apr 8, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 8, 2022

📌 Commit 3bbb781 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 8, 2022
@bors
Copy link
Collaborator

bors commented Apr 8, 2022

⌛ Testing commit 3bbb781 with merge fc5035d...

@bors
Copy link
Collaborator

bors commented Apr 8, 2022

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

@bors bors merged commit fc5035d into rust-lang:master Apr 8, 2022
@Muscraft Muscraft mentioned this pull request Apr 13, 2022
Merged
bors added a commit that referenced this pull request Apr 13, 2022
@bors
Auto merge of #10563 - Muscraft:rfc2906-part5, r=epage
de4bd68 
Part 5 of RFC2906 - Add support for inheriting `rust-version`

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

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

This PR focuses on adding support for inheriting `rust-version`. While this was not in the original RFC it was decided that it should be added per [epage's comment](#8415 (comment)).
- Changed `rust-version` to `Option<MaybeWorkspace<String>>` in `TomlProject`
- Added `rust-version` to `TomlWorkspace` to allow for inheritance
- Added `rust-version` to `InheritableFields1 and a method to get it as needed
- Updated tests to include `rust-version`

Remaining implementation work for the RFC
- Switch the inheritance source from `workspace` to `workspace.package`, see [epage's comment](#8415 (comment))
- Add `include` and `exclude` now that `workspace.package` is done, see [epage's comment](#8415 (comment))
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations, as needed
@Muscraft Muscraft mentioned this pull request Apr 13, 2022
Merged
bors added a commit that referenced this pull request Apr 13, 2022
@bors
Auto merge of #10564 - Muscraft:rfc2906-part6, r=epage
dba5baf 
Part 6 of RFC2906 - Switch the inheritance source from `workspace` to…

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](#8415 (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
- Add `include` and `exclude` now that `workspace.package` is done, see [epage's comment](#8415 (comment))
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations, as needed
@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
@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-part4 branch May 27, 2022 01:16
@epage epage mentioned this pull request Jun 6, 2024
Open
@epage epage mentioned this pull request Aug 30, 2024
Closed
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