Skip to content

Displays a one line progress of what crates are currently built. #5620

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.

Sign up for GitHub

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

Merged
bors merged 3 commits into from
Jun 29, 2018
Merged

Displays a one line progress of what crates are currently built. #5620

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3dbe701
Displays a one line progress of what crates are currently built.
kennytm Jun 8, 2018
a753b50
Addressed comments.
kennytm Jun 28, 2018
a8081a0
More fixes.
kennytm Jun 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions src/cargo/core/compiler/job_queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use core::{PackageId, Target};
use handle_error;
use util::{internal, profile, CargoResult, CargoResultExt, ProcessBuilder};
use util::{Config, DependencyQueue, Dirty, Fresh, Freshness};
use util::{Progress, ProgressStyle};

use super::job::Job;
use super::{BuildContext, BuildPlan, CompileMode, Context, Kind, Unit};
Expand All @@ -28,7 +29,7 @@ pub struct JobQueue<'a> {
queue: DependencyQueue<Key<'a>, Vec<(Job, Freshness)>>,
tx: Sender<Message<'a>>,
rx: Receiver<Message<'a>>,
active: usize,
active: Vec<Key<'a>>,
pending: HashMap<Key<'a>, PendingBuild>,
compiled: HashSet<&'a PackageId>,
documented: HashSet<&'a PackageId>,
Expand Down Expand Up @@ -98,7 +99,7 @@ impl<'a> JobQueue<'a> {
queue: DependencyQueue::new(),
tx,
rx,
active: 0,
active: Vec::new(),
pending: HashMap::new(),
compiled: HashSet::new(),
documented: HashSet::new(),
Expand Down Expand Up @@ -180,6 +181,8 @@ impl<'a> JobQueue<'a> {
// successful and otherwise wait for pending work to finish if it failed
// and then immediately return.
let mut error = None;
let mut progress = Progress::with_style("Building", ProgressStyle::Ratio, cx.bcx.config);
let queue_len = self.queue.len();
loop {
// Dequeue as much work as we can, learning about everything
// possible that can run. Note that this is also the point where we
Expand All @@ -196,7 +199,7 @@ impl<'a> JobQueue<'a> {
);
for (job, f) in jobs {
queue.push((key, job, f.combine(fresh)));
if self.active + queue.len() > 0 {
if !self.active.is_empty() || !queue.is_empty() {
jobserver_helper.request_token();
}
}
Expand All @@ -205,14 +208,14 @@ impl<'a> JobQueue<'a> {
// Now that we've learned of all possible work that we can execute
// try to spawn it so long as we've got a jobserver token which says
// we're able to perform some parallel work.
while error.is_none() && self.active < tokens.len() + 1 && !queue.is_empty() {
while error.is_none() && self.active.len() < tokens.len() + 1 && !queue.is_empty() {
let (key, job, fresh) = queue.remove(0);
self.run(key, fresh, job, cx.bcx.config, scope, build_plan)?;
}

// If after all that we're not actually running anything then we're
// done!
if self.active == 0 {
if self.active.is_empty() {
break;
}

Expand All @@ -221,9 +224,18 @@ impl<'a> JobQueue<'a> {
// jobserver interface is architected we may acquire a token that we
// don't actually use, and if this happens just relinquish it back
// to the jobserver itself.
tokens.truncate(self.active - 1);

match self.rx.recv().unwrap() {
tokens.truncate(self.active.len() - 1);

let count = queue_len - self.queue.len();
Copy link
Contributor

@matklad matklad Jun 29, 2018

Choose a reason for hiding this comment

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

Micronit: queue.len() - queueue_len looks funny. Perhaps, rename it to total -self.que.len()?

let active_names = self.active.iter().map(|key| match key.mode {
CompileMode::Doc { .. } => format!("{}(doc)", key.pkg.name()),
_ => key.pkg.name().to_string(),
}).collect::<Vec<_>>();
drop(progress.tick_now(count, queue_len, format!(": {}", active_names.join(", "))));
let event = self.rx.recv().unwrap();
progress.clear();

match event {
Message::Run(cmd) => {
cx.bcx
.config
Expand All @@ -245,8 +257,12 @@ impl<'a> JobQueue<'a> {
}
Message::Finish(key, result) => {
info!("end: {:?}", key);
self.active -= 1;
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) {
Copy link
Contributor

@matklad matklad Jun 29, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@kennytm kennytm Jun 29, 2018

Choose a reason for hiding this comment

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

Will do :)

self.active.remove(pos);
}
if !self.active.is_empty() {
assert!(!tokens.is_empty());
drop(tokens.pop());
}
Expand All @@ -256,7 +272,7 @@ impl<'a> JobQueue<'a> {
let msg = "The following warnings were emitted during compilation:";
self.emit_warnings(Some(msg), &key, cx)?;

if self.active > 0 {
if !self.active.is_empty() {
error = Some(format_err!("build failed"));
handle_error(e, &mut *cx.bcx.config.shell());
cx.bcx.config.shell().warn(
Expand All @@ -274,6 +290,7 @@ impl<'a> JobQueue<'a> {
}
}
}
drop(progress);

let build_type = if self.is_release { "release" } else { "dev" };
// NOTE: This may be a bit inaccurate, since this may not display the
Expand Down Expand Up @@ -334,7 +351,7 @@ impl<'a> JobQueue<'a> {
) -> CargoResult<()> {
info!("start: {:?}", key);

self.active += 1;
self.active.push(key);
*self.counts.get_mut(key.pkg).unwrap() -= 1;

let my_tx = self.tx.clone();
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub use self::to_semver::ToSemver;
pub use self::to_url::ToUrl;
pub use self::vcs::{FossilRepo, GitRepo, HgRepo, PijulRepo};
pub use self::read2::read2;
pub use self::progress::Progress;
pub use self::progress::{Progress, ProgressStyle};

pub mod config;
pub mod errors;
Expand Down
74 changes: 56 additions & 18 deletions src/cargo/util/progress.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::cmp;
use std::env;
use std::iter;
use std::time::{Duration, Instant};

use core::shell::Verbosity;
Expand All @@ -10,8 +9,15 @@ pub struct Progress<'cfg> {
state: Option<State<'cfg>>,
}

pub enum ProgressStyle {
Percentage,
Ratio,
}

struct State<'cfg> {
config: &'cfg Config,
style: ProgressStyle,
max_width: usize,
width: usize,
first: bool,
last_update: Instant,
Expand All @@ -20,7 +26,7 @@ struct State<'cfg> {
}

impl<'cfg> Progress<'cfg> {
pub fn new(name: &str, cfg: &'cfg Config) -> Progress<'cfg> {
pub fn with_style(name: &str, style: ProgressStyle, cfg: &'cfg Config) -> Progress<'cfg> {
// report no progress when -q (for quiet) or TERM=dumb are set
let dumb = match env::var("TERM") {
Ok(term) => term == "dumb",
Expand All @@ -33,6 +39,8 @@ impl<'cfg> Progress<'cfg> {
Progress {
state: cfg.shell().err_width().map(|n| State {
config: cfg,
style,
max_width: n,
width: cmp::min(n, 80),
first: true,
last_update: Instant::now(),
Expand All @@ -42,16 +50,33 @@ impl<'cfg> Progress<'cfg> {
}
}

pub fn new(name: &str, cfg: &'cfg Config) -> Progress<'cfg> {
Self::with_style(name, ProgressStyle::Percentage, cfg)
}

pub fn tick(&mut self, cur: usize, max: usize) -> CargoResult<()> {
match self.state {
Some(ref mut s) => s.tick(cur, max),
Some(ref mut s) => s.tick(cur, max, String::new(), true),
None => Ok(()),
}
}

pub fn clear(&mut self) {
if let Some(ref mut s) = self.state {
clear(s.max_width, s.config);
}
}

pub fn tick_now(&mut self, cur: usize, max: usize, msg: String) -> CargoResult<()> {
match self.state {
Some(ref mut s) => s.tick(cur, max, msg, false),
None => Ok(()),
}
}
}

impl<'cfg> State<'cfg> {
fn tick(&mut self, cur: usize, max: usize) -> CargoResult<()> {
fn tick(&mut self, cur: usize, max: usize, msg: String, throttle: bool) -> CargoResult<()> {
if self.done {
return Ok(());
}
Expand All @@ -68,25 +93,30 @@ impl<'cfg> State<'cfg> {
// 2. If we've drawn something, then we rate limit ourselves to only
// draw to the console every so often. Currently there's a 100ms
// delay between updates.
if self.first {
let delay = Duration::from_millis(500);
if self.last_update.elapsed() < delay {
return Ok(());
}
self.first = false;
} else {
let interval = Duration::from_millis(100);
if self.last_update.elapsed() < interval {
return Ok(());
if throttle {
if self.first {
let delay = Duration::from_millis(500);
if self.last_update.elapsed() < delay {
return Ok(());
}
self.first = false;
} else {
let interval = Duration::from_millis(100);
if self.last_update.elapsed() < interval {
return Ok(());
}
}
self.last_update = Instant::now();
}
self.last_update = Instant::now();

// Render the percentage at the far right and then figure how long the
// progress bar is
let pct = (cur as f64) / (max as f64);
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),
Copy link
Contributor

@matklad matklad Jun 29, 2018

Choose a reason for hiding this comment

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

Nit: let’s move pct calculation inside the match arm to minimize scope?

Copy link
Member Author

@kennytm kennytm Jun 29, 2018

Choose a reason for hiding this comment

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

@matklad The pct is later used to calculate the size of the progress bar, so no.

ProgressStyle::Ratio => format!(" {}/{}", cur, max),
};
let extra_len = stats.len() + 2 /* [ and ] */ + 15 /* status header */;
let display_width = match self.width.checked_sub(extra_len) {
Some(n) => n,
Expand Down Expand Up @@ -116,6 +146,14 @@ impl<'cfg> State<'cfg> {
string.push_str("]");
string.push_str(&stats);

let avail_msg_len = self.max_width - self.width;
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)]);
Copy link
Contributor

@matklad matklad Jun 29, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@kennytm kennytm Jun 29, 2018

Choose a reason for hiding this comment

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

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.

string.push_str("...");
}

// Write out a pretty header, then the progress bar itself, and then
// return back to the beginning of the line for the next print.
self.config.shell().status_header(&self.name)?;
Expand All @@ -125,12 +163,12 @@ impl<'cfg> State<'cfg> {
}

fn clear(width: usize, config: &Config) {
let blank = iter::repeat(" ").take(width).collect::<String>();
let blank = " ".repeat(width);
drop(write!(config.shell().err(), "{}\r", blank));
}

impl<'cfg> Drop for State<'cfg> {
fn drop(&mut self) {
clear(self.width, self.config);
clear(self.max_width, self.config);
}
}