-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(package): improve error reporting for unreadable directories #16517
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -299,6 +299,21 @@ fn do_package<'a>( | |||||
| let ar_files = prepare_archive(ws, &pkg, &opts)?; | ||||||
|
|
||||||
| if opts.list { | ||||||
| // Check file accessibility before listing to warn about potential issues | ||||||
| for ar_file in &ar_files { | ||||||
| if let FileContents::OnDisk(ref path) = ar_file.contents { | ||||||
| // Use the same File::open logic that the archiver uses to maintain sync | ||||||
| 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.", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we feel that this is worth a richer annotate-snippets style report? I see that a warning without action item is somehow confusing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, the hand written note on the error can be made an annotate snippets note here. I'd recommend making making the path an |
||||||
| path.display(), | ||||||
| e | ||||||
| )); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| match opts.fmt { | ||||||
| PackageMessageFormat::Human => { | ||||||
| // While this form is called "human", | ||||||
|
|
@@ -894,8 +909,19 @@ fn tar( | |||||
| let mut header = Header::new_gnu(); | ||||||
| match contents { | ||||||
| FileContents::OnDisk(disk_path) => { | ||||||
| let mut file = File::open(&disk_path).with_context(|| { | ||||||
| format!("failed to open for archiving: `{}`", disk_path.display()) | ||||||
| let mut file = File::open(&disk_path).map_err(|e| { | ||||||
| // 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.", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| disk_path.display() | ||||||
| )) | ||||||
| } else { | ||||||
| anyhow::Error::from(e).context(format!( | ||||||
| "failed to open for archiving: `{}`", | ||||||
| disk_path.display() | ||||||
|
Comment on lines
+913
to
+922
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| )) | ||||||
| } | ||||||
| })?; | ||||||
| let metadata = file.metadata().with_context(|| { | ||||||
| format!("could not learn metadata for: `{}`", disk_path.display()) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7895,3 +7895,81 @@ fn package_dir_not_excluded_from_backups() { | |
| "CACHEDIR.TAG should exist in target directory to exclude it from backups" | ||
| ); | ||
| } | ||
|
|
||
| // Unix-only test because Windows has different permission handling | ||
| #[cfg(unix)] | ||
| #[cargo_test] | ||
| fn unreadable_directory_error_message() { | ||
|
Comment on lines
+7899
to
+7902
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We generally ask that PRs be split into at least two commits
See also https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also
That is a good hint that that should be at least two separate commits, if not two PRs. |
||
| use std::os::unix::fs::PermissionsExt; | ||
|
|
||
| let p = project().file("src/lib.rs", "").build(); | ||
|
|
||
| // Create an unreadable directory | ||
| let unreadable_dir = p.root().join("unreadable-dir"); | ||
| fs::create_dir(&unreadable_dir).unwrap(); | ||
| fs::set_permissions(&unreadable_dir, fs::Permissions::from_mode(0o000)).unwrap(); | ||
|
|
||
| // Ensure cleanup happens even if test fails | ||
| struct Cleanup(std::path::PathBuf); | ||
| impl Drop for Cleanup { | ||
| fn drop(&mut self) { | ||
| let _ = fs::set_permissions(&self.0, fs::Permissions::from_mode(0o755)); | ||
| } | ||
| } | ||
| let _cleanup = Cleanup(unreadable_dir.clone()); | ||
|
|
||
| // cargo package should fail with Permission Denied error and helpful hint | ||
| p.cargo("package --no-verify") | ||
| .with_status(101) | ||
| .with_stderr_data(str![[r#" | ||
| [WARNING] manifest has no description, license, license-file, documentation, homepage or repository | ||
| | | ||
| = [NOTE] see https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info | ||
| [PACKAGING] foo v0.0.1 ([ROOT]/foo) | ||
| [ERROR] failed to prepare local package for uploading | ||
|
|
||
| Caused by: | ||
| failed to open for archiving: `[ROOT]/foo/unreadable-dir` | ||
|
|
||
| Note: If this is a local artifact, add it to .gitignore or the 'exclude' list in Cargo.toml. | ||
|
|
||
| Caused by: | ||
| Permission denied (os error 13) | ||
|
|
||
| "#]]) | ||
| .run(); | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| #[cargo_test] | ||
| fn unreadable_directory_list_warning() { | ||
| use std::os::unix::fs::PermissionsExt; | ||
|
|
||
| let p = project().file("src/lib.rs", "").build(); | ||
|
|
||
| // Create an unreadable directory | ||
| let unreadable_dir = p.root().join("unreadable-dir"); | ||
| fs::create_dir(&unreadable_dir).unwrap(); | ||
| fs::set_permissions(&unreadable_dir, fs::Permissions::from_mode(0o000)).unwrap(); | ||
|
|
||
| // Ensure cleanup happens even if test fails | ||
| struct Cleanup(std::path::PathBuf); | ||
| impl Drop for Cleanup { | ||
| fn drop(&mut self) { | ||
| let _ = fs::set_permissions(&self.0, fs::Permissions::from_mode(0o755)); | ||
| } | ||
| } | ||
| let _cleanup = Cleanup(unreadable_dir.clone()); | ||
|
|
||
| // cargo package --list should succeed but warn about unreadable directory | ||
| p.cargo("package --list") | ||
| .with_status(0) // Should still succeed | ||
| .with_stderr_data(str![[r#" | ||
| [WARNING] manifest has no description, license, license-file, documentation, homepage or repository | ||
| | | ||
| = [NOTE] see https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info | ||
| [WARNING] cannot access `[ROOT]/foo/unreadable-dir`: Permission denied (os error 13). `cargo package` will fail. | ||
|
|
||
| "#]]) | ||
| .run(); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.