-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Part 1 of RFC2906 - Packages can inherit fields from their root workspace #10497
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. Please see the contribution instructions for more information. |
As a heads up the "miscellaneous" aspect of part 3 is about to grow. I think it could make sense to break this out into a step 4. I could help out of implement that part. I've recorded more details on the tracking issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! I'm very excited by this feature!
I'm taking a break for now; all I have left are the tests.
Could you be more explicit in what the parts are? I might have missed it but looking at the past comments, I'm not seeing "Path dependencies infer version directive" mentioned and wanting to make sure that we are taking that into account for closing out the RFC.
I edited my original comment/explanation to better explain what is happening in each part. Let me know if I need to change anything or make the parts clearer, |
Thanks! Its helpful when reviewing to have the full context of what is expected to be in scope and out of scope so I know what to look for. I'm thinking it would be good to break up Part 2 to be more focused so the review process is easier for everyone. My suggestion for different PRs
These are
When PRs try to fulfill too many purposes
|
Support inheriting in any workspace member, and Correctly inherit fields that are relative paths to me are fairly tied together since this would delay If we were to split these into Part 2 and Part 3, I think Part 1 should remove I'm flexible on how we want to go about this and will happily do whatever is easiest to review. |
As I said, I'm fine with it if you want to remove them from this PR.
Let's move forward with this then so this has the best chances of making it in! |
-- `resolve` not takes `get_ws: impl FnOnce() -> CargoResult<T>` -- removed `MaybeWorkspace` from `readme` and `license-file` -- changed error messages and test names
Let me know your thoughts on the newest commit, I believe I made all the changes you had mentioned so far. Changing to a What are your thoughts on adding I will update what is being done in the parts in the main part of my PR so others can see what is going on easier. |
I think it looks great. Putting it on the inheritable fields helps keep all the error reporting in one place so we can keep them in sync.
I think this is a wider conversation that should be addressed independent of this PR. I've gone ahead and updated the tracking issue with a check box for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got to looking at the tests
@Muscraft I hope you don't mind, I made some edits to the PR description to try make the intent clearer to bring others on board more quickly as I share this. I also updated the relative path handling to include path-dependencies on top of Looking over the RFC, a couple more items I've noticed that we don't have covered
|
Would these not be covered by other tests in cargo? or are you meaning explicit tests that have
The only way I think we could keep them more in sync would be to make a function in fn error(&self, field: &str) -> anyhow::Error {
anyhow!("`workspace.{}` was not defined", field)
} then each getter would be similar to: pub fn dependencies(&self) -> CargoResult<BTreeMap<String, TomlDependency>> {
self.dependencies.clone().map_or(
Err(self.error("dependencies")),
|d| Ok(d),
)
} This is nitpicky and doesn't have to be done. The real thing would be to get rid of the very similar map_or(Err(anyhow!("message")), |i| Ok(i))) I tried to get rid of this but I made things a bit odd in my brief attempt. |
Yes, I was thinking of when they are using
If consolidating the error message makes the code worse, I'm personally fine having it in each function. Having them all in one tight location where its obvious that you should update others when updating one is a big help! |
Sounds good, I saw you added it to the RFC so I will add the tests for it then. |
-- removed `[]` from error messages in favor of back-ticks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work on this!
Anytime! It's been something I've been meaning to work on for a while and got time to do so. I will start work on Part 2 once this gets merged but it may take me a bit to get reacclimated with what I need to do. |
Going ahead with it as its feature gated, well tested, and should have minimal impact for existing users. @bors r+ |
📌 Commit e6992d4 has been approved by |
Update cargo 13 commits in 109bfbd055325ef87a6e7f63d67da7e838f8300b..1ef1e0a12723ce9548d7da2b63119de9002bead8 2022-03-17 21:43:09 +0000 to 2022-03-31 00:17:18 +0000 - Support `-Zmultitarget` in cargo config (rust-lang/cargo#10473) - doc: Fix document url for libcurl format (rust-lang/cargo#10515) - Fix wrong info in "Environment variables" docs (rust-lang/cargo#10513) - Use the correct flag in --locked --offline error message (rust-lang/cargo#10512) - Don't treat host/target duplicates as duplicates (rust-lang/cargo#10466) - Unstable --keep-going flag (rust-lang/cargo#10383) - Part 1 of RFC2906 - Packages can inherit fields from their root workspace (rust-lang/cargo#10497) - Remove unused profile support for -Zpanic-abort-tests (rust-lang/cargo#10495) - HTTP registry implementation (rust-lang/cargo#10470) - Add a notice about review capacity. (rust-lang/cargo#10501) - Add tests for ignoring symlinks (rust-lang/cargo#10047) - Update doc string for deps_of/compute_deps. (rust-lang/cargo#10494) - Consistently use crate::display_error on errors during drain (rust-lang/cargo#10394)
Part 2 of RFC2906 -- allow inheriting from a different `Cargo.toml` Tracking issue: #8415 RFC: rust-lang/rfcs#2906 [Part 1](#10497) This PR focuses on inheriting from a root workspace: - Allow inheriting from a different `Cargo.toml` - Add in searching for a workspace root in `to_real_manifest` as needed - Fixed problem where a package would try to pull a dependency from a workspace and specify `{ workspace = true, optional = true }` and it would not respect the `optional` - Added tests to verify everything is in working order Remaining implementation work for the RFC - Correctly inherit fields that are relative paths - Including adding support for inheriting `license_file`, `readme`, and path-dependencies - 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`) Problems: - There is duplication of code that can't be removed without significant refactoring - Potential to parse the same manifest many times when searching for a root - This should not happen when a `[package]` specifies its workspace - This should only happen if the workspace root is greater than one folder above
Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` Tracking issue: #8415 RFC: rust-lang/rfcs#2906 [Part 1](#10497) [Part 2](#10517) This PR focuses on adding support for inheriting `license-path`, and `depednency.path`: - To adjust the relative paths from being workspace-relative to package-relative, we use `pathdiff` which `cargo-add` is also going to be using for a similar purpose - `ws_path` was added to `InheritableFields` so we can resolve relative paths from workspace-relative to package-relative - Moved `resolve` for toml dependencies from `TomlDependency::<P>` to `TomlDependency` - This was done since resolving a relative path should be a string - You should never inherit from a `.cargo/config.toml` which is the reason `P` was added Remaining implementation work for the RFC - Relative paths for `readme` - 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`)
Part 4 of RFC2906 - Add support for inheriting `readme` Tracking issue: #8415 RFC: rust-lang/rfcs#2906 [Part 1](#10497) [Part 2](#10517) [Part 3](#10538) 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`)
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
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
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
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
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
Tracking issue: #8415
RFC: rust-lang/rfcs#2906
All changes were heavily inspired by #8664 and #9684
A big thanks to both @yaymukund and @ParkMyCar. I pulled a lot of inspiration from their branches.
I would also like to thank @alexcrichton for all the feedback on the first attempt. It has brought this into a much better state.
All changes have been made according to the RFC as well as @alexcrichton's comment.
This is part 1 of many, as described by this comment, this comment and redefined by this one.
This PR focuses on inheriting in root package, including:
MaybeWorkspace<T>
to allow for{ workspace = true }
TomlDependency
to allow inheriting dependencies from a workspaceInheritableFields
so that we have somewhere to get a value from when weresolve
aMaybeWorkspace<T>
license_file
andreadme
are inInheritableFields
in this part but are notMaybeWorkspace
for reasons described hereresolve
aMaybeWorkspace<T>
into aT
that can fail if we have nowhere to pull from or the workspace does not define that fieldCargo.toml
Cargo.toml
, i.e. it has both a[workspace]
and a[package]
MaybeWorkspace<T>
and the newTomlDependency
variant without having to add searching for a workspace root and the complexity with itRemaining implementation work for the RFC
license_file
,readme
, and path-dependenciesrust-version
){ workspace = true }
define the workspace they pull from in[package]
Areas of concern:
TomlDependency
Deserialization is a mess, and I could not figure out how to do it in a cleaner way without significant headaches. I ended up having to do the same thing as last time which was an issue.Resolving on a
MaybeWorkspace
feels extremely verbose currently:This way allows for lazy resolution of inheritable fields and finding a workspace (in part 2) so you do not pay a penalty for not using this feature. The other bit of good news is
&features
will go away as it is just feature checking currently.This feature requires some level of duplication of code as well as remaking the original
TomlManifest
which adds extra length to the changes.This feature also takes a linear process and makes it potentially non-linear which adds lots of complexity (which will get worse in Part 2)
Please let me know if you have any feedback or suggestions!