Displays a one line progress of what crates are currently built. #5620
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
This is pretty awesome, thanks so much for implementing this @kennytm! I've long wanted to make Cargo's progress indicators more user-friendly (or have them at all!)
Would it be possible to perhaps have different modes on the progress bar? Downloads I think are best done with a percentage but the number of targets being built (sort of rustc invocations?) may be best done as a discrete number like "18/25" maybe?
src/cargo/core/compiler/job_queue.rs
Outdated
There was a problem hiding this comment.
I think that this may actually be easier to read as a Vec perhaps? That way names don't jump around but they should always be in the same place relative to other names. Should this also perhaps use a HashSet to deduplicate when we're building multiple targets for one crate?
kennytm
force-pushed
the
compile-progress
branch
from
22133eb to
a753b50
Compare
|
@alexcrichton Fixed! New asciicast: https://asciinema.org/a/YTiBAz4K4vfidNTAnehtyH46l |
|
Looks perfect to me! @matklad any thoughts on this? Or shall I r+? |
src/cargo/core/compiler/job_queue.rs
Outdated
| if self.active > 0 { | ||
|
|
||
| // self.active.remove_item(&key); // <- switch to this when stabilized. | ||
| if let Some(pos) = self.active.iter().position(|k| *k == key) { |
There was a problem hiding this comment.
This if let some looks suspicious to me. It should never fail, right? If that’s the case, let’s replace it with expect? If we later make a bug somewhere around here, expect would be much easier to debug.
| let pct = if !pct.is_finite() { 0.0 } else { pct }; | ||
| let stats = format!(" {:6.02}%", pct * 100.0); | ||
| let stats = match self.style { | ||
| ProgressStyle::Percentage => format!(" {:6.02}%", pct * 100.0), |
There was a problem hiding this comment.
Nit: let’s move pct calculation inside the match arm to minimize scope?
There was a problem hiding this comment.
@matklad The pct is later used to calculate the size of the progress bar, so no.
src/cargo/util/progress.rs
Outdated
| if avail_msg_len >= msg.len() + 3 { | ||
| string.push_str(&msg); | ||
| } else if avail_msg_len >= 4 { | ||
| string.push_str(&msg[..(avail_msg_len - 3)]); |
There was a problem hiding this comment.
Crate names can be non-ascii:
~/tmp
λ cargo new "привет-мир"
Created binary (application) `привет-мир` project
~/tmp
λ cd привет-мир
~/tmp/привет-мир master*
λ cargo run
Compiling привет-мир v0.1.0 (file:///home/matklad/tmp/%D0%BF%D1%80%D0%B8%D0%B2%D0%B5%D1%82-%D0%BC%D0%B8%D1%80)
Finished dev [unoptimized + debuginfo] target(s) in 0.99 secs
Running `target/debug/привет-мир`
Hello, world!
So I think this might fail at run time with utf8-boundary error :-) If we fix this, might be a good idea to write a unit-test for this formatting function as well.
There was a problem hiding this comment.
Ah thanks. Since this is a UI component we should limit by the display width, which counts by grapheme cluster and each cluster's width can be 1 or 2 spaces (half width or full width) long 💀. Since your Cargo.lock already depends on unicode-width via clap, I'll just add this dependency.
src/cargo/core/compiler/job_queue.rs
Outdated
| match self.rx.recv().unwrap() { | ||
| tokens.truncate(self.active.len() - 1); | ||
|
|
||
| let count = queue_len - self.queue.len(); |
There was a problem hiding this comment.
Micronit: queue.len() - queueue_len looks funny. Perhaps, rename it to total -self.que.len()?
|
@bors r+ Awesome tests! |
|
📌 Commit a8081a0 has been approved by |
Displays a one line progress of what crates are currently built. cc #2536, #3448. The change is based on #3451, but uses the progress bar introduced in #4646 instead. The percentage is simply the number of crates processed ÷ total crates count, which is inaccurate but better than nothing. Output looks like: [](https://asciinema.org/a/YTiBAz4K4vfidNTAnehtyH46l)
|
☀️ Test successful - status-appveyor, status-travis |
](https://asciinema.org/a/YTiBAz4K4vfidNTAnehtyH46l){kind=link}
6 participants
cc #2536, #3448.
The change is based on #3451, but uses the progress bar introduced in #4646 instead. The percentage is simply the number of crates processed ÷ total crates count, which is inaccurate but better than nothing.
Output looks like:
