Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` #10538

Muscraft · 2022-04-06T23:12:57Z

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

Part 1
Part 2

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)

rust-highfive · 2022-04-06T23:13:01Z

r? @ehuss

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

Muscraft · 2022-04-06T23:13:18Z

r? @epage

epage

Thanks for keeping this going!

src/cargo/core/workspace.rs

epage · 2022-04-07T17:51:22Z

src/cargo/util/toml/mod.rs

@@ -1021,6 +1022,12 @@ impl<T> MaybeWorkspace<T> {
            )),
        }
    }
+    fn get_defined(&self) -> CargoResult<&T> {
+        match self {
+            MaybeWorkspace::Workspace(_) => Err(anyhow!("MaybeWorkspace was Workspace")),


nit: I think I'd just do this as an as_defined(&self) -> Option<&T> and let the error message be completely controlled by the caller

I could go either way on this. If you feel its better I'll change it to something like this

src/cargo/util/toml/mod.rs

epage · 2022-04-07T18:02:36Z

src/cargo/util/toml/mod.rs

+                if let Some(diff_str) = diff_path.to_str() {
+                    self.path = Some(diff_str.to_owned());
+                }
+            }


If I'm reading this correctly, we are leaving the incorrect relative path in if it doesn't work?

Wonder if we should have a function shared between the the path rewriting so we can be consistent on error reporting.

You mean something like

pub fn resolve_relative_path(label: &str, root_path: Pathbuf, package_root: PathBuf) -> CargoResult<String> { match diff_paths(root_path, package_root) { None => Err(anyhow!("`workspace.{}` was defined but could not be resolved with {}", label, package_root.display())), Some(path) => Ok( path .to_str() .ok_or_else(|| anyhow!("`{}` resolved to non-UTF value (`{}`)", label, path.display()))? .to_owned() ), } }

I'd recommend having old_root, new_root, and rel_path as parameters so it can also handle join + normalize_path for you

workspace. doesn't quite work for path dependencies. Maybe just leave it as {label} and re-word to the first to include both paths

pub fn resolve_relative_path( label: &str, old_root: &PathBuf, new_root: &PathBuf, rel_path: &str, ) -> CargoResult<String> { let joined_path = normalize_path(&old_root.join(rel_path)); match diff_paths(joined_path, new_root) { None => Err(anyhow!( "`{}` was defined in {} but could not be resolved with {}", label, old_root.display(), package_root.display() )), Some(path) => Ok(path .to_str() .ok_or_else(|| { anyhow!( "`{}` resolved to non-UTF value (`{}`)", label, path.display() ) })? .to_owned()), } }

Seems like that'd work

epage · 2022-04-07T18:04:53Z

src/cargo/util/toml/mod.rs

+    fn resolve_path(&mut self, root_path: &Path, package_root: &Path, config: &Config) {
+        if let Some(from_root) = &self.path {
+            let resolved = from_root.resolve(config);
+            if let Some(diff_path) = diff_paths(root_path.join(resolved), package_root) {


Does diff_paths gracefully handle diffing /foo/bar/../baz with /foo/bee? Do we need to call cargo_utils::paths::normlize_path after doing the join?

I remember having to be particular about that in cargo-add but I don't remember if it was related to diff_paths or other stuff

(maybe another reason for a shared function, see below)

I don't know if it handles that well or not. I have a feeling that we will need to call cargo_utils::paths::normlize_path as well. I'll change it to this.

epage · 2022-04-07T18:06:11Z

tests/testsuite/inheritable_workspace_fields.rs

-          "license": null,
-          "license_file": null,
+          "license": "MIT",
+          "license_file": "[..]LICENSE",


I feel like we'll get a better idea if the tests are doing what we expect if they were to use [ROOT] instead of [..]

I'll change it to [ROOT], I agree that it gives a better idea of what is going on

I tried to do this and it didn't work for some reason. I just changed it for ../LICENSE

Oh right, its relative now. Thats what I'm looking for, thanks!

epage · 2022-04-07T18:07:30Z

src/cargo/util/toml/mod.rs

+}

+impl TomlDependency {


Why the new block?

Same reasons as for DetailedTomlDependency

src/cargo/core/workspace.rs

epage · 2022-04-07T20:54:18Z

src/cargo/core/workspace.rs

@@ -1645,7 +1646,7 @@ pub struct InheritableFields {
    keywords: Option<Vec<String>>,
    categories: Option<Vec<String>>,
    license: Option<String>,
-    license_file: Option<String>,
+    license_file: Option<PathBuf>,


Is there a reason this was changed or was it left over from the experiment?

It was from the experiment. It was done instead of resolving to a path, converting to a string, and then later converting back to a path. If you like the experiment we can keep it. It's up to you.

Let's go back

epage · 2022-04-07T20:55:19Z

src/cargo/core/workspace.rs

+    new_root: &PathBuf,
+    abs_path: &PathBuf,


Please take &Path instead of &PathBuf. This will also let you change &package_root.to_path_buf() to packaeg_root

Ill change it to this

epage · 2022-04-07T20:57:11Z

src/cargo/util/toml/mod.rs

@@ -2122,6 +2148,40 @@ impl TomlManifest {
    }
 }

+fn abs_path_for_deps(


Is this change also left over from the experiment?

Yes, I thought we were going ahead with that change so all of it is form the experiment

…epednency.path`

epage · 2022-04-08T15:38:02Z

Thanks!

@bors r+

bors · 2022-04-08T15:38:03Z

📌 Commit 3d07652 has been approved by epage

bors · 2022-04-08T15:38:10Z

⌛ Testing commit 3d07652 with merge b36cc6e...

bors · 2022-04-08T16:19:32Z

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

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

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)

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

rust-highfive assigned ehuss Apr 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2022

rust-highfive assigned epage and unassigned ehuss Apr 6, 2022

epage mentioned this pull request Apr 7, 2022

Tracking issue for Deduplicate Cargo workspace information RFC #8415

Closed

12 tasks

Muscraft force-pushed the rfc2906-part3 branch from 18f290e to d36f7c3 Compare April 7, 2022 16:15

Muscraft changed the title ~~Part 3 of RFC2906 - Correctly inherit fields and dependencies that are relative paths~~ Part 3 of RFC2906 - Add support for inheriting license-path, and `depednency.path Apr 7, 2022

Muscraft changed the title ~~Part 3 of RFC2906 - Add support for inheriting license-path, and `depednency.path~~ Part 3 of RFC2906 - Add support for inheriting license-path, and depednency.path Apr 7, 2022

epage reviewed Apr 7, 2022

View reviewed changes

Muscraft force-pushed the rfc2906-part3 branch from d36f7c3 to 3663259 Compare April 7, 2022 20:20

epage reviewed Apr 7, 2022

View reviewed changes

Part 3 of RFC2906 - Add support for inheriting license-path, and `d…

3d07652

…epednency.path`

Muscraft force-pushed the rfc2906-part3 branch from 3663259 to 3d07652 Compare April 7, 2022 21:34

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 merged commit b36cc6e into rust-lang:master Apr 8, 2022

bors mentioned this pull request Apr 8, 2022

feat: Import cargo-add into cargo #10472

Merged

Muscraft mentioned this pull request Apr 8, 2022

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

Merged

Muscraft mentioned this pull request Apr 13, 2022

Part 5 of RFC2906 - Add support for inheriting rust-version #10563

Merged

Muscraft mentioned this pull request Apr 13, 2022

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

Merged

Muscraft mentioned this pull request Apr 13, 2022

Part 7 of RFC2906 - Add support for inheriting exclude and include #10565

Merged

ehuss mentioned this pull request Apr 14, 2022

Update cargo rust-lang/rust#96031

Merged

Muscraft mentioned this pull request Apr 14, 2022

Part 8 of RFC2906 - Keep InheritableFields in a LazyCell inside `… #10568

Merged

Muscraft mentioned this pull request Apr 20, 2022

Prefer key.workspace = true to key = { workspace = true } #10584

Merged

Muscraft mentioned this pull request Apr 20, 2022

Allow running cargo add foo when a foo.workspace = true already exists #10585

Closed

ehuss added this to the 1.62.0 milestone Apr 22, 2022

Muscraft deleted the rfc2906-part3 branch May 27, 2022 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` #10538

Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` #10538

Muscraft commented Apr 6, 2022 •

edited

Loading

rust-highfive commented Apr 6, 2022

Muscraft commented Apr 6, 2022

epage left a comment

epage Apr 7, 2022

Muscraft Apr 7, 2022

epage Apr 7, 2022

Muscraft Apr 7, 2022

epage Apr 7, 2022

Muscraft Apr 7, 2022

epage Apr 7, 2022

epage Apr 7, 2022

Muscraft Apr 7, 2022

epage Apr 7, 2022

Muscraft Apr 7, 2022

Muscraft Apr 7, 2022

epage Apr 7, 2022

epage Apr 7, 2022

Muscraft Apr 7, 2022

epage Apr 7, 2022

Muscraft Apr 7, 2022

epage Apr 7, 2022

epage Apr 7, 2022

Muscraft Apr 7, 2022

epage Apr 7, 2022

Muscraft Apr 7, 2022

epage commented Apr 8, 2022

bors commented Apr 8, 2022

bors commented Apr 8, 2022

bors commented Apr 8, 2022

Part 3 of RFC2906 - Add support for inheriting license-path, and depednency.path #10538

Part 3 of RFC2906 - Add support for inheriting license-path, and depednency.path #10538

Conversation

Muscraft commented Apr 6, 2022 • edited Loading

rust-highfive commented Apr 6, 2022

Muscraft commented Apr 6, 2022

epage 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

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

Choose a reason for hiding this comment

epage commented Apr 8, 2022

bors commented Apr 8, 2022

bors commented Apr 8, 2022

bors commented Apr 8, 2022

Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` #10538

Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` #10538

Muscraft commented Apr 6, 2022 •

edited

Loading