Skip to content

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

Merged
nrc merged 2 commits intorust-lang:masterfrom
kinnison:kinnison/unpack-progress
Jan 15, 2019
Merged

rustup-dist: Use Download notifications to track install#1593
nrc merged 2 commits intorust-lang:masterfrom
kinnison:kinnison/unpack-progress

Conversation

@kinnison
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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