Auto merge of #10548 - Muscraft:rfc2906-part4, r=epage

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`)

Author: bors

src/cargo/core/workspace.rs

@@ -23,7 +23,7 @@ use crate::util::errors::{CargoResult, ManifestError};
 use crate::util::interning::InternedString;
 use crate::util::lev_distance;
 use crate::util::toml::{
-    read_manifest, StringOrBool, TomlDependency, TomlProfiles, VecStringOrBool,
+    read_manifest, readme_for_project, StringOrBool, TomlDependency, TomlProfiles, VecStringOrBool,
 };
 use crate::util::{config::ConfigRelativePath, Config, Filesystem, IntoUrl};
 use cargo_util::paths;
@@ -1754,12 +1754,15 @@ impl InheritableFields {
         )
     }
 
-    pub fn readme(&self) -> CargoResult<StringOrBool> {
-        self.readme
-            .clone()
-            .map_or(Err(anyhow!("`workspace.readme` was not defined")), |d| {
-                Ok(d)
-            })
+    pub fn readme(&self, package_root: &Path) -> CargoResult<StringOrBool> {
+        readme_for_project(self.ws_root.as_path(), self.readme.clone()).map_or(
+            Err(anyhow!("`workspace.readme` was not defined")),
+            |readme| {
+                let rel_path =
+                    resolve_relative_path("readme", &self.ws_root, package_root, &readme)?;
+                Ok(StringOrBool::String(rel_path))
+            },
+        )
     }
 
     pub fn keywords(&self) -> CargoResult<Vec<String>> {

src/cargo/ops/cargo_package.rs

@@ -260,46 +260,16 @@ fn build_ar_list(
     }
     if let Some(license_file) = &pkg.manifest().metadata().license_file {
         let license_path = Path::new(license_file);
-        let abs_license_path = paths::normalize_path(&pkg.root().join(license_path));
-        if abs_license_path.exists() {
-            match abs_license_path.strip_prefix(&pkg.root()) {
-                Ok(rel_license_path) => {
-                    if !result.iter().any(|ar| ar.rel_path == rel_license_path) {
-                        result.push(ArchiveFile {
-                            rel_path: rel_license_path.to_path_buf(),
-                            rel_str: rel_license_path
-                                .to_str()
-                                .expect("everything was utf8")
-                                .to_string(),
-                            contents: FileContents::OnDisk(abs_license_path),
-                        });
-                    }
-                }
-                Err(_) => {
-                    // The license exists somewhere outside of the package.
-                    let license_name = license_path.file_name().unwrap();
-                    if result
-                        .iter()
-                        .any(|ar| ar.rel_path.file_name().unwrap() == license_name)
-                    {
-                        ws.config().shell().warn(&format!(
-                            "license-file `{}` appears to be a path outside of the package, \
-                            but there is already a file named `{}` in the root of the package. \
-                            The archived crate will contain the copy in the root of the package. \
-                            Update the license-file to point to the path relative \
-                            to the root of the package to remove this warning.",
-                            license_file,
-                            license_name.to_str().unwrap()
-                        ))?;
-                    } else {
-                        result.push(ArchiveFile {
-                            rel_path: PathBuf::from(license_name),
-                            rel_str: license_name.to_str().unwrap().to_string(),
-                            contents: FileContents::OnDisk(abs_license_path),
-                        });
-                    }
-                }
-            }
+        let abs_file_path = paths::normalize_path(&pkg.root().join(license_path));
+        if abs_file_path.exists() {
+            check_for_file_and_add(
+                "license-file",
+                license_path,
+                abs_file_path,
+                pkg,
+                &mut result,
+                ws,
+            )?;
         } else {
             let rel_msg = if license_path.is_absolute() {
                 "".to_string()
@@ -316,11 +286,69 @@ fn build_ar_list(
             ))?;
         }
     }
+    if let Some(readme) = &pkg.manifest().metadata().readme {
+        let readme_path = Path::new(readme);
+        let abs_file_path = paths::normalize_path(&pkg.root().join(readme_path));
+        if abs_file_path.exists() {
+            check_for_file_and_add("readme", readme_path, abs_file_path, pkg, &mut result, ws)?;
+        }
+    }
     result.sort_unstable_by(|a, b| a.rel_path.cmp(&b.rel_path));
 
     Ok(result)
 }
 
+fn check_for_file_and_add(
+    label: &str,
+    file_path: &Path,
+    abs_file_path: PathBuf,
+    pkg: &Package,
+    result: &mut Vec<ArchiveFile>,
+    ws: &Workspace<'_>,
+) -> CargoResult<()> {
+    match abs_file_path.strip_prefix(&pkg.root()) {
+        Ok(rel_file_path) => {
+            if !result.iter().any(|ar| ar.rel_path == rel_file_path) {
+                result.push(ArchiveFile {
+                    rel_path: rel_file_path.to_path_buf(),
+                    rel_str: rel_file_path
+                        .to_str()
+                        .expect("everything was utf8")
+                        .to_string(),
+                    contents: FileContents::OnDisk(abs_file_path),
+                })
+            }
+        }
+        Err(_) => {
+            // The file exists somewhere outside of the package.
+            let file_name = file_path.file_name().unwrap();
+            if result
+                .iter()
+                .any(|ar| ar.rel_path.file_name().unwrap() == file_name)
+            {
+                ws.config().shell().warn(&format!(
+                    "{} `{}` appears to be a path outside of the package, \
+                            but there is already a file named `{}` in the root of the package. \
+                            The archived crate will contain the copy in the root of the package. \
+                            Update the {} to point to the path relative \
+                            to the root of the package to remove this warning.",
+                    label,
+                    file_path.display(),
+                    file_name.to_str().unwrap(),
+                    label,
+                ))?;
+            } else {
+                result.push(ArchiveFile {
+                    rel_path: PathBuf::from(file_name),
+                    rel_str: file_name.to_str().unwrap().to_string(),
+                    contents: FileContents::OnDisk(abs_file_path),
+                })
+            }
+        }
+    }
+    Ok(())
+}
+
 /// Construct `Cargo.lock` for the package to be published.
 fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult<String> {
     let config = ws.config();

src/cargo/util/toml/mod.rs

@@ -1073,7 +1073,7 @@ pub struct TomlProject {
     description: Option<MaybeWorkspace<String>>,
     homepage: Option<MaybeWorkspace<String>>,
     documentation: Option<MaybeWorkspace<String>>,
-    readme: Option<StringOrBool>,
+    readme: Option<MaybeWorkspace<StringOrBool>>,
     keywords: Option<MaybeWorkspace<Vec<String>>>,
     categories: Option<MaybeWorkspace<Vec<String>>>,
     license: Option<MaybeWorkspace<String>>,
@@ -1175,6 +1175,31 @@ impl TomlManifest {
                 ));
             }
         }
+
+        if let Some(readme) = &package.readme {
+            let readme = readme
+                .as_defined()
+                .context("readme should have been resolved before `prepare_for_publish()`")?;
+            match readme {
+                StringOrBool::String(readme) => {
+                    let readme_path = Path::new(&readme);
+                    let abs_readme_path = paths::normalize_path(&package_root.join(readme_path));
+                    if abs_readme_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.readme = Some(MaybeWorkspace::Defined(StringOrBool::String(
+                            readme_path
+                                .file_name()
+                                .unwrap()
+                                .to_str()
+                                .unwrap()
+                                .to_string(),
+                        )));
+                    }
+                }
+                StringOrBool::Bool(_) => {}
+            }
+        }
         let all = |_d: &TomlDependency| true;
         return Ok(TomlManifest {
             package: Some(package),
@@ -1693,7 +1718,19 @@ impl TomlManifest {
                     })
                 })
                 .transpose()?,
-            readme: readme_for_project(package_root, project),
+            readme: readme_for_project(
+                package_root,
+                project
+                    .readme
+                    .clone()
+                    .map(|mw| {
+                        mw.resolve(&features, "readme", || {
+                            get_ws(config, resolved_path.clone(), workspace_config.clone())?
+                                .readme(package_root)
+                        })
+                    })
+                    .transpose()?,
+            ),
             authors: project
                 .authors
                 .clone()
@@ -1778,6 +1815,10 @@ impl TomlManifest {
             .documentation
             .clone()
             .map(|documentation| MaybeWorkspace::Defined(documentation));
+        project.readme = metadata
+            .readme
+            .clone()
+            .map(|readme| MaybeWorkspace::Defined(StringOrBool::String(readme)));
         project.authors = project
             .authors
             .as_ref()
@@ -2164,8 +2205,8 @@ fn inheritable_from_path(
 }
 
 /// Returns the name of the README file for a `TomlProject`.
-fn readme_for_project(package_root: &Path, project: &TomlProject) -> Option<String> {
-    match &project.readme {
+pub fn readme_for_project(package_root: &Path, readme: Option<StringOrBool>) -> Option<String> {
+    match &readme {
         None => default_readme_from_package_root(package_root),
         Some(value) => match value {
             StringOrBool::Bool(false) => None,

tests/testsuite/inheritable_workspace_fields.rs

@@ -581,6 +581,7 @@ fn inherit_workspace_fields() {
             authors = ["Rustaceans"]
             description = "This is a crate"
             documentation = "https://www.rust-lang.org/learn"
+            readme = "README.md"
             homepage = "https://www.rust-lang.org"
             repository = "https://github.com/example/example"
             license = "MIT"
@@ -606,6 +607,7 @@ fn inherit_workspace_fields() {
             authors = { workspace = true }
             description = { workspace = true }
             documentation = { workspace = true }
+            readme = { workspace = true }
             homepage = { workspace = true }
             repository = { workspace = true }
             license = { workspace = true }
@@ -617,6 +619,7 @@ fn inherit_workspace_fields() {
         "#,
         )
         .file("LICENSE", "license")
+        .file("README.md", "README.md")
         .file("bar/src/main.rs", "fn main() {}")
         .build();
 
@@ -642,8 +645,8 @@ fn inherit_workspace_fields() {
           "license_file": "../LICENSE",
           "links": null,
           "name": "bar",
-          "readme": null,
-          "readme_file": null,
+          "readme": "README.md",
+          "readme_file": "../README.md",
           "repository": "https://github.com/example/example",
           "vers": "1.2.3"
           }
@@ -654,6 +657,7 @@ fn inherit_workspace_fields() {
             "Cargo.toml",
             "Cargo.toml.orig",
             "src/main.rs",
+            "README.md",
             "LICENSE",
             ".cargo_vcs_info.json",
         ],
@@ -672,6 +676,7 @@ publish = true
 description = "This is a crate"
 homepage = "https://www.rust-lang.org"
 documentation = "https://www.rust-lang.org/learn"
+readme = "README.md"
 keywords = ["cli"]
 categories = ["development-tools"]
 license = "MIT"