From d649c6619181744ccf4384273323fb526a82c0ed Mon Sep 17 00:00:00 2001 From: mchernyavsky Date: Sat, 12 Sep 2020 20:15:47 +0300 Subject: [PATCH] Add a term option to configure the progress bar --- crates/cargo-test-support/src/lib.rs | 1 + src/cargo/ops/cargo_compile.rs | 2 + src/cargo/util/config/de.rs | 84 +++++---- src/cargo/util/config/mod.rs | 119 ++++++++++++- src/cargo/util/progress.rs | 25 ++- src/doc/src/reference/config.md | 19 +++ .../src/reference/environment-variables.md | 4 + tests/testsuite/main.rs | 1 + tests/testsuite/progress.rs | 159 ++++++++++++++++++ 9 files changed, 368 insertions(+), 46 deletions(-) create mode 100644 tests/testsuite/progress.rs diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 57da10dd54b..13d8f50c2de 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -1645,6 +1645,7 @@ fn substitute_macros(input: &str) -> String { ("[IGNORED]", " Ignored"), ("[INSTALLED]", " Installed"), ("[REPLACED]", " Replaced"), + ("[BUILDING]", " Building"), ]; let mut result = input.to_owned(); for &(pat, subst) in ¯os { diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index b8dbc211d72..7544d5dec73 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -289,6 +289,7 @@ pub fn create_bcx<'a, 'cfg>( } = *options; let config = ws.config(); + // Perform some pre-flight validation. match build_config.mode { CompileMode::Test | CompileMode::Build @@ -309,6 +310,7 @@ pub fn create_bcx<'a, 'cfg>( } } } + config.validate_term_config()?; let target_data = RustcTargetData::new(ws, &build_config.requested_kinds)?; diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index b822732eb24..758f5c23f13 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -40,13 +40,12 @@ macro_rules! deserialize_method { }; } -impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { - type Error = ConfigError; - - fn deserialize_any(self, visitor: V) -> Result - where - V: de::Visitor<'de>, - { +impl<'config> Deserializer<'config> { + /// This is a helper for getting a CV from a file or env var. + /// + /// If this returns CV::List, then don't look at the value. Handling lists + /// is deferred to ConfigSeqAccess. + fn get_cv_with_env(&self) -> Result, ConfigError> { // Determine if value comes from env, cli, or file, and merge env if // possible. let cv = self.config.get_cv(&self.key)?; @@ -58,36 +57,53 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { _ => false, }; - if use_env { - // Future note: If you ever need to deserialize a non-self describing - // map type, this should implement a starts_with check (similar to how - // ConfigMapAccess does). - let env = env.unwrap(); - let res: Result = if env == "true" || env == "false" { - visitor.visit_bool(env.parse().unwrap()) - } else if let Ok(env) = env.parse::() { - visitor.visit_i64(env) - } else if self.config.cli_unstable().advanced_env - && env.starts_with('[') - && env.ends_with(']') - { - visitor.visit_seq(ConfigSeqAccess::new(self.clone())?) - } else { - // Try to merge if possible. - match cv { - Some(CV::List(_cv_list, _cv_def)) => { - visitor.visit_seq(ConfigSeqAccess::new(self.clone())?) - } - _ => { - // Note: CV::Table merging is not implemented, as env - // vars do not support table values. - visitor.visit_str(env) - } + if !use_env { + return Ok(cv); + } + + // Future note: If you ever need to deserialize a non-self describing + // map type, this should implement a starts_with check (similar to how + // ConfigMapAccess does). + let env = env.unwrap(); + if env == "true" { + Ok(Some(CV::Boolean(true, env_def))) + } else if env == "false" { + Ok(Some(CV::Boolean(false, env_def))) + } else if let Ok(i) = env.parse::() { + Ok(Some(CV::Integer(i, env_def))) + } else if self.config.cli_unstable().advanced_env + && env.starts_with('[') + && env.ends_with(']') + { + // Parsing is deferred to ConfigSeqAccess. + Ok(Some(CV::List(Vec::new(), env_def))) + } else { + // Try to merge if possible. + match cv { + Some(CV::List(cv_list, _cv_def)) => { + // Merging is deferred to ConfigSeqAccess. + Ok(Some(CV::List(cv_list, env_def))) } - }; - return res.map_err(|e| e.with_key_context(&self.key, env_def)); + _ => { + // Note: CV::Table merging is not implemented, as env + // vars do not support table values. In the future, we + // could check for `{}`, and interpret it as TOML if + // that seems useful. + Ok(Some(CV::String(env.to_string(), env_def))) + } + } } + } +} +impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { + type Error = ConfigError; + + fn deserialize_any(self, visitor: V) -> Result + where + V: de::Visitor<'de>, + { + let cv = self.get_cv_with_env()?; if let Some(cv) = cv { let res: (Result, Definition) = match cv { CV::Integer(i, def) => (visitor.visit_i64(i), def), diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index b3d1d7b5e86..eda4daaded4 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -176,6 +176,7 @@ pub struct Config { build_config: LazyCell, target_cfgs: LazyCell>, doc_extern_map: LazyCell, + progress_config: ProgressConfig, } impl Config { @@ -247,6 +248,7 @@ impl Config { build_config: LazyCell::new(), target_cfgs: LazyCell::new(), doc_extern_map: LazyCell::new(), + progress_config: ProgressConfig::default(), } } @@ -459,8 +461,8 @@ impl Config { /// Get a configuration value by key. /// - /// This does NOT look at environment variables, the caller is responsible - /// for that. + /// This does NOT look at environment variables. See `get_cv_with_env` for + /// a variant that supports environment variables. fn get_cv(&self, key: &ConfigKey) -> CargoResult> { log::trace!("get cv {:?}", key); let vals = self.values()?; @@ -720,13 +722,9 @@ impl Config { let extra_verbose = verbose >= 2; let verbose = verbose != 0; - #[derive(Deserialize, Default)] - struct TermConfig { - verbose: Option, - color: Option, - } - - // Ignore errors in the configuration files. + // Ignore errors in the configuration files. We don't want basic + // commands like `cargo version` to error out due to config file + // problems. let term = self.get::("term").unwrap_or_default(); let color = color.or_else(|| term.color.as_deref()); @@ -754,6 +752,7 @@ impl Config { self.shell().set_verbosity(verbosity); self.shell().set_color_choice(color)?; + self.progress_config = term.progress.unwrap_or_default(); self.extra_verbose = extra_verbose; self.frozen = frozen; self.locked = locked; @@ -1192,6 +1191,20 @@ impl Config { .try_borrow_with(|| Ok(self.get::("build")?)) } + pub fn progress_config(&self) -> &ProgressConfig { + &self.progress_config + } + + /// This is used to validate the `term` table has valid syntax. + /// + /// This is necessary because loading the term settings happens very + /// early, and in some situations (like `cargo version`) we don't want to + /// fail if there are problems with the config file. + pub fn validate_term_config(&self) -> CargoResult<()> { + drop(self.get::("term")?); + Ok(()) + } + /// Returns a list of [target.'cfg()'] tables. /// /// The list is sorted by the table name. @@ -1778,6 +1791,94 @@ pub struct CargoBuildConfig { pub out_dir: Option, } +#[derive(Deserialize, Default)] +struct TermConfig { + verbose: Option, + color: Option, + #[serde(default)] + #[serde(deserialize_with = "progress_or_string")] + progress: Option, +} + +#[derive(Debug, Default, Deserialize)] +pub struct ProgressConfig { + pub when: ProgressWhen, + pub width: Option, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum ProgressWhen { + Auto, + Never, + Always, +} + +impl Default for ProgressWhen { + fn default() -> ProgressWhen { + ProgressWhen::Auto + } +} + +fn progress_or_string<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::de::Deserializer<'de>, +{ + struct ProgressVisitor; + + impl<'de> serde::de::Visitor<'de> for ProgressVisitor { + type Value = Option; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("a string (\"auto\" or \"never\") or a table") + } + + fn visit_str(self, s: &str) -> Result + where + E: serde::de::Error, + { + match s { + "auto" => Ok(Some(ProgressConfig { + when: ProgressWhen::Auto, + width: None, + })), + "never" => Ok(Some(ProgressConfig { + when: ProgressWhen::Never, + width: None, + })), + "always" => Err(E::custom("\"always\" progress requires a `width` key")), + _ => Err(E::unknown_variant(s, &["auto", "never"])), + } + } + + fn visit_none(self) -> Result + where + E: serde::de::Error, + { + Ok(None) + } + + fn visit_some(self, deserializer: D) -> Result + where + D: serde::de::Deserializer<'de>, + { + let pc = ProgressConfig::deserialize(deserializer)?; + if let ProgressConfig { + when: ProgressWhen::Always, + width: None, + } = pc + { + return Err(serde::de::Error::custom( + "\"always\" progress requires a `width` key", + )); + } + Ok(Some(pc)) + } + } + + deserializer.deserialize_option(ProgressVisitor) +} + /// A type to deserialize a list of strings from a toml file. /// /// Supports deserializing either a whitespace-separated list of arguments in a diff --git a/src/cargo/util/progress.rs b/src/cargo/util/progress.rs index 76536b8d16d..8bfc082788e 100644 --- a/src/cargo/util/progress.rs +++ b/src/cargo/util/progress.rs @@ -3,6 +3,7 @@ use std::env; use std::time::{Duration, Instant}; use crate::core::shell::Verbosity; +use crate::util::config::ProgressWhen; use crate::util::{is_ci, CargoResult, Config}; use unicode_width::UnicodeWidthChar; @@ -28,6 +29,7 @@ struct State<'cfg> { done: bool, throttle: Throttle, last_line: Option, + fixed_width: Option, } struct Format { @@ -45,12 +47,26 @@ impl<'cfg> Progress<'cfg> { Ok(term) => term == "dumb", Err(_) => false, }; + let progress_config = cfg.progress_config(); + match progress_config.when { + ProgressWhen::Always => return Progress::new_priv(name, style, cfg), + ProgressWhen::Never => return Progress { state: None }, + ProgressWhen::Auto => {} + } if cfg.shell().verbosity() == Verbosity::Quiet || dumb || is_ci() { return Progress { state: None }; } + Progress::new_priv(name, style, cfg) + } + + fn new_priv(name: &str, style: ProgressStyle, cfg: &'cfg Config) -> Progress<'cfg> { + let progress_config = cfg.progress_config(); + let width = progress_config + .width + .or_else(|| cfg.shell().err_width().progress_max_width()); Progress { - state: cfg.shell().err_width().progress_max_width().map(|n| State { + state: width.map(|n| State { config: cfg, format: Format { style, @@ -61,6 +77,7 @@ impl<'cfg> Progress<'cfg> { done: false, throttle: Throttle::new(), last_line: None, + fixed_width: progress_config.width, }), } } @@ -216,8 +233,10 @@ impl<'cfg> State<'cfg> { } fn try_update_max_width(&mut self) { - if let Some(n) = self.config.shell().err_width().progress_max_width() { - self.format.max_width = n; + if self.fixed_width.is_none() { + if let Some(n) = self.config.shell().err_width().progress_max_width() { + self.format.max_width = n; + } } } } diff --git a/src/doc/src/reference/config.md b/src/doc/src/reference/config.md index 6fafaecc962..1ff26208c34 100644 --- a/src/doc/src/reference/config.md +++ b/src/doc/src/reference/config.md @@ -147,6 +147,8 @@ metadata_key2 = "value" [term] verbose = false # whether cargo provides verbose output color = 'auto' # whether cargo colorizes output +progress.when = 'auto' # whether cargo shows progress bar +progress.width = 80 # width of progress bar ``` ### Environment variables @@ -903,6 +905,23 @@ Controls whether or not colored output is used in the terminal. Possible values: Can be overridden with the `--color` command-line option. +##### `term.progress.when` +* Type: string +* Default: "auto" +* Environment: `CARGO_TERM_PROGRESS_WHEN` + +Controls whether or not progress bar is shown in the terminal. Possible values: + +* `auto` (default): Intelligently guess whether to show progress bar. +* `always`: Always show progress bar. +* `never`: Never show progress bar. + +##### `term.progress.width` +* Type: integer +* Default: none +* Environment: `CARGO_TERM_PROGRESS_WIDTH` + +Sets the width for progress bar. [`cargo bench`]: ../commands/cargo-bench.md [`cargo login`]: ../commands/cargo-login.md diff --git a/src/doc/src/reference/environment-variables.md b/src/doc/src/reference/environment-variables.md index fc580e22fc4..a10d7c8bf20 100644 --- a/src/doc/src/reference/environment-variables.md +++ b/src/doc/src/reference/environment-variables.md @@ -103,6 +103,8 @@ supported environment variables are: * `CARGO_TARGET__RUSTFLAGS` — Extra `rustc` flags for a target, see [`target..rustflags`]. * `CARGO_TERM_VERBOSE` — The default terminal verbosity, see [`term.verbose`]. * `CARGO_TERM_COLOR` — The default color mode, see [`term.color`]. +* `CARGO_TERM_PROGRESS_WHEN` — The default progress bar showing mode, see [`term.progress.when`]. +* `CARGO_TERM_PROGRESS_WIDTH` — The default progress bar width, see [`term.progress.width`]. [`cargo doc`]: ../commands/cargo-doc.md [`cargo install`]: ../commands/cargo-install.md @@ -158,6 +160,8 @@ supported environment variables are: [`target..rustflags`]: config.md#targettriplerustflags [`term.verbose`]: config.md#termverbose [`term.color`]: config.md#termcolor +[`term.progress.when`]: config.md#termprogresswhen +[`term.progress.width`]: config.md#termprogresswidth ### Environment variables Cargo sets for crates diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 78379e8a01f..3ff15f404ba 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -88,6 +88,7 @@ mod profile_custom; mod profile_overrides; mod profile_targets; mod profiles; +mod progress; mod pub_priv; mod publish; mod publish_lockfile; diff --git a/tests/testsuite/progress.rs b/tests/testsuite/progress.rs new file mode 100644 index 00000000000..8e343144cda --- /dev/null +++ b/tests/testsuite/progress.rs @@ -0,0 +1,159 @@ +//! Tests for progress bar. + +use cargo_test_support::project; +use cargo_test_support::registry::Package; + +#[cargo_test] +fn bad_progress_config_unknown_when() { + let p = project() + .file( + ".cargo/config", + r#" + [term] + progress = { when = 'unknown' } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build") + .with_status(101) + .with_stderr( + "\ +[ERROR] error in [..].cargo/config: \ +could not load config key `term.progress.when` + +Caused by: + unknown variant `unknown`, expected one of `auto`, `never`, `always` +", + ) + .run(); +} + +#[cargo_test] +fn bad_progress_config_missing_width() { + let p = project() + .file( + ".cargo/config", + r#" + [term] + progress = { when = 'always' } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build") + .with_status(101) + .with_stderr( + "\ +[ERROR] \"always\" progress requires a `width` key +", + ) + .run(); +} + +#[cargo_test] +fn bad_progress_config_missing_when() { + let p = project() + .file( + ".cargo/config", + r#" + [term] + progress = { width = 1000 } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build") + .with_status(101) + .with_stderr( + "\ +error: missing field `when` +", + ) + .run(); +} + +#[cargo_test] +fn always_shows_progress() { + const N: usize = 3; + let mut deps = String::new(); + for i in 1..=N { + Package::new(&format!("dep{}", i), "1.0.0").publish(); + deps.push_str(&format!("dep{} = \"1.0\"\n", i)); + } + + let p = project() + .file( + ".cargo/config", + r#" + [term] + progress = { when = 'always', width = 100 } + "#, + ) + .file( + "Cargo.toml", + &format!( + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + {} + "#, + deps + ), + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build") + .with_stderr_contains("[DOWNLOADING] [..] crates [..]") + .with_stderr_contains("[..][DOWNLOADED] 3 crates ([..]) in [..]") + .with_stderr_contains("[BUILDING] [..] [..]/4: [..]") + .run(); +} + +#[cargo_test] +fn never_progress() { + const N: usize = 3; + let mut deps = String::new(); + for i in 1..=N { + Package::new(&format!("dep{}", i), "1.0.0").publish(); + deps.push_str(&format!("dep{} = \"1.0\"\n", i)); + } + + let p = project() + .file( + ".cargo/config", + r#" + [term] + progress = { when = 'never' } + "#, + ) + .file( + "Cargo.toml", + &format!( + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + {} + "#, + deps + ), + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build") + .with_stderr_does_not_contain("[DOWNLOADING] [..] crates [..]") + .with_stderr_does_not_contain("[..][DOWNLOADED] 3 crates ([..]) in [..]") + .with_stderr_does_not_contain("[BUILDING] [..] [..]/4: [..]") + .run(); +}