From a64beabdca9ea450df214ae8424dc254b6af3778 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 7 Feb 2020 17:09:32 -0800 Subject: [PATCH] WIP --- src/cargo/ops/cargo_compile.rs | 2 + src/cargo/util/config/de.rs | 120 ++++++++++++++++-------- src/cargo/util/config/mod.rs | 126 ++++++++++++++++++++++++-- src/cargo/util/progress.rs | 23 ++++- tests/testsuite/cargo_alias_config.rs | 5 +- tests/testsuite/main.rs | 1 + tests/testsuite/progress.rs | 54 +++++++++++ 7 files changed, 280 insertions(+), 51 deletions(-) create mode 100644 tests/testsuite/progress.rs diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 99b5c050177..fb144fab0f7 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -279,6 +279,7 @@ pub fn compile_ws<'a>( ref export_dir, } = *options; + // Perform some pre-flight validation. match build_config.mode { CompileMode::Test | CompileMode::Build @@ -299,6 +300,7 @@ pub fn compile_ws<'a>( } } } + config.validate_term_config()?; let profiles = Profiles::new( ws.profiles(), diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index 32e8ee6d93a..2410a75fd42 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -37,13 +37,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)?; @@ -54,36 +53,52 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { (None, Some(_)) => true, _ => 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) - } - } - }; - return res.map_err(|e| e.with_key_context(&self.key, env_def)); + 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" { + return Ok(Some(CV::Boolean(true, env_def))); + } else if env == "false" { + return Ok(Some(CV::Boolean(false, env_def))); + } else if let Ok(i) = env.parse::() { + return 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. + return 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. + return Ok(Some(CV::List(cv_list, 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. + return 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), @@ -176,11 +191,44 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { visitor.visit_seq(ConfigSeqAccess::new(self)?) } + fn deserialize_enum( + self, + _name: &'static str, + _variants: &'static [&'static str], + 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_enum((i as u32).into_deserializer()), def), + CV::String(s, def) => (visitor.visit_enum(s.into_deserializer()), def), + CV::List(..) | CV::Boolean(..) => { + return Err(ConfigError::expected( + &self.key, + "an enum-compatible type", + &cv, + )) + } + CV::Table(_, def) => ( + // TODO: Check how this path can be visited. + visitor.visit_map(ConfigMapAccess::new_map(self.clone())?), + def, + ), + }; + let (res, def) = res; + return res.map_err(|e| e.with_key_context(&self.key, def)); + } + Err(ConfigError::missing(&self.key)) + } + // These aren't really supported, yet. serde::forward_to_deserialize_any! { f32 f64 char str bytes byte_buf unit unit_struct - enum identifier ignored_any newtype_struct + identifier ignored_any newtype_struct } } diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index bc3d5434c04..97819c216a4 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -172,6 +172,7 @@ pub struct Config { net_config: LazyCell, build_config: LazyCell, target_cfgs: LazyCell>, + progress_config: ProgressConfig, } impl Config { @@ -241,6 +242,7 @@ impl Config { net_config: LazyCell::new(), build_config: LazyCell::new(), target_cfgs: LazyCell::new(), + progress_config: ProgressConfig::default(), } } @@ -442,8 +444,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()?; @@ -620,13 +622,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_ref().map(|s| s.as_ref())); @@ -654,6 +652,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; @@ -1054,6 +1053,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. @@ -1640,6 +1653,101 @@ 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_none(self) -> Result + where + E: serde::de::Error, + { + Ok(None) + } + + fn visit_some(self, deserializer: D) -> Result + where + D: serde::de::Deserializer<'de>, + { + ProgressConfig::deserialize(deserializer).map(Some) + } + + 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_map(self, map: M) -> Result + where + M: serde::de::MapAccess<'de>, + { + let pc = Deserialize::deserialize(serde::de::value::MapAccessDeserializer::new(map))?; + 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_any(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 d62600379cb..2df5ffc516e 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,24 @@ 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 { - state: cfg.shell().err_width().map(|n| State { + state: width.map(|n| State { config: cfg, format: Format { style, @@ -61,6 +75,7 @@ impl<'cfg> Progress<'cfg> { done: false, throttle: Throttle::new(), last_line: None, + fixed_width: progress_config.width, }), } } @@ -216,8 +231,10 @@ impl<'cfg> State<'cfg> { } fn try_update_max_width(&mut self) { - if let Some(n) = self.config.shell().err_width() { - self.format.max_width = n; + if self.fixed_width.is_none() { + if let Some(n) = self.config.shell().err_width() { + self.format.max_width = n; + } } } } diff --git a/tests/testsuite/cargo_alias_config.rs b/tests/testsuite/cargo_alias_config.rs index ffa8c73bf6b..fd1f29a065d 100644 --- a/tests/testsuite/cargo_alias_config.rs +++ b/tests/testsuite/cargo_alias_config.rs @@ -19,9 +19,8 @@ fn alias_incorrect_config_type() { p.cargo("b-cargo-test -v") .with_status(101) .with_stderr_contains( - "\ -[ERROR] invalid configuration for key `alias.b-cargo-test` -expected a list, but found a integer for [..]", + "[ERROR] error in [..]/foo/.cargo/config: \ + `alias.b-cargo-test` expected array, but found a integer", ) .run(); } diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index bbea3cc022e..96bf8023a54 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -81,6 +81,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..bd2075f55eb --- /dev/null +++ b/tests/testsuite/progress.rs @@ -0,0 +1,54 @@ +//! Tests for progress bar. + +use cargo_test_support::project; +use cargo_test_support::registry::Package; + +#[cargo_test] +fn bad_progress_config() { + // Some tests for bad configurations. +} + +#[cargo_test] +fn always_shows_progress() { + // Tests that when="always" shows progress. + // Add some dependencies just to give it something to display. + const N: usize = 10; + 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(); + + let output = p.cargo("check").exec_with_output().unwrap(); + assert!(output.status.success()); + let stderr = std::str::from_utf8(&output.stderr).unwrap(); + println!("{}", stderr); +} + +#[cargo_test] +fn never_progress() {}