fix(package): improve error reporting for unreadable directories#16517
fix(package): improve error reporting for unreadable directories#16517arin-r wants to merge 1 commit intorust-lang:masterfrom
Conversation
rustbot
added
Command-package
S-waiting-on-review
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
| // Unix-only test because Windows has different permission handling | ||
| #[cfg(unix)] | ||
| #[cargo_test] | ||
| fn unreadable_directory_error_message() { |
There was a problem hiding this comment.
We generally ask that PRs be split into at least two commits
- Adding tests that reproduce the existing behavior
- The bugfix with updates to the tests to show the new behavior
See also https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request
There was a problem hiding this comment.
Also
It introduces two improvements:
That is a good hint that that should be at least two separate commits, if not two PRs.
It would be inaccurate to describe this as a crash. That could apply to a non-assert panic but not an error like this. |
| // If this is a permission denied error, add a helpful hint | ||
| if e.kind() == std::io::ErrorKind::PermissionDenied { | ||
| anyhow::Error::from(e).context(format!( | ||
| "failed to open for archiving: `{}`\n\nNote: If this is a local artifact, add it to .gitignore or the 'exclude' list in Cargo.toml.", |
There was a problem hiding this comment.
| "failed to open for archiving: `{}`\n\nNote: If this is a local artifact, add it to .gitignore or the 'exclude' list in Cargo.toml.", | |
| "failed to open for archiving: `{}`\n\nnote: to exclude local artifacts, add it to `.gitignore` or `Cargo.toml`s 'package.exclude'", |
| // If this is a permission denied error, add a helpful hint | ||
| if e.kind() == std::io::ErrorKind::PermissionDenied { | ||
| anyhow::Error::from(e).context(format!( | ||
| "failed to open for archiving: `{}`\n\nNote: If this is a local artifact, add it to .gitignore or the 'exclude' list in Cargo.toml.", | ||
| disk_path.display() | ||
| )) | ||
| } else { | ||
| anyhow::Error::from(e).context(format!( | ||
| "failed to open for archiving: `{}`", | ||
| disk_path.display() |
There was a problem hiding this comment.
generally how we structure these cases is
let suggestion = if condition {
// ...
} else {
"".to_owned();
};
anyhow::Error::from(e).context(format!(
"failed to open for archiving: `{}`{}",
disk_path.display(),
suggestion,
))which avoids repeating the error message.
| if let Err(e) = File::open(path) { | ||
| // Emit a warning but continue processing | ||
| let _ = ws.gctx().shell().warn(format!( | ||
| "cannot access `{}`: {}. `cargo package` will fail.", |
There was a problem hiding this comment.
While I understand where you are coming from, I'm not thrilled with the way this message is being communicated atm and find it easier for now to remove it
| "cannot access `{}`: {}. `cargo package` will fail.", | |
| "cannot access `{}`: {}", |
There was a problem hiding this comment.
Do we feel that this is worth a richer annotate-snippets style report? I see that a warning without action item is somehow confusing.
There was a problem hiding this comment.
True, the hand written note on the error can be made an annotate snippets note here.
I'd recommend making making the path an Origin while at it.
This adds a warning to 'cargo package --list' when encountering unreadable directories and appends a hint to 'cargo package' permission errors suggesting exclusion via .gitignore.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
☔ The latest upstream changes (possibly #16688) made this pull request unmergeable. Please resolve the merge conflicts. |
5 participants
What does this PR try to resolve?
This PR addresses the issue where
cargo packagecrashes on unreadable directories in the project root (common in Docker workflows) andcargo package --listmisleadingly lists them as valid.As discussed with @epage in #16465, this change focuses on better error reporting rather than changing build behavior. It introduces two improvements:
prepare_archive(used bycargo package), suggesting the user add the file to.gitignoreorexclude.cargo package --listwhen it encounters an unreadable directory, ensuring the user is alerted to potential issues without breaking the command. This check mirrors the archiver's access logic to ensure consistency across platforms.Note: The wording of the error hint is a proposal and can be adjusted during review to match project standards.
How to test and review this PR?
You can verify these changes by creating a dummy project with an unreadable directory:
cargo new --lib test_proj cd test_proj mkdir unreadable_dir chmod 000 unreadable_dir1. Test
cargo package --listwarning:Run
cargo package --listand verify that it prints a warning to stderr aboutunreadable_dirinstead of silently listing it.2. Test
cargo packageerror hint:Run
cargo packageand verify that the build failure message now includes the new hint suggesting exclusion via.gitignoreor exclude.Fixes #16465