Auto merge of #10538 - Muscraft:rfc2906-part3, r=epage

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

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

[Part 1](https://github.com/rust-lang/cargo/pull/10497)
[Part 2](https://github.com/rust-lang/cargo/pull/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`)

Cargo.toml

@@ -45,6 +45,7 @@ libgit2-sys = "0.13.2"
 memchr = "2.1.3"
 opener = "0.5"
 os_info = "3.0.7"
+pathdiff = "0.2.1"
 percent-encoding = "2.0"
 rustfix = "0.6.0"
 semver = { version = "1.0.3", features = ["serde"] }

src/cargo/core/mod.rs

@@ -11,8 +11,8 @@ pub use self::shell::{Shell, Verbosity};
 pub use self::source::{GitReference, Source, SourceId, SourceMap};
 pub use self::summary::{FeatureMap, FeatureValue, Summary};
 pub use self::workspace::{
-    find_workspace_root, InheritableFields, MaybePackage, Workspace, WorkspaceConfig,
+    find_workspace_root, resolve_relative_path, InheritableFields, MaybePackage, Workspace,
-    WorkspaceRootConfig,
+    WorkspaceConfig, WorkspaceRootConfig,
 };
 
 pub mod compiler;

src/cargo/core/workspace.rs

@@ -27,6 +27,8 @@ use crate::util::toml::{
 };
 use crate::util::{config::ConfigRelativePath, Config, Filesystem, IntoUrl};
 use cargo_util::paths;
+use cargo_util::paths::normalize_path;
+use pathdiff::diff_paths;
 
 /// The core abstraction in Cargo for working with a workspace of crates.
 ///
@@ -1650,6 +1652,7 @@ pub struct InheritableFields {
     publish: Option<VecStringOrBool>,
     edition: Option<String>,
     badges: Option<BTreeMap<String, BTreeMap<String, String>>>,
+    ws_root: PathBuf,
 }
 
 impl InheritableFields {
@@ -1669,6 +1672,7 @@ impl InheritableFields {
         publish: Option<VecStringOrBool>,
         edition: Option<String>,
         badges: Option<BTreeMap<String, BTreeMap<String, String>>>,
+        ws_root: PathBuf,
     ) -> InheritableFields {
         Self {
             dependencies,
@@ -1686,6 +1690,7 @@ impl InheritableFields {
             publish,
             edition,
             badges,
+            ws_root,
         }
     }
 
@@ -1780,10 +1785,10 @@ impl InheritableFields {
             })
     }
 
-    pub fn license_file(&self) -> CargoResult<String> {
+    pub fn license_file(&self, package_root: &Path) -> CargoResult<String> {
         self.license_file.clone().map_or(
             Err(anyhow!("`workspace.license_file` was not defined")),
-            |d| Ok(d),
+            |d| resolve_relative_path("license-file", &self.ws_root, package_root, &d),
         )
     }
 
@@ -1817,6 +1822,37 @@ impl InheritableFields {
                 Ok(d)
             })
     }
+
+    pub fn ws_root(&self) -> &PathBuf {
+        &self.ws_root
+    }
+}
+
+pub fn resolve_relative_path(
+    label: &str,
+    old_root: &Path,
+    new_root: &Path,
+    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(),
+            new_root.display()
+        )),
+        Some(path) => Ok(path
+            .to_str()
+            .ok_or_else(|| {
+                anyhow!(
+                    "`{}` resolved to non-UTF value (`{}`)",
+                    label,
+                    path.display()
+                )
+            })?
+            .to_owned()),
+    }
 }
 
 fn parse_manifest(manifest_path: &Path, config: &Config) -> CargoResult<EitherManifest> {

src/cargo/util/toml/mod.rs

@@ -20,7 +20,9 @@ use crate::core::compiler::{CompileKind, CompileTarget};
 use crate::core::dependency::{Artifact, ArtifactTarget, DepKind};
 use crate::core::manifest::{ManifestMetadata, TargetSourcePath, Warnings};
 use crate::core::resolver::ResolveBehavior;
-use crate::core::{find_workspace_root, Dependency, Manifest, PackageId, Summary, Target};
+use crate::core::{
+    find_workspace_root, resolve_relative_path, Dependency, Manifest, PackageId, Summary, Target,
+};
 use crate::core::{
     Edition, EitherManifest, Feature, Features, InheritableFields, VirtualManifest, Workspace,
 };
@@ -1021,6 +1023,12 @@ impl<T> MaybeWorkspace<T> {
             )),
         }
     }
+    fn as_defined(&self) -> Option<&T> {
+        match self {
+            MaybeWorkspace::Workspace(_) => None,
+            MaybeWorkspace::Defined(defined) => Some(defined),
+        }
+    }
 }
 
 #[derive(Deserialize, Serialize, Clone, Debug)]
@@ -1069,7 +1077,7 @@ pub struct TomlProject {
     keywords: Option<MaybeWorkspace<Vec<String>>>,
     categories: Option<MaybeWorkspace<Vec<String>>>,
     license: Option<MaybeWorkspace<String>>,
-    license_file: Option<String>,
+    license_file: Option<MaybeWorkspace<String>>,
     repository: Option<MaybeWorkspace<String>>,
     resolver: Option<String>,
 
@@ -1149,19 +1157,22 @@ impl TomlManifest {
         package.workspace = None;
         package.resolver = ws.resolve_behavior().to_manifest();
         if let Some(license_file) = &package.license_file {
+            let license_file = license_file
+                .as_defined()
+                .context("license file should have been resolved before `prepare_for_publish()`")?;
             let license_path = Path::new(&license_file);
             let abs_license_path = paths::normalize_path(&package_root.join(license_path));
             if abs_license_path.strip_prefix(package_root).is_err() {
                 // This path points outside of the package root. `cargo package`
                 // will copy it into the root, so adjust the path to this location.
-                package.license_file = Some(
+                package.license_file = Some(MaybeWorkspace::Defined(
                     license_path
                         .file_name()
                         .unwrap()
                         .to_str()
                         .unwrap()
                         .to_string(),
-                );
+                ));
             }
         }
         let all = |_d: &TomlDependency| true;
@@ -1340,6 +1351,7 @@ impl TomlManifest {
                     config.publish.clone(),
                     config.edition.clone(),
                     config.badges.clone(),
+                    package_root.to_path_buf(),
                 );
 
                 WorkspaceConfig::Root(WorkspaceRootConfig::new(
@@ -1506,13 +1518,12 @@ impl TomlManifest {
             };
             let mut deps: BTreeMap<String, TomlDependency> = BTreeMap::new();
             for (n, v) in dependencies.iter() {
-                let resolved = v.clone().resolve(features, n, || {
+                let resolved = v.clone().resolve(features, n, cx, || {
                     get_ws(
                         cx.config,
                         cx.root.join("Cargo.toml"),
                         workspace_config.clone(),
-                    )?
+                    )
-                    .get_dependency(n)
                 })?;
                 let dep = resolved.to_dependency(n, cx, kind)?;
                 validate_package_name(dep.name_in_toml().as_str(), "dependency name", "")?;
@@ -1702,7 +1713,16 @@ impl TomlManifest {
                     })
                 })
                 .transpose()?,
-            license_file: project.license_file.clone(),
+            license_file: project
+                .license_file
+                .clone()
+                .map(|mw| {
+                    mw.resolve(&features, "license", || {
+                        get_ws(config, resolved_path.clone(), workspace_config.clone())?
+                            .license_file(package_root)
+                    })
+                })
+                .transpose()?,
             repository: project
                 .repository
                 .clone()
@@ -1766,6 +1786,10 @@ impl TomlManifest {
             .license
             .clone()
             .map(|license| MaybeWorkspace::Defined(license));
+        project.license_file = metadata
+            .license_file
+            .clone()
+            .map(|license_file| MaybeWorkspace::Defined(license_file));
         project.repository = metadata
             .repository
             .clone()
@@ -1999,6 +2023,7 @@ impl TomlManifest {
                     config.publish.clone(),
                     config.edition.clone(),
                     config.badges.clone(),
+                    root.to_path_buf(),
                 );
                 WorkspaceConfig::Root(WorkspaceRootConfig::new(
                     root,
@@ -2240,13 +2265,16 @@ impl<P: ResolveToPath + Clone> TomlDependency<P> {
             TomlDependency::Workspace(w) => w.optional.unwrap_or(false),
         }
     }
+}
 
+impl TomlDependency {
     fn resolve<'a>(
         self,
         cargo_features: &Features,
         label: &str,
-        get_ws_dependency: impl FnOnce() -> CargoResult<TomlDependency<P>>,
+        cx: &mut Context<'_, '_>,
-    ) -> CargoResult<TomlDependency<P>> {
+        get_inheritable: impl FnOnce() -> CargoResult<InheritableFields>,
+    ) -> CargoResult<TomlDependency> {
         match self {
             TomlDependency::Detailed(d) => Ok(TomlDependency::Detailed(d)),
             TomlDependency::Simple(s) => Ok(TomlDependency::Simple(s)),
@@ -2256,28 +2284,30 @@ impl<P: ResolveToPath + Clone> TomlDependency<P> {
                 optional,
             }) => {
                 cargo_features.require(Feature::workspace_inheritance())?;
-                get_ws_dependency().context(format!(
+                let inheritable = get_inheritable()?;
+                inheritable.get_dependency(label).context(format!(
                     "error reading `dependencies.{}` from workspace root manifest's `workspace.dependencies.{}`",
                     label, label
                 )).map(|dep| {
                     match dep {
                         TomlDependency::Simple(s) => {
                             if optional.is_some() || features.is_some() {
-                                TomlDependency::Detailed(DetailedTomlDependency::<P> {
+                                Ok(TomlDependency::Detailed(DetailedTomlDependency {
                                     version: Some(s),
                                     optional,
                                     features,
                                     ..Default::default()
-                                })
+                                }))
                             } else {
-                                TomlDependency::Simple(s)
+                                Ok(TomlDependency::Simple(s))
                             }
                         },
                         TomlDependency::Detailed(d) => {
                             let mut dep = d.clone();
                             dep.add_features(features);
                             dep.update_optional(optional);
-                            TomlDependency::Detailed(dep)
+                            dep.resolve_path(label,inheritable.ws_root(), cx.root)?;
+                            Ok(TomlDependency::Detailed(dep))
                         },
                         TomlDependency::Workspace(_) => {
                             unreachable!(
@@ -2288,7 +2318,7 @@ impl<P: ResolveToPath + Clone> TomlDependency<P> {
                             )
                         },
                     }
-                })
+                })?
             }
             TomlDependency::Workspace(TomlWorkspaceDependency {
                 workspace: false, ..
@@ -2543,7 +2573,9 @@ impl<P: ResolveToPath + Clone> DetailedTomlDependency<P> {
         }
         Ok(dep)
     }
+}
 
+impl DetailedTomlDependency {
     fn add_features(&mut self, features: Option<Vec<String>>) {
         self.features = match (self.features.clone(), features.clone()) {
             (Some(dep_feat), Some(inherit_feat)) => Some(
@@ -2561,6 +2593,23 @@ impl<P: ResolveToPath + Clone> DetailedTomlDependency<P> {
     fn update_optional(&mut self, optional: Option<bool>) {
         self.optional = optional;
     }
+
+    fn resolve_path(
+        &mut self,
+        name: &str,
+        root_path: &Path,
+        package_root: &Path,
+    ) -> CargoResult<()> {
+        if let Some(rel_path) = &self.path {
+            self.path = Some(resolve_relative_path(
+                name,
+                root_path,
+                package_root,
+                rel_path,
+            )?)
+        }
+        Ok(())
+    }
 }
 
 #[derive(Default, Serialize, Deserialize, Debug, Clone)]

tests/testsuite/inheritable_workspace_fields.rs

@@ -1,6 +1,8 @@
 //! Tests for inheriting Cargo.toml fields with { workspace = true }
 use cargo_test_support::registry::{Dependency, Package};
-use cargo_test_support::{basic_lib_manifest, git, path2url, paths, project, publish, registry};
+use cargo_test_support::{
+    basic_lib_manifest, basic_manifest, git, path2url, paths, project, publish, registry,
+};
 
 #[cargo_test]
 fn permit_additional_workspace_fields() {
@@ -581,6 +583,8 @@ fn inherit_workspace_fields() {
             documentation = "https://www.rust-lang.org/learn"
             homepage = "https://www.rust-lang.org"
             repository = "https://github.com/example/example"
+            license = "MIT"
+            license-file = "LICENSE"
             keywords = ["cli"]
             categories = ["development-tools"]
             publish = true
@@ -604,12 +608,15 @@ fn inherit_workspace_fields() {
             documentation = { workspace = true }
             homepage = { workspace = true }
             repository = { workspace = true }
+            license = { workspace = true }
+            license-file = { workspace = true }
             keywords = { workspace = true }
             categories = { workspace = true }
             publish = { workspace = true }
             edition = { workspace = true }
         "#,
         )
+        .file("LICENSE", "license")
         .file("bar/src/main.rs", "fn main() {}")
         .build();
 
@@ -631,8 +638,8 @@ fn inherit_workspace_fields() {
           "features": {},
           "homepage": "https://www.rust-lang.org",
           "keywords": ["cli"],
-          "license": null,
+          "license": "MIT",
-          "license_file": null,
+          "license_file": "../LICENSE",
           "links": null,
           "name": "bar",
           "readme": null,
@@ -647,6 +654,7 @@ fn inherit_workspace_fields() {
             "Cargo.toml",
             "Cargo.toml.orig",
             "src/main.rs",
+            "LICENSE",
             ".cargo_vcs_info.json",
         ],
         &[(
@@ -666,6 +674,8 @@ homepage = "https://www.rust-lang.org"
 documentation = "https://www.rust-lang.org/learn"
 keywords = ["cli"]
 categories = ["development-tools"]
+license = "MIT"
+license-file = "LICENSE"
 repository = "https://github.com/example/example"
 
 [badges.gitlab]
@@ -1038,6 +1048,52 @@ fn inherit_detailed_dependencies() {
         .run();
 }
 
+#[cargo_test]
+fn inherit_path_dependencies() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [workspace]
+            members = ["bar"]
+            [workspace.dependencies]
+            dep = { path = "dep" }
+        "#,
+        )
+        .file(
+            "bar/Cargo.toml",
+            r#"
+            cargo-features = ["workspace-inheritance"]
+
+            [project]
+            workspace = ".."
+            name = "bar"
+            version = "0.2.0"
+            authors = []
+            [dependencies]
+            dep = { workspace = true }
+        "#,
+        )
+        .file("bar/src/main.rs", "fn main() {}")
+        .file("dep/Cargo.toml", &basic_manifest("dep", "0.9.0"))
+        .file("dep/src/lib.rs", "")
+        .build();
+
+    p.cargo("build")
+        .masquerade_as_nightly_cargo()
+        .with_stderr(
+            "\
+[COMPILING] dep v0.9.0 ([CWD]/dep)
+[COMPILING] bar v0.2.0 ([CWD]/bar)
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
+",
+        )
+        .run();
+
+    let lockfile = p.read_lockfile();
+    assert!(lockfile.contains("dep"));
+}
+
 #[cargo_test]
 fn error_workspace_false() {
     registry::init();