Download cleanup #4531
Download cleanup #4531
Conversation
src/utils/mod.rs
Outdated
| pub(crate) struct FileReaderWithProgress<'a> { | ||
| pub(crate) struct FileReaderWithProgress { | ||
| fh: io::BufReader<File>, | ||
| tracker: &'a DownloadTracker, |
There was a problem hiding this comment.
Question: If I'm reading this right, you are completely removing the progress bar during installation? Why?
There was a problem hiding this comment.
Before this PR, calling handle() on a Notification would do nothing unless a Notification::DownloadingComponent value had been handled first, which would internally call create_progress_bar() and added an entry into the file_progress_bar HashMap. For Notifications with a None URL field, calling these methods actually did nothing. Thus, I removed all DownloadTracker method calls passing None as the url argument, which made the DownloadTracker unused in a bunch of places.
There was a problem hiding this comment.
Apologies for joining this conversation uninvited, but I would like to note that I have rebased my current PR on top of this one, and the progress bars are not appearing when downloading components -- the only notification shown is when the installation has already completed.
There was a problem hiding this comment.
No need to apologize, any feedback is very welcome!
What is your test scenario? Can you use git bisect to see which commit breaks it?
There was a problem hiding this comment.
I ran rustup toolchain install beta (also testing with different values for RUSTUP_CONCURRENT_DOWNLOADS) and consistently observed the same behavior.
I have already performed a bisect, which pointed to commit b42b6b2b, the second one of this PR.
After reviewing and testing this commit, I confirmed that the DownloadTracker methods are being called correctly.
However, the ProgressBars are hidden (as verified using the .is_hidden() method), which explains the lack of visible progress reporting.
I traced this issue to the creation of the DownloadTracker, where it appears that the display_progress boolean variable is inverted, causing the ProgressDrawTarget to be hidden.
There was a problem hiding this comment.
@FranciscoTGouveia great work! Fixed in my latest push.
Can't reiterate enough -- please always feel free to give feedback on any rustup PRs.
might be an unexpected regression introduced by previous changes,
Yeah, I think that's what happened -- anyway, I think these changes also make it easier to reintroduce progress bars for any other downloads we think are worth tracking.
There was a problem hiding this comment.
I have just fetched your branch and confirmed that the progress bars are still not appearing.
This time the issue was introduced in 2050503 (not related with cfg.quiet this time).
There was a problem hiding this comment.
There was a problem hiding this comment.
Great catch! Pushed another update, and going to do another careful self review as well.
There was a problem hiding this comment.
Confirmed that the progress bars show up for me now, although there's a lot of flickering involved.
|
Please note that since this PR is related to more subtle changes in the user behavior that is not fully covered by the test suite, more attention should be paid to ensure the necessary preservation of original semantics. |
3 participants
Followup from:
Sorry @FranciscoTGouveia, here are some more changes which IMO make things a bunch cleaner. I haven't done any testing yet (would be great if you want to do some of that) but I think the changes here are relatively straightforward?