Skip to content

#3827 has broken the ETA format when downloading/installing components #3828

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

Closed
rami3l opened this issue May 15, 2024 · 9 comments · Fixed by #3829
Closed

#3827 has broken the ETA format when downloading/installing components #3828

rami3l opened this issue May 15, 2024 · 9 comments · Fixed by #3829

Comments

@rami3l
Copy link
Member

rami3l commented May 15, 2024

@djc Just tried the latest build and this looks incorrect.

image

Originally posted by @rami3l in #3827 (comment)

This regression has been discovered in the following version:

> rustup --version
rustup 1.27.1 :: 1.27.0+133 (eca50133c 2024-05-15)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.78.0 (9b00956e5 2024-04-29)`
@rami3l
Copy link
Member Author

rami3l commented May 15, 2024

Suggestion: we should stick to the old format:

format!(
"{} / {} ({:3.0} %) {} in {} ETA: {}",
total_h,
content_len_h,
percent,
speed_h,
elapsed_h.display(),
eta_h.display(),
)

... which means there should be an ETA: {} somewhere.

My suggestion write!(f, " ETA: {}", self.0.display()), in #3827 (comment) should do exactly this.

@djc
Copy link
Contributor

djc commented May 15, 2024

Why should we stick to the old format? I agree with #3826 that showing it after finishing is pointless.

@rami3l
Copy link
Member Author

rami3l commented May 15, 2024

Why should we stick to the old format? I agree with #3826 that showing it after finishing is pointless.

@djc Sorry, I meant stick to the old format when downloading. The expected result should be something like in 1s ETA: Unknown.

@rami3l
Copy link
Member Author

rami3l commented May 15, 2024

For example if ETA is 3s, the current version will print in 1s3s, which is clearly not ideal.

@djc
Copy link
Contributor

djc commented May 15, 2024

Ahh, no, I only applied half of your suggestion -- sorry, was too hasty; fix in #3829.

I was confused for a while by your screenshot showing "Unknown", but that seems unrelated.

@rami3l
Copy link
Member Author

rami3l commented May 15, 2024

Ahh, no, I only applied half of your suggestion -- sorry, was too hasty; fix in #3829.

I was confused for a while by your screenshot showing "Unknown", but that seems unrelated.

@djc Sorry, that Unknown is caused by a division-by-zero I believe. I probably won't be able to make a better screenshot: it all goes too fast on my machine :]

@djc
Copy link
Contributor

djc commented May 15, 2024

All of this code looks pretty shitty and would like to use indicatif instead (#1835), but that'll have to wait for another day.

@rami3l
Copy link
Member Author

rami3l commented May 15, 2024

All of this code looks pretty shitty and would like to use indicatif instead (#1835), but that'll have to wait for another day.

@djc I'm also in favor of using the console suite (incl. dialoguer and indicatif). In #3803 however I tried my best not to introduce new dependencies 😅

@djc
Copy link
Contributor

djc commented May 15, 2024

Yeah, one step at a time. async -> tracing -> indicatif.

@djc djc closed this as completed in #3829 May 15, 2024
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 a pull request may close this issue.

2 participants