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

rustup-dist: Use Download notifications to track install #1593

Merged
merged 2 commits into from
Jan 15, 2019

Conversation

kinnison
Copy link
Contributor

@kinnison kinnison commented Dec 27, 2018

People have requested some indication of progress for long-running
install steps. This commit re-uses the download tracker logic to
provide a progress bar (the length is the compressed component
tarball but should be good enough) to provide such feedback.

Fixes: #1557

@kinnison kinnison force-pushed the kinnison/unpack-progress branch from ef14a81 to 77cbe9f Compare December 27, 2018 10:00
@kinnison
Copy link
Contributor Author

All the rebase does is remove one of the fixes messages because it was over-enthusiastic.

@kinnison kinnison force-pushed the kinnison/unpack-progress branch 2 times, most recently from 33d6a8c to 771c120 Compare January 13, 2019 09:14
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I think this is probably a good idea. I would move the reader from the manifestation module to somewhere else though, maybe the dist::download module or a utils module somewhere

@kinnison
Copy link
Contributor Author

I will look at the right place to move it to, and attempt to get to it later when I'm home for the evening. I worry that the callback nesting might take me a while to get right though, so it might be a day or two before I can post depending on how awake I am when I get home. Thanks for the confirmation that the idea itself is okay though.

@kinnison kinnison force-pushed the kinnison/unpack-progress branch from 771c120 to 66336e5 Compare January 14, 2019 21:29
@kinnison
Copy link
Contributor Author

@nrc turns out that the FileReaderWithProgress moved nicely to rustup_utils but I'm not entirely clear on how I might add tests for this. Obviously I've run it by hand and am confident it does what I wanted, but do you feel it needs something under cargo test ?

@kinnison
Copy link
Contributor Author

Interestingly there's some formatting differences in the CI. I'll have to double-check my rustfmt install.

@kinnison kinnison force-pushed the kinnison/unpack-progress branch from 66336e5 to c8d7c8e Compare January 14, 2019 22:01
@kinnison
Copy link
Contributor Author

I've rebased and hopefully that'll clear up that formatting issue.

In order to be able to report unpack progress, add support for a file reader
which emits notifications akin to the downloading of a file.  This allows the
generic progress bar supporting downloads to also handle the installation of
components.

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
People have requested some indication of progress for long-running
install steps.  This commit uses the new FileReaderWithProgress to
provide a progress bar (the length is the compressed component
tarball but should be good enough) to provide such feedback.

Fixes: rust-lang#1557

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
@kinnison kinnison force-pushed the kinnison/unpack-progress branch from c8d7c8e to aaead82 Compare January 14, 2019 22:24
@nrc nrc merged commit a29076b into rust-lang:master Jan 15, 2019
@nrc
Copy link
Member

nrc commented Jan 15, 2019

Thanks!

@kinnison kinnison deleted the kinnison/unpack-progress branch October 20, 2019 09:09
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.

2 participants