Skip to content

Warn instead of error in cargo package on empty readme or license-file in manifest #12036

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

Merged
merged 3 commits into from
Apr 25, 2023
Merged

Warn instead of error in cargo package on empty readme or license-file in manifest #12036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27e9599
failing tests on empty readme and license-path fields
kylematsuda Apr 25, 2023
560a3bf
check if readme and license-file point to files
kylematsuda Apr 25, 2023
90c7d3b
update the publish_with_missing_readme test with new warning
kylematsuda Apr 25, 2023
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
41 changes: 26 additions & 15 deletions src/cargo/ops/cargo_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ fn build_ar_list(
if let Some(license_file) = &pkg.manifest().metadata().license_file {
let license_path = Path::new(license_file);
let abs_file_path = paths::normalize_path(&pkg.root().join(license_path));
if abs_file_path.exists() {
if abs_file_path.is_file() {
check_for_file_and_add(
"license-file",
license_path,
Expand All @@ -291,26 +291,16 @@ fn build_ar_list(
ws,
)?;
} else {
let rel_msg = if license_path.is_absolute() {
"".to_string()
} else {
format!(" (relative to `{}`)", pkg.root().display())
};
ws.config().shell().warn(&format!(
"license-file `{}` does not appear to exist{}.\n\
Please update the license-file setting in the manifest at `{}`\n\
This may become a hard error in the future.",
license_path.display(),
rel_msg,
pkg.manifest_path().display()
))?;
warn_on_nonexistent_file(&pkg, &license_path, "license-file", &ws)?;
}
}
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() {
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)?;
}
}
result.sort_unstable_by(|a, b| a.rel_path.cmp(&b.rel_path));
Expand Down Expand Up @@ -369,6 +359,27 @@ fn check_for_file_and_add(
Ok(())
}

fn warn_on_nonexistent_file(
pkg: &Package,
path: &Path,
manifest_key_name: &'static str,
ws: &Workspace<'_>,
) -> CargoResult<()> {
let rel_msg = if path.is_absolute() {
"".to_string()
} else {
format!(" (relative to `{}`)", pkg.root().display())
};
ws.config().shell().warn(&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.",
path.display(),
rel_msg,
pkg.manifest_path().display()
))
}

/// Construct `Cargo.lock` for the package to be published.
fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult<String> {
let config = ws.config();
Expand Down
136 changes: 136 additions & 0 deletions tests/testsuite/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1774,6 +1774,142 @@ 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.
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.0.0"
readme = ""
license = "MIT"
description = "foo"
homepage = "foo"
"#,
)
.file("src/lib.rs", "")
.build();

p.cargo("package --no-verify")
.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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the wording here, I just copied the existing warning when license-file doesn't exist. Not sure if it's appropriate for readme or if it needs to be tweaked. If I understand correctly, one of license or license-file is required for crates.io, but it's okay for the readme to be missing, so not sure that the "hard error" caution is accurate here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a hard error caution is warranted because the user told us to do something and we couldn't do it.

[PACKAGING] foo v1.0.0 ([..]/foo)
[PACKAGED] [..] files, [..] ([..] compressed)
",
)
.run();
}

#[cargo_test]
fn invalid_readme_path() {
// Warn but don't fail if `readme` path is invalid.
// Issue #11522.
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.0.0"
readme = "DOES-NOT-EXIST"
license = "MIT"
description = "foo"
homepage = "foo"
"#,
)
.file("src/lib.rs", "")
.build();

p.cargo("package --no-verify")
.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)
",
)
.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.
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.0.0"
readme = "./src"
license-file = "./src"
description = "foo"
homepage = "foo"
"#,
)
.file("src/lib.rs", "")
.build();

p.cargo("package --no-verify")
.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)
",
)
.run();
}

#[cargo_test]
fn empty_license_file_path() {
// Warn but don't fail if license-file is empty.
// Issue #11522.
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.0.0"
license-file = ""
description = "foo"
homepage = "foo"
"#,
)
.file("src/lib.rs", "")
.build();

p.cargo("package --no-verify")
.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)
",
)
.run();
}

#[cargo_test]
fn invalid_license_file_path() {
// Test warning when license-file points to a non-existent file.
Expand Down
3 changes: 3 additions & 0 deletions tests/testsuite/publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1756,6 +1756,9 @@ fn publish_with_missing_readme() {
.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 [..]
Expand Down