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.

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

Remove redundant message if an error occurs during package extraction #3464

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

ongchi
Copy link
Contributor

@ongchi ongchi commented Aug 23, 2023

This is the message when there is an error on extracting, it does contain the source of the cause. For example, when out of space on extraction:

error: failed to extract package (perhaps you ran out of disk space?): No space left on device (os error 28)

This PR remove the message in parentheses like this:

error: failed to extract package: No space left on device (os error 28)

Note: the error message already contains the source of cause by the selector "{:#}" in cli::common::report_error.
See also: https://docs.rs/anyhow/latest/anyhow/struct.Error.html#display-representations

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

So yes, for that specific ENOSPC error it would still make sense. My question is if there are other potential errors for unpack_without_first_dir() that might be indirectly caused by a lack of disk space, which could still benefit from the existing hint?

@djc
Copy link
Contributor

djc commented Aug 23, 2023

The hint was added in https://github.com/rust-lang/rustup/pull/2721/files#diff-47e56d6d43f3702f3f6de0b34ad1988bcccfdba0d493fdfa247c8fd544195327R153, which is sort of generic (conversion to anyhow), so it doesn't necessarily look like there was a specific reason to add this there?

@ongchi
Copy link
Contributor Author

ongchi commented Aug 24, 2023

Possible errors are return immediately, I think this should be obvious, except for these lines:

for mut item in io_executor.completed().collect::<Vec<_>>() {

for mut op in io_executor.completed().collect::<Vec<_>>() {

for mut item in io_executor.execute(item).collect::<Vec<_>>() {

for mut item in io_executor.join().collect::<Vec<_>>() {

The item or op above is an instance of CompletedIo.

rustup/src/diskio/mod.rs

Lines 187 to 210 in a4dd7d0

/// The details of the IO operation
#[derive(Debug)]
pub(crate) struct Item {
/// The path to operate on
pub(crate) full_path: PathBuf,
/// The operation to perform
pub(crate) kind: Kind,
/// When the operation started
start: Option<Instant>,
/// Amount of time the operation took to finish
finish: Option<Duration>,
/// The result of the operation (could now be factored into CompletedIO...)
pub(crate) result: io::Result<()>,
/// The mode to apply
mode: u32,
}
#[derive(Debug)]
pub(crate) enum CompletedIo {
/// A submitted Item has completed
Item(Item),
/// An IncrementalFile has completed a single chunk
Chunk(usize),
}

The io_executor may produce multiple results, but only the the first error would be returned. This is the only reason I can imagine why the error source returned is not an ENOSPC but might be the root cause.
However, I believe user still has to resolve any error from unpack_without_first_dir, as an ENOSPC message will eventually present if it is one of the reasons for the failure.

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

It does seem fair to conclude that this is not the right place to narrow the context to suggest ENOSPC if that is necessary at all. Given that was added in a commit that was arguably unrelated, I think it makes sense to remove it all, though maybe @rbtcollins remembers why he thought adding it here was useful.

@rami3l rami3l mentioned this pull request Oct 5, 2023
@djc djc merged commit 2780881 into rust-lang:master Oct 10, 2023
@ongchi ongchi deleted the remove-redundant-msg-on-pkg-extraction branch December 24, 2023 14:22
@rami3l rami3l added this to the 1.27.0 milestone Jan 18, 2024
@rami3l rami3l mentioned this pull request Jan 29, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants