Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(toml): Convert warnings that licence and readme files do not exist into errors #13921

Merged
merged 1 commit into from
Jun 9, 2024
Merged

fix(toml): Convert warnings that licence and readme files do not exist into errors #13921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions src/cargo/ops/cargo_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 {
Expand All @@ -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()
Expand Down Expand Up @@ -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!(
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
"{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(
Expand Down
57 changes: 21 additions & 36 deletions tests/testsuite/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1943,8 +1943,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",
Expand All @@ -1963,22 +1962,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",
Expand All @@ -1997,22 +1993,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",
Expand All @@ -2031,25 +2024,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",
Expand All @@ -2067,15 +2056,13 @@ 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();
Expand All @@ -2101,13 +2088,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();
Expand Down
50 changes: 1 addition & 49 deletions tests/testsuite/publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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() {
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down