Skip to content

Handle the progress reporting of installations through progress bars - #4552

Merged
djc merged 2 commits into
rust-lang:mainfrom
FranciscoTGouveia:progress-bar-installs
Oct 21, 2025
Merged

djc merged 2 commits into
rust-lang:mainfrom
FranciscoTGouveia:progress-bar-installs

Conversation

@FranciscoTGouveia

Copy link
Copy Markdown
Contributor

Cherry-picked from #4471, in response to #4471 (comment).

@rami3l
rami3l requested review from djc and rami3l October 20, 2025 13:09

@rami3l rami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@djc To clarify, this PR introduces two messages, pending installation for a toolchain waiting to be installed after the download phase, and a spinner to replace the original installing component... line.

@djc djc left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to have a bunch of smaller changes in a single commit. It would be nice to split them apart so that each is individually motivated. (IIRC it was two commits in #4471 in the first place.)

Comment thread src/dist/download.rs
@FranciscoTGouveia

FranciscoTGouveia commented Oct 21, 2025 •

Copy link
Copy Markdown
Contributor Author

This seems to have a bunch of smaller changes in a single commit. It would be nice to split them apart so that each is individually motivated. (IIRC it was two commits in #4471 in the first place.)

The two new progress bar states introduced (installing() and installed()) are closely related, so I don’t believe they can be separated into distinct commits.

Additionally, the original PR (4471) contained two commits because it introduced a special case for components that were already downloaded -- which I do not think is necessary.
When installing, the size of the component’s download is already available, and for components that are already installed, that value is simply 0 B.

A screenshot can be seen below to illustrate this:

image

However, I made some changes for the different possible states of the progress bars to be column-aligned and I think those would benefit being in a separate commit.

@djc
djc enabled auto-merge October 21, 2025 10:00
@djc

djc commented Oct 21, 2025

Copy link
Copy Markdown
Contributor

When installing, the size of the component’s download is already available, and for components that are already installed, that value is simply 0 B.

I think it might be better to retain the component's size or drop the size, since 0 B seems kinda wrong.

@djc
djc added this pull request to the merge queue Oct 21, 2025
Merged via the queue into rust-lang:main with commit c5839b9 Oct 21, 2025
29 checks passed
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