Auto merge of #13921 - heisen-li:licence_readme_warning, r=<try>

fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors ### What does this PR try to resolve? In this PR: - Changed the warning to a hard error and modified the associated test function; - Removed what should have been a redundant test function：`publish::publish_with_missing_readme`； - Since `cargo publish` is preceded by the execution of `cargo package`, the error message in the test `function bad_license_file` needs to be modified. issue: #13629 (comment). ### Additional information It seems that this is not enough, the current situation is that `cargo package` warns if `package.readme` is an empty string or the wrong file location, but if I cancel `package.readme`, no warning is generated. I'm wondering if I should judge `package.readme&licence` when executing `cargo package` and return an error if it doesn't exist? As this has not been done before, your advice is sought.
rust-lang · May 29, 2024 · 2faa4a0 · 2faa4a0
2 parents cbc12a2 + 5c35e29
commit 2faa4a0
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 106 deletions.
diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs
@@ -310,6 +310,8 @@ fn build_ar_list(
             });
     }
 
+    let mut invalid_manifest_field: Vec<String> = vec![];
+
     let mut result = result.into_values().flatten().collect();
     if let Some(license_file) = &pkg.manifest().metadata().license_file {
         let license_path = Path::new(license_file);
@@ -324,7 +326,12 @@ fn build_ar_list(
                 ws,
             )?;
         } else {
-            warn_on_nonexistent_file(&pkg, &license_path, "license-file", &ws)?;
+            error_on_nonexistent_file(
+                &pkg,
+                &license_path,
+                "license-file",
+                &mut invalid_manifest_field,
+            );
         }
     }
     if let Some(readme) = &pkg.manifest().metadata().readme {
@@ -333,10 +340,14 @@ fn build_ar_list(
         if abs_file_path.is_file() {
             check_for_file_and_add("readme", readme_path, abs_file_path, pkg, &mut result, ws)?;
         } else {
-            warn_on_nonexistent_file(&pkg, &readme_path, "readme", &ws)?;
+            error_on_nonexistent_file(&pkg, &readme_path, "readme", &mut invalid_manifest_field);
         }
     }
 
+    if !invalid_manifest_field.is_empty() {
+        return Err(anyhow::anyhow!(invalid_manifest_field.join("\n")));
+    }
+
     for t in pkg
         .manifest()
         .targets()
@@ -406,25 +417,27 @@ fn check_for_file_and_add(
     Ok(())
 }
 
-fn warn_on_nonexistent_file(
+fn error_on_nonexistent_file(
     pkg: &Package,
     path: &Path,
     manifest_key_name: &'static str,
-    ws: &Workspace<'_>,
-) -> CargoResult<()> {
+    invalid: &mut Vec<String>,
+) {
     let rel_msg = if path.is_absolute() {
         "".to_string()
     } else {
         format!(" (relative to `{}`)", pkg.root().display())
     };
-    ws.gctx().shell().warn(&format!(
+
+    let msg = format!(
         "{manifest_key_name} `{}` does not appear to exist{}.\n\
-                Please update the {manifest_key_name} setting in the manifest at `{}`\n\
-                This may become a hard error in the future.",
+                Please update the {manifest_key_name} setting in the manifest at `{}`.",
         path.display(),
         rel_msg,
         pkg.manifest_path().display()
-    ))
+    );
+
+    invalid.push(msg);
 }
 
 fn error_custom_build_file_not_in_package(
@@ -500,11 +513,7 @@ fn check_metadata(pkg: &Package, gctx: &GlobalContext) -> CargoResult<()> {
             )*
         }}
     }
-    lacking!(
-        description,
-        license || license_file,
-        documentation || homepage || repository
-    );
+    lacking!(description, documentation || homepage || repository);
 
     if !missing.is_empty() {
         let mut things = missing[..missing.len() - 1].join(", ");

diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs
@@ -85,8 +85,7 @@ fn metadata_warning() {
     p.cargo("package")
         .with_stderr(
             "\
-warning: manifest has no description, license, license-file, documentation, \
-homepage or repository.
+warning: manifest has no description, documentation, homepage or repository.
 See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
 [PACKAGING] foo v0.0.1 ([CWD])
 [VERIFYING] foo v0.0.1 ([CWD])
@@ -1943,8 +1942,7 @@ fn exclude_dot_files_and_directories_by_default() {
 
 #[cargo_test]
 fn empty_readme_path() {
-    // Warn but don't fail if `readme` is empty.
-    // Issue #11522.
+    // fail if `readme` is empty.
     let p = project()
         .file(
             "Cargo.toml",
@@ -1963,22 +1961,19 @@ fn empty_readme_path() {
         .build();
 
     p.cargo("package --no-verify")
+        .with_status(101)
         .with_stderr(
             "\
-[WARNING] readme `` does not appear to exist (relative to `[..]/foo`).
-Please update the readme setting in the manifest at `[..]/foo/Cargo.toml`
-This may become a hard error in the future.
-[PACKAGING] foo v1.0.0 ([..]/foo)
-[PACKAGED] [..] files, [..] ([..] compressed)
+[ERROR] readme `` does not appear to exist (relative to `[..]/foo`).
+Please update the readme setting in the manifest at `[..]/foo/Cargo.toml`.
 ",
         )
         .run();
 }
 
 #[cargo_test]
 fn invalid_readme_path() {
-    // Warn but don't fail if `readme` path is invalid.
-    // Issue #11522.
+    // fail if `readme` path is invalid.
     let p = project()
         .file(
             "Cargo.toml",
@@ -1997,22 +1992,19 @@ fn invalid_readme_path() {
         .build();
 
     p.cargo("package --no-verify")
+        .with_status(101)
         .with_stderr(
             "\
-[WARNING] readme `DOES-NOT-EXIST` does not appear to exist (relative to `[..]/foo`).
-Please update the readme setting in the manifest at `[..]/foo/Cargo.toml`
-This may become a hard error in the future.
-[PACKAGING] foo v1.0.0 ([..]/foo)
-[PACKAGED] [..] files, [..] ([..] compressed)
+[ERROR] readme `DOES-NOT-EXIST` does not appear to exist (relative to `[..]/foo`).
+Please update the readme setting in the manifest at `[..]/foo/Cargo.toml`.
 ",
         )
         .run();
 }
 
 #[cargo_test]
 fn readme_or_license_file_is_dir() {
-    // Test warning when `readme` or `license-file` is a directory, not a file.
-    // Issue #11522.
+    // Test error when `readme` or `license-file` is a directory, not a file.
     let p = project()
         .file(
             "Cargo.toml",
@@ -2031,25 +2023,21 @@ fn readme_or_license_file_is_dir() {
         .build();
 
     p.cargo("package --no-verify")
+        .with_status(101)
         .with_stderr(
             "\
-[WARNING] license-file `./src` does not appear to exist (relative to `[..]/foo`).
-Please update the license-file setting in the manifest at `[..]/foo/Cargo.toml`
-This may become a hard error in the future.
-[WARNING] readme `./src` does not appear to exist (relative to `[..]/foo`).
-Please update the readme setting in the manifest at `[..]/foo/Cargo.toml`
-This may become a hard error in the future.
-[PACKAGING] foo v1.0.0 ([..]/foo)
-[PACKAGED] [..] files, [..] ([..] compressed)
+[ERROR] license-file `./src` does not appear to exist (relative to `[..]/foo`).
+Please update the license-file setting in the manifest at `[..]/foo/Cargo.toml`.
+readme `./src` does not appear to exist (relative to `[..]/foo`).
+Please update the readme setting in the manifest at `[..]/foo/Cargo.toml`.
 ",
         )
         .run();
 }
 
 #[cargo_test]
 fn empty_license_file_path() {
-    // Warn but don't fail if license-file is empty.
-    // Issue #11522.
+    // fail if license-file is empty.
     let p = project()
         .file(
             "Cargo.toml",
@@ -2067,15 +2055,11 @@ fn empty_license_file_path() {
         .build();
 
     p.cargo("package --no-verify")
+        .with_status(101)
         .with_stderr(
             "\
-[WARNING] manifest has no license or license-file.
-See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
-[WARNING] license-file `` does not appear to exist (relative to `[..]/foo`).
-Please update the license-file setting in the manifest at `[..]/foo/Cargo.toml`
-This may become a hard error in the future.
-[PACKAGING] foo v1.0.0 ([..]/foo)
-[PACKAGED] [..] files, [..] ([..] compressed)
+[ERROR] license-file `` does not appear to exist (relative to `[..]/foo`).
+Please update the license-file setting in the manifest at `[..]/foo/Cargo.toml`.
 ",
         )
         .run();
@@ -2101,13 +2085,11 @@ fn invalid_license_file_path() {
         .build();
 
     p.cargo("package --no-verify")
+        .with_status(101)
         .with_stderr(
             "\
-[WARNING] license-file `does-not-exist` does not appear to exist (relative to `[..]/foo`).
-Please update the license-file setting in the manifest at `[..]/foo/Cargo.toml`
-This may become a hard error in the future.
-[PACKAGING] foo v1.0.0 ([..]/foo)
-[PACKAGED] [..] files, [..] ([..] compressed)
+[ERROR] license-file `does-not-exist` does not appear to exist (relative to `[..]/foo`).
+Please update the license-file setting in the manifest at `[..]/foo/Cargo.toml`.
 ",
         )
         .run();
@@ -3522,7 +3504,7 @@ fn versionless_package() {
     p.cargo("package")
         .with_stderr(
             "\
-warning: manifest has no license, license-file, documentation, homepage or repository.
+warning: manifest has no documentation, homepage or repository.
 See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
    Packaging foo v0.0.0 ([CWD])
    Verifying foo v0.0.0 ([CWD])
@@ -3728,7 +3710,7 @@ fn symlink_manifest_path() {
         .arg(foo_symlink.join("Cargo.toml"))
         .with_stderr(
             "\
-warning: manifest has no description, license, license-file, documentation, homepage or repository.
+warning: manifest has no description, documentation, homepage or repository.
 See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
 [PACKAGING] foo v1.0.0 ([..]foo-symlink)
 [PACKAGED] 6 files[..]

diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs
@@ -3,7 +3,7 @@
 use cargo_test_support::git::{self, repo};
 use cargo_test_support::paths;
 use cargo_test_support::registry::{self, Package, RegistryBuilder, Response};
-use cargo_test_support::{basic_manifest, no_such_file_err_msg, project, publish};
+use cargo_test_support::{basic_manifest, project, publish};
 use std::fs;
 use std::sync::{Arc, Mutex};
 
@@ -2137,54 +2137,6 @@ include `--registry dummy-registry` or `--registry crates-io`
         .run();
 }
 
-#[cargo_test]
-fn publish_with_missing_readme() {
-    // Use local registry for faster test times since no publish will occur
-    let registry = registry::init();
-
-    let p = project()
-        .file(
-            "Cargo.toml",
-            r#"
-                [package]
-                name = "foo"
-                version = "0.1.0"
-                edition = "2015"
-                authors = []
-                license = "MIT"
-                description = "foo"
-                homepage = "https://example.com/"
-                readme = "foo.md"
-            "#,
-        )
-        .file("src/lib.rs", "")
-        .build();
-
-    p.cargo("publish --no-verify")
-        .replace_crates_io(registry.index_url())
-        .with_status(101)
-        .with_stderr(&format!(
-            "\
-[UPDATING] [..]
-[WARNING] readme `foo.md` does not appear to exist (relative to `[..]/foo`).
-Please update the readme setting in the manifest at `[..]/foo/Cargo.toml`
-This may become a hard error in the future.
-[PACKAGING] foo v0.1.0 [..]
-[PACKAGED] [..] files, [..] ([..] compressed)
-[UPLOADING] foo v0.1.0 [..]
-[ERROR] failed to read `readme` file for package `foo v0.1.0 ([ROOT]/foo)`
-
-Caused by:
-  failed to read `[ROOT]/foo/foo.md`
-
-Caused by:
-  {}
-",
-            no_such_file_err_msg()
-        ))
-        .run();
-}
-
 // Registry returns an API error.
 #[cargo_test]
 fn api_error_json() {

diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs
@@ -1146,7 +1146,7 @@ fn bad_license_file(registry: &TestRegistry) {
     p.cargo("publish -v")
         .replace_crates_io(registry.index_url())
         .with_status(101)
-        .with_stderr_contains("[ERROR] the license file `foo` does not exist")
+        .with_stderr_contains("[ERROR] license-file `foo` does not appear to exist ([..]).")
         .run();
 }