From d64122454b3cbd84b412b442a45d6d04524ba8a5 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 14 May 2018 21:57:47 -0700 Subject: [PATCH 1/7] Typed Config Access This introduces a new API for accessing config values using serde to automatically convert to a destination type. By itself this shouldn't introduce any behavioral changes (except for some slight wording changes to error messages). However, it unlocks the ability to use richer data types in the future (such as `profile`, or `source`). Example: ```rust let p: Option = config.get("profile.dev")?; ``` Supports environment variables when fetching structs or maps. Note that it can support underscores in env var for struct field names, but not maps. So for example, "opt_level" works, but not "serde_json" (example: `CARGO_PROFILE_DEV_OVERRIDES_serde_OPT_LEVEL`). I don't have any ideas for a workaround (though I feel this is an overuse of env vars). It supports environment variables for lists. The value in the env var will get appended to anything in the config. It uses TOML syntax, and currently only supports strings. Example: `CARGO_FOO=['a', 'b']`. I did *not* modify `get_list` to avoid changing behavior, but that can easily be changed. --- src/cargo/core/compiler/build_config.rs | 21 +- src/cargo/lib.rs | 2 + src/cargo/util/config.rs | 794 +++++++++++++++++++++--- src/cargo/util/toml/mod.rs | 10 +- tests/testsuite/bad_config.rs | 13 +- tests/testsuite/config.rs | 578 ++++++++++++++++- 6 files changed, 1297 insertions(+), 121 deletions(-) diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index 7eb863f05e4..eb2a0b414a0 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -62,26 +62,7 @@ impl BuildConfig { its environment, ignoring the `-j` parameter", )?; } - let cfg_jobs = match config.get_i64("build.jobs")? { - Some(v) => { - if v.val <= 0 { - bail!( - "build.jobs must be positive, but found {} in {}", - v.val, - v.definition - ) - } else if v.val >= i64::from(u32::max_value()) { - bail!( - "build.jobs is too large: found {} in {}", - v.val, - v.definition - ) - } else { - Some(v.val as u32) - } - } - None => None, - }; + let cfg_jobs: Option = config.get("build.jobs")?; let jobs = jobs.or(cfg_jobs).unwrap_or(::num_cpus::get() as u32); Ok(BuildConfig { requested_target: target, diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 07b2cd408f1..b05cc6ec059 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -30,8 +30,10 @@ extern crate libgit2_sys; #[macro_use] extern crate log; extern crate num_cpus; +extern crate num_traits; extern crate same_file; extern crate semver; +#[macro_use] extern crate serde; #[macro_use] extern crate serde_derive; diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index f3a1d796810..a97a713459d 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -1,34 +1,38 @@ +use std; use std::cell::{RefCell, RefMut}; -use std::collections::HashSet; use std::collections::hash_map::Entry::{Occupied, Vacant}; use std::collections::hash_map::HashMap; +use std::collections::HashSet; use std::env; use std::fmt; use std::fs::{self, File}; -use std::io::SeekFrom; use std::io::prelude::*; +use std::io::SeekFrom; use std::mem; use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::{Once, ONCE_INIT}; use std::time::Instant; +use std::vec; use curl::easy::Easy; +use failure; use jobserver; -use serde::{Serialize, Serializer}; -use toml; use lazycell::LazyCell; +use num_traits; +use serde::{de, de::IntoDeserializer, Serialize, Serializer}; +use toml; use core::shell::Verbosity; use core::{CliUnstable, Shell, SourceId, Workspace}; use ops; use url::Url; -use util::ToUrl; -use util::Rustc; -use util::errors::{internal, CargoError, CargoResult, CargoResultExt}; +use util::errors::{internal, CargoResult, CargoResultExt}; use util::paths; use util::toml as cargo_toml; use util::Filesystem; +use util::Rustc; +use util::ToUrl; use self::ConfigValue as CV; @@ -38,7 +42,7 @@ use self::ConfigValue as CV; /// This struct implements `Default`: all fields can be inferred. #[derive(Debug)] pub struct Config { - /// The location of the users's 'home' directory. OS-dependent. + /// The location of the user's 'home' directory. OS-dependent. home_path: Filesystem, /// Information about how to write messages to the shell shell: RefCell, @@ -72,6 +76,8 @@ pub struct Config { creation_time: Instant, /// Target Directory via resolved Cli parameter target_dir: Option, + /// Environment variables, separated to assist testing. + env: HashMap, } impl Config { @@ -87,8 +93,18 @@ impl Config { } }); - let cache_rustc_info = match env::var("CARGO_CACHE_RUSTC_INFO") { - Ok(cache) => cache != "0", + let env: HashMap<_, _> = env::vars_os() + .filter_map(|(k, v)| { + // Ignore any key/values that are not valid Unicode. + match (k.into_string(), v.into_string()) { + (Ok(k), Ok(v)) => Some((k, v)), + _ => None, + } + }) + .collect(); + + let cache_rustc_info = match env.get("CARGO_CACHE_RUSTC_INFO".into()) { + Some(cache) => cache != "0", _ => true, }; @@ -116,6 +132,7 @@ impl Config { cache_rustc_info, creation_time: Instant::now(), target_dir: None, + env, } } @@ -179,7 +196,8 @@ impl Config { Rustc::new( self.get_tool("rustc")?, self.maybe_get_tool("rustc_wrapper")?, - &self.home() + &self + .home() .join("bin") .join("rustc") .into_path_unlocked() @@ -229,10 +247,12 @@ impl Config { .map(AsRef::as_ref) } + // TODO: Why is this `pub`? pub fn values(&self) -> CargoResult<&HashMap> { self.values.try_borrow_with(|| self.load_values()) } + // Note: This is used by RLS, not Cargo. pub fn set_values(&self, values: HashMap) -> CargoResult<()> { if self.values.borrow().is_some() { bail!("config values already found") @@ -260,7 +280,7 @@ impl Config { } } - fn get(&self, key: &str) -> CargoResult> { + fn get_cv(&self, key: &str) -> CargoResult> { let vals = self.values()?; let mut parts = key.split('.').enumerate(); let mut val = match vals.get(parts.next().unwrap().1) { @@ -294,49 +314,91 @@ impl Config { Ok(Some(val.clone())) } - fn get_env(&self, key: &str) -> CargoResult>> + // Helper primarily for testing. + pub fn set_env(&mut self, env: HashMap) { + self.env = env; + } + + fn get_env(&self, key: &ConfigKey) -> Result>, ConfigError> where - CargoError: From, + T: FromStr, + ::Err: fmt::Display, { - let key = key.replace(".", "_") - .replace("-", "_") - .chars() - .flat_map(|c| c.to_uppercase()) - .collect::(); - match env::var(&format!("CARGO_{}", key)) { - Ok(value) => Ok(Some(Value { - val: value.parse()?, - definition: Definition::Environment, - })), - Err(..) => Ok(None), + let key = key.to_env(); + match self.env.get(&key) { + Some(value) => { + let definition = Definition::Environment(key); + Ok(Some(Value { + val: value + .parse() + .map_err(|e| ConfigError::new(format!("{}", e), definition.clone()))?, + definition, + })) + } + None => Ok(None), } } - pub fn get_string(&self, key: &str) -> CargoResult>> { - if let Some(v) = self.get_env(key)? { - return Ok(Some(v)); + fn has_key(&self, key: &ConfigKey) -> bool { + let env_key = key.to_env(); + if self.env.get(&env_key).is_some() { + return true; } - match self.get(key)? { - Some(CV::String(i, path)) => Ok(Some(Value { - val: i, - definition: Definition::Path(path), - })), - Some(val) => self.expected("string", key, val), - None => Ok(None), + let env_pattern = format!("{}_", env_key); + if self.env.keys().any(|k| k.starts_with(&env_pattern)) { + return true; } + if let Ok(o_cv) = self.get_cv(&key.to_config()) { + if o_cv.is_some() { + return true; + } + } + false } - pub fn get_bool(&self, key: &str) -> CargoResult>> { - if let Some(v) = self.get_env(key)? { - return Ok(Some(v)); + pub fn get_string(&self, key: &str) -> CargoResult>> { + self.get_string_priv(&ConfigKey::from_str(key)) + .map_err(|e| e.into()) + } + + fn get_string_priv(&self, key: &ConfigKey) -> Result>, ConfigError> { + match self.get_env(key)? { + Some(v) => Ok(Some(v)), + None => { + let config_key = key.to_config(); + let o_cv = self.get_cv(&config_key)?; + match o_cv { + Some(CV::String(s, path)) => Ok(Some(Value { + val: s, + definition: Definition::Path(path), + })), + Some(cv) => Err(ConfigError::expected(&config_key, "a string", &cv)), + None => Ok(None), + } + } } - match self.get(key)? { - Some(CV::Boolean(b, path)) => Ok(Some(Value { - val: b, - definition: Definition::Path(path), - })), - Some(val) => self.expected("bool", key, val), - None => Ok(None), + } + + pub fn get_bool(&self, key: &str) -> CargoResult>> { + self.get_bool_priv(&ConfigKey::from_str(key)) + .map_err(|e| e.into()) + } + + fn get_bool_priv(&self, key: &ConfigKey) -> Result>, ConfigError> { + match self.get_env(key)? { + Some(v) => Ok(Some(v)), + None => { + let config_key = key.to_config(); + let o_cv = self.get_cv(&config_key)?; + match o_cv { + Some(CV::Boolean(b, path)) => Ok(Some(Value { + val: b, + definition: Definition::Path(path), + })), + Some(cv) => Err(ConfigError::expected(&config_key, "true/false", &cv)), + None => Ok(None), + } + } } } @@ -379,8 +441,10 @@ impl Config { Ok(None) } + // NOTE: This does *not* support environment variables. Use `get` instead + // if you want that. pub fn get_list(&self, key: &str) -> CargoResult>>> { - match self.get(key)? { + match self.get_cv(key)? { Some(CV::List(i, path)) => Ok(Some(Value { val: i, definition: Definition::Path(path), @@ -391,18 +455,14 @@ impl Config { } pub fn get_list_or_split_string(&self, key: &str) -> CargoResult>>> { - match self.get_env::(key) { - Ok(Some(value)) => { - return Ok(Some(Value { - val: value.val.split(' ').map(str::to_string).collect(), - definition: value.definition, - })) - } - Err(err) => return Err(err), - Ok(None) => (), + if let Some(value) = self.get_env::(&ConfigKey::from_str(key))? { + return Ok(Some(Value { + val: value.val.split(' ').map(str::to_string).collect(), + definition: value.definition, + })); } - match self.get(key)? { + match self.get_cv(key)? { Some(CV::List(i, path)) => Ok(Some(Value { val: i.into_iter().map(|(s, _)| s).collect(), definition: Definition::Path(path), @@ -417,7 +477,7 @@ impl Config { } pub fn get_table(&self, key: &str) -> CargoResult>>> { - match self.get(key)? { + match self.get_cv(key)? { Some(CV::Table(i, path)) => Ok(Some(Value { val: i, definition: Definition::Path(path), @@ -427,38 +487,67 @@ impl Config { } } + // Recommend use `get` if you want a specific type, such as an unsigned value. + // Example: config.get::>("some.key")? pub fn get_i64(&self, key: &str) -> CargoResult>> { - if let Some(v) = self.get_env(key)? { - return Ok(Some(v)); - } - match self.get(key)? { - Some(CV::Integer(i, path)) => Ok(Some(Value { - val: i, - definition: Definition::Path(path), - })), - Some(val) => self.expected("integer", key, val), - None => Ok(None), - } + self.get_integer(&ConfigKey::from_str(key)) + .map_err(|e| e.into()) } - pub fn net_retry(&self) -> CargoResult { - match self.get_i64("net.retry")? { - Some(v) => { - let value = v.val; - if value < 0 { - bail!( - "net.retry must be positive, but found {} in {}", - v.val, - v.definition - ) + fn get_integer(&self, key: &ConfigKey) -> Result>, ConfigError> + where + T: FromStr + num_traits::NumCast + num_traits::Bounded + num_traits::Zero + fmt::Display, + ::Err: fmt::Display, + { + let config_key = key.to_config(); + let v = match self.get_env::(key)? { + Some(v) => v, + None => match self.get_cv(&config_key)? { + Some(CV::Integer(i, path)) => Value { + val: i, + definition: Definition::Path(path), + }, + Some(cv) => return Err(ConfigError::expected(&config_key, "an integer", &cv)), + None => return Ok(None), + }, + }; + // Attempt to cast to the correct type, otherwise return a helpful + // error message. + match num_traits::cast(v.val) { + Some(casted_v) => Ok(Some(Value { + val: casted_v, + definition: v.definition, + })), + None => { + if T::min_value().is_zero() && v.val < 0 { + Err(ConfigError::new( + format!("`{}` must be positive, found {}", config_key, v.val), + v.definition, + )) } else { - Ok(value) + Err(ConfigError::new( + format!( + "`{}` is too large (min/max {}/{}), found {}", + config_key, + T::min_value(), + T::max_value(), + v.val + ), + v.definition, + )) } } + } + } + + pub fn net_retry(&self) -> CargoResult { + match self.get::>("net.retry")? { + Some(v) => Ok(v as i64), None => Ok(2), } } + // TODO: why is this pub? pub fn expected(&self, ty: &str, key: &str, val: CV) -> CargoResult { val.expected(ty, key) .map_err(|e| format_err!("invalid configuration for key `{}`\n{}", key, e)) @@ -541,6 +630,7 @@ impl Config { !self.frozen && !self.locked } + // TODO: this was pub for RLS but may not be needed anymore? /// Loads configuration from the filesystem pub fn load_values(&self) -> CargoResult> { let mut cfg = CV::Table(HashMap::new(), PathBuf::from(".")); @@ -645,7 +735,8 @@ impl Config { /// Look for a path for `tool` in an environment variable or config path, but return `None` /// if it's not present. fn maybe_get_tool(&self, tool: &str) -> CargoResult> { - let var = tool.chars() + let var = tool + .chars() .flat_map(|c| c.to_uppercase()) .collect::(); if let Some(tool_path) = env::var_os(&var) { @@ -681,7 +772,8 @@ impl Config { } pub fn http(&self) -> CargoResult<&RefCell> { - let http = self.easy + let http = self + .easy .try_borrow_with(|| ops::http_handle(self).map(RefCell::new))?; { let mut http = http.borrow_mut(); @@ -701,14 +793,538 @@ impl Config { pub fn creation_time(&self) -> Instant { self.creation_time } + + // Retrieve a config variable. + // + // This supports most serde `Deserialize` types. Examples: + // let v: Option = config.get("some.nested.key")?; + // let v: Option = config.get("some.key")?; + // let v: Option> = config.get("foo")?; + pub fn get<'de, T: de::Deserialize<'de>>(&self, key: &str) -> CargoResult { + let d = Deserializer { + config: self, + key: ConfigKey::from_str(key), + }; + T::deserialize(d).map_err(|e| e.into()) + } +} + +/// A segment of a config key. +/// +/// Config keys are split on dots for regular keys, or underscores for +/// environment keys. +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +enum ConfigKeyPart { + /// Case-insensitive part (checks uppercase in environment keys). + Part(String), + /// Case-sensitive part (environment keys must match exactly). + CasePart(String), } -#[derive(Eq, PartialEq, Clone, Copy)] -pub enum Location { - Project, - Global, +impl ConfigKeyPart { + fn to_env(&self) -> String { + match self { + ConfigKeyPart::Part(s) => s.replace("-", "_").to_uppercase(), + ConfigKeyPart::CasePart(s) => s.clone(), + } + } + + fn to_config(&self) -> String { + match self { + ConfigKeyPart::Part(s) => s.clone(), + ConfigKeyPart::CasePart(s) => s.clone(), + } + } +} + +/// Key for a configuration variable. +#[derive(Debug, Clone)] +struct ConfigKey(Vec); + +impl ConfigKey { + fn from_str(key: &str) -> ConfigKey { + ConfigKey( + key.split('.') + .map(|p| ConfigKeyPart::Part(p.to_string())) + .collect(), + ) + } + + fn join(&self, next: ConfigKeyPart) -> ConfigKey { + let mut res = self.clone(); + res.0.push(next); + res + } + + fn to_env(&self) -> String { + format!( + "CARGO_{}", + self.0 + .iter() + .map(|p| p.to_env()) + .collect::>() + .join("_") + ) + } + + fn to_config(&self) -> String { + self.0 + .iter() + .map(|p| p.to_config()) + .collect::>() + .join(".") + } + + fn last(&self) -> &ConfigKeyPart { + self.0.last().unwrap() + } +} + +impl fmt::Display for ConfigKey { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.to_config().fmt(f) + } +} + +/// Internal error for serde errors. +#[derive(Debug)] +pub struct ConfigError { + message: String, + definition: Option, +} + +impl ConfigError { + fn new(message: String, definition: Definition) -> ConfigError { + ConfigError { + message, + definition: Some(definition), + } + } + + fn expected(key: &str, expected: &str, found: &ConfigValue) -> ConfigError { + ConfigError { + message: format!( + "`{}` expected {}, but found a {}", + key, + expected, + found.desc() + ), + definition: Some(Definition::Path(found.definition_path().to_path_buf())), + } + } + + fn missing(key: String) -> ConfigError { + ConfigError { + message: format!("missing config key `{}`", key), + definition: None, + } + } + + fn with_key_context(self, key: String, definition: Definition) -> ConfigError { + ConfigError { + message: format!("could not load config key `{}`: {}", key, self), + definition: Some(definition), + } + } +} + +impl std::error::Error for ConfigError { + fn description(&self) -> &str { + self.message.as_str() + } +} + +impl fmt::Display for ConfigError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + if let Some(ref definition) = self.definition { + write!(f, "error in {}: {}", definition, self.message) + } else { + self.message.fmt(f) + } + } +} + +impl de::Error for ConfigError { + fn custom(msg: T) -> Self { + ConfigError { + message: msg.to_string(), + definition: None, + } + } +} + +// TODO: AFAIK, you cannot override Fail::cause (due to specialization), so we +// have no way to bubble up the underlying error. For now, it just formats +// the underlying cause as a string. +impl From for ConfigError { + fn from(e: failure::Error) -> Self { + let message = e + .causes() + .map(|e| e.to_string()) + .collect::>() + .join("\nCaused by:\n "); + ConfigError { + message, + definition: None, + } + } +} + +/// Serde deserializer used to convert config values to a target type using +/// `Config::get`. +pub struct Deserializer<'config> { + config: &'config Config, + key: ConfigKey, +} + +macro_rules! deserialize_method { + ($method:ident, $visit:ident, $getter:ident) => { + fn $method(self, visitor: V) -> Result + where + V: de::Visitor<'de>, + { + let v = self.config.$getter(&self.key)?.ok_or_else(|| + ConfigError::missing(self.key.to_config()))?; + let Value{val, definition} = v; + let res: Result = visitor.$visit(val); + res.map_err(|e| e.with_key_context(self.key.to_config(), definition)) + } + } +} + +impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { + type Error = ConfigError; + + fn deserialize_any(self, visitor: V) -> Result + where + V: de::Visitor<'de>, + { + // 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). + if let Some(v) = self.config.env.get(&self.key.to_env()) { + let res: Result = if v == "true" || v == "false" { + visitor.visit_bool(v.parse().unwrap()) + } else if let Ok(v) = v.parse::() { + visitor.visit_i64(v) + } else if v.starts_with("[") && v.ends_with("]") { + visitor.visit_seq(ConfigSeqAccess::new(self.config, self.key.clone())?) + } else { + visitor.visit_string(v.clone()) + }; + return res.map_err(|e| { + e.with_key_context( + self.key.to_config(), + Definition::Environment(self.key.to_env()), + ) + }); + } + + let o_cv = self.config.get_cv(&self.key.to_config())?; + if let Some(cv) = o_cv { + let res: (Result, PathBuf) = match cv { + CV::Integer(i, path) => (visitor.visit_i64(i), path), + CV::String(s, path) => (visitor.visit_string(s), path), + CV::List(_, path) => ( + visitor.visit_seq(ConfigSeqAccess::new(self.config, self.key.clone())?), + path, + ), + CV::Table(_, path) => ( + visitor.visit_map(ConfigMapAccess::new_map(self.config, self.key.clone())?), + path, + ), + CV::Boolean(b, path) => (visitor.visit_bool(b), path), + }; + let (res, path) = res; + return res + .map_err(|e| e.with_key_context(self.key.to_config(), Definition::Path(path))); + } + Err(ConfigError::missing(self.key.to_config())) + } + + deserialize_method!(deserialize_bool, visit_bool, get_bool_priv); + deserialize_method!(deserialize_i8, visit_i8, get_integer); + deserialize_method!(deserialize_i16, visit_i16, get_integer); + deserialize_method!(deserialize_i32, visit_i32, get_integer); + deserialize_method!(deserialize_i64, visit_i64, get_integer); + deserialize_method!(deserialize_u8, visit_u8, get_integer); + deserialize_method!(deserialize_u16, visit_u16, get_integer); + deserialize_method!(deserialize_u32, visit_u32, get_integer); + deserialize_method!(deserialize_u64, visit_u64, get_integer); + deserialize_method!(deserialize_string, visit_string, get_string_priv); + + fn deserialize_option(self, visitor: V) -> Result + where + V: de::Visitor<'de>, + { + if self.config.has_key(&self.key) { + visitor.visit_some(self) + } else { + // Treat missing values as None. + visitor.visit_none() + } + } + + fn deserialize_struct( + self, + _name: &'static str, + fields: &'static [&'static str], + visitor: V, + ) -> Result + where + V: de::Visitor<'de>, + { + visitor.visit_map(ConfigMapAccess::new_struct(self.config, self.key, fields)?) + } + + fn deserialize_map(self, visitor: V) -> Result + where + V: de::Visitor<'de>, + { + visitor.visit_map(ConfigMapAccess::new_map(self.config, self.key)?) + } + + fn deserialize_seq(self, visitor: V) -> Result + where + V: de::Visitor<'de>, + { + visitor.visit_seq(ConfigSeqAccess::new(self.config, self.key)?) + } + + fn deserialize_tuple(self, _len: usize, visitor: V) -> Result + where + V: de::Visitor<'de>, + { + visitor.visit_seq(ConfigSeqAccess::new(self.config, self.key)?) + } + + fn deserialize_tuple_struct( + self, + _name: &'static str, + _len: usize, + visitor: V, + ) -> Result + where + V: de::Visitor<'de>, + { + visitor.visit_seq(ConfigSeqAccess::new(self.config, self.key)?) + } + + fn deserialize_newtype_struct( + self, + name: &'static str, + visitor: V, + ) -> Result + where + V: de::Visitor<'de>, + { + if name == "ConfigRelativePath" { + match self.config.get_string_priv(&self.key)? { + Some(v) => { + let path = v + .definition + .root(self.config) + .join(v.val) + .display() + .to_string(); + visitor.visit_newtype_struct(path.into_deserializer()) + } + None => Err(ConfigError::missing(self.key.to_config())), + } + } else { + visitor.visit_newtype_struct(self) + } + } + + // These aren't really supported, yet. + forward_to_deserialize_any! { + f32 f64 char str bytes + byte_buf unit unit_struct + enum identifier ignored_any + } +} + +struct ConfigMapAccess<'config> { + config: &'config Config, + key: ConfigKey, + set_iter: as IntoIterator>::IntoIter, + next: Option, +} + +impl<'config> ConfigMapAccess<'config> { + fn new_map( + config: &'config Config, + key: ConfigKey, + ) -> Result, ConfigError> { + let mut set = HashSet::new(); + if let Some(mut v) = config.get_table(&key.to_config())? { + // v: Value> + for (key, _value) in v.val.drain() { + set.insert(ConfigKeyPart::CasePart(key)); + } + } + // CARGO_PROFILE_DEV_OVERRIDES_ + let env_pattern = format!("{}_", key.to_env()); + for env_key in config.env.keys() { + if env_key.starts_with(&env_pattern) { + // CARGO_PROFILE_DEV_OVERRIDES_bar_OPT_LEVEL = 3 + let rest = &env_key[env_pattern.len()..]; + // rest = bar_OPT_LEVEL + let part = rest.splitn(2, "_").next().unwrap(); + // part = "bar" + set.insert(ConfigKeyPart::CasePart(part.to_string())); + } + } + Ok(ConfigMapAccess { + config, + key, + set_iter: set.into_iter(), + next: None, + }) + } + + fn new_struct( + config: &'config Config, + key: ConfigKey, + fields: &'static [&'static str], + ) -> Result, ConfigError> { + let mut set = HashSet::new(); + for field in fields { + set.insert(ConfigKeyPart::Part(field.to_string())); + } + if let Some(mut v) = config.get_table(&key.to_config())? { + for (t_key, value) in v.val.drain() { + let part = ConfigKeyPart::Part(t_key); + if !set.contains(&part) { + config.shell().warn(format!( + "unused key `{}` in config file `{}`", + key.join(part).to_config(), + value.definition_path().display() + ))?; + } + } + } + Ok(ConfigMapAccess { + config, + key, + set_iter: set.into_iter(), + next: None, + }) + } +} + +impl<'de, 'config> de::MapAccess<'de> for ConfigMapAccess<'config> { + type Error = ConfigError; + + fn next_key_seed(&mut self, seed: K) -> Result, Self::Error> + where + K: de::DeserializeSeed<'de>, + { + match self.set_iter.next() { + Some(key) => { + let de_key = key.to_config(); + self.next = Some(key); + seed.deserialize(de_key.into_deserializer()).map(Some) + } + None => Ok(None), + } + } + + fn next_value_seed(&mut self, seed: V) -> Result + where + V: de::DeserializeSeed<'de>, + { + // TODO: Is it safe to assume next_value_seed is always called + // (exactly once) after next_key_seed? + let next_key = self.next.take().expect("next field missing"); + let next_key = self.key.join(next_key); + seed.deserialize(Deserializer { + config: self.config, + key: next_key, + }) + } +} + +struct ConfigSeqAccess { + list_iter: vec::IntoIter<(String, Definition)>, +} + +impl ConfigSeqAccess { + fn new(config: &Config, key: ConfigKey) -> Result { + let mut res = Vec::new(); + if let Some(v) = config.get_list(&key.to_config())? { + for (s, path) in v.val { + res.push((s, Definition::Path(path))); + } + } + + // Parse an environment string as a TOML array. + let env_key = key.to_env(); + let def = Definition::Environment(env_key.clone()); + if let Some(v) = config.env.get(&env_key) { + if !(v.starts_with("[") && v.ends_with("]")) { + return Err(ConfigError::new( + format!("should have TOML list syntax, found `{}`", v), + def.clone(), + )); + } + let temp_key = key.last().to_env(); + let toml_s = format!("{}={}", temp_key, v); + let toml_v: toml::Value = toml::de::from_str(&toml_s).map_err(|e| { + ConfigError::new(format!("could not parse TOML list: {}", e), def.clone()) + })?; + let values = toml_v + .as_table() + .unwrap() + .get(&temp_key) + .unwrap() + .as_array() + .expect("env var was not array"); + for value in values { + // TODO: support other types + let s = value.as_str().ok_or_else(|| { + ConfigError::new( + format!("expected string, found {}", value.type_str()), + def.clone(), + ) + })?; + res.push((s.to_string(), def.clone())); + } + } + Ok(ConfigSeqAccess { + list_iter: res.into_iter(), + }) + } +} + +impl<'de> de::SeqAccess<'de> for ConfigSeqAccess { + type Error = ConfigError; + + fn next_element_seed(&mut self, seed: T) -> Result, Self::Error> + where + T: de::DeserializeSeed<'de>, + { + match self.list_iter.next() { + // TODO: Add def to err? + Some((value, _def)) => seed.deserialize(value.into_deserializer()).map(Some), + None => Ok(None), + } + } +} + +/// Use with the `get` API to fetch a string that will be converted to a +/// `PathBuf`. Relative paths are converted to absolute paths based on the +/// location of the config file. +#[derive(Debug, Eq, PartialEq, Clone, Deserialize)] +pub struct ConfigRelativePath(PathBuf); + +impl ConfigRelativePath { + pub fn path(self) -> PathBuf { + self.0 + } } +// TODO: Why does this derive Deserialize? It is unused. #[derive(Eq, PartialEq, Clone, Deserialize)] pub enum ConfigValue { Integer(i64, PathBuf), @@ -723,9 +1339,10 @@ pub struct Value { pub definition: Definition, } +#[derive(Clone, Debug)] pub enum Definition { Path(PathBuf), - Environment, + Environment(String), } impl fmt::Debug for ConfigValue { @@ -749,6 +1366,7 @@ impl fmt::Debug for ConfigValue { } } +// TODO: Why is this here? It is unused. impl Serialize for ConfigValue { fn serialize(&self, s: S) -> Result { match *self { @@ -926,7 +1544,7 @@ impl Definition { pub fn root<'a>(&'a self, config: &'a Config) -> &'a Path { match *self { Definition::Path(ref p) => p.parent().unwrap().parent().unwrap(), - Definition::Environment => config.cwd(), + Definition::Environment(_) => config.cwd(), } } } @@ -935,7 +1553,7 @@ impl fmt::Display for Definition { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { Definition::Path(ref p) => p.display().fmt(f), - Definition::Environment => "the environment".fmt(f), + Definition::Environment(ref key) => write!(f, "environment variable `{}`", key), } } } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index fd664572ecf..a987eb31fa6 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -265,7 +265,7 @@ impl TomlProfiles { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Eq, PartialEq)] pub struct TomlOptLevel(pub String); impl<'de> de::Deserialize<'de> for TomlOptLevel { @@ -305,7 +305,7 @@ impl<'de> de::Deserialize<'de> for TomlOptLevel { } } - d.deserialize_u32(Visitor) + d.deserialize_any(Visitor) } } @@ -321,7 +321,7 @@ impl ser::Serialize for TomlOptLevel { } } -#[derive(Clone, Debug, Serialize)] +#[derive(Clone, Debug, Serialize, Eq, PartialEq)] #[serde(untagged)] pub enum U32OrBool { U32(u32), @@ -368,7 +368,7 @@ impl<'de> de::Deserialize<'de> for U32OrBool { } } -#[derive(Deserialize, Serialize, Clone, Debug, Default)] +#[derive(Deserialize, Serialize, Clone, Debug, Default, Eq, PartialEq)] #[serde(rename_all = "kebab-case")] pub struct TomlProfile { pub opt_level: Option, @@ -480,7 +480,7 @@ impl TomlProfile { } } -#[derive(Clone, Debug, Serialize)] +#[derive(Clone, Debug, Serialize, Eq, PartialEq)] #[serde(untagged)] pub enum StringOrBool { String(String), diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index 0986fda731f..45c3efd115d 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -109,8 +109,7 @@ fn bad3() { error: failed to update registry [..] Caused by: - invalid configuration for key `http.proxy` -expected a string, but found a boolean for `http.proxy` in [..]config + error in [..]config: `http.proxy` expected a string, but found a boolean ", ), ); @@ -134,8 +133,7 @@ fn bad4() { [ERROR] Failed to create project `foo` at `[..]` Caused by: - invalid configuration for key `cargo-new.name` -expected a string, but found a boolean for `cargo-new.name` in [..]config + error in [..]config: `cargo-new.name` expected a string, but found a boolean ", ), ); @@ -211,8 +209,7 @@ fn bad6() { error: failed to update registry [..] Caused by: - invalid configuration for key `http.user-agent` -expected a string, but found a boolean for `http.user-agent` in [..]config + error in [..]config: `http.user-agent` expected a string, but found a boolean ", ), ); @@ -243,7 +240,9 @@ fn bad_cargo_config_jobs() { p.cargo("build").arg("-v"), execs() .with_status(101) - .with_stderr("[ERROR] build.jobs must be positive, but found -1 in [..]"), + .with_stderr("\ +[ERROR] error in [..].cargo[/]config: `build.jobs` must be positive, found -1 +"), ); } diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index f76c278e984..88a6831b577 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1,5 +1,11 @@ -use cargotest::support::{execs, project}; +use cargo::core::Shell; +use cargo::util::config::{self, Config}; +use cargo::util::toml::{self, VecStringOrBool as VSOB}; +use cargo::CargoError; +use cargotest::support::{execs, lines_match, paths, project}; use hamcrest::assert_that; +use std::collections; +use std::fs; #[test] fn read_env_vars_for_config() { @@ -31,3 +37,573 @@ fn read_env_vars_for_config() { execs().with_status(0), ); } + +fn write_config(config: &str) { + let path = paths::root().join(".cargo/config"); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, config).unwrap(); +} + +fn new_config(env: &[(&str, &str)]) -> Config { + let shell = Shell::new(); + let cwd = paths::root(); + let homedir = paths::home(); + let env = env + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + let mut config = Config::new(shell, cwd, homedir); + config.set_env(env); + config +} + +fn assert_error(error: CargoError, msgs: &str) { + let causes = error + .causes() + .map(|e| e.to_string()) + .collect::>() + .join("\n"); + if !lines_match(msgs, &causes) { + panic!( + "Did not find expected:\n{}\nActual error:\n{}\n", + msgs, causes + ); + } +} + +#[test] +fn get_config() { + write_config( + "\ +[S] +f1 = 123 +", + ); + + let config = new_config(&[]); + + #[derive(Debug, Deserialize, Eq, PartialEq)] + struct S { + f1: Option, + } + let s: S = config.get("S").unwrap(); + assert_eq!(s, S { f1: Some(123) }); + let config = new_config(&[("CARGO_S_F1", "456")]); + let s: S = config.get("S").unwrap(); + assert_eq!(s, S { f1: Some(456) }); +} + +#[test] +fn config_unused_fields() { + write_config( + "\ +[S] +unused = 456 +", + ); + + let config = new_config(&[("CARGO_S_UNUSED2", "1"), ("CARGO_S2_UNUSED", "2")]); + + #[derive(Debug, Deserialize, Eq, PartialEq)] + struct S { + f1: Option, + } + // TODO: This currently does not verify the stderr output (not sure how). + // This prints the following: + // warning: unused key `S.unused` in config file `[..][/].cargo[/]config` + let s: S = config.get("S").unwrap(); + assert_eq!(s, S { f1: None }); + // This does not print anything, we cannot easily/reliably warn for + // environment variables. + let s: S = config.get("S2").unwrap(); + assert_eq!(s, S { f1: None }); +} + +#[test] +fn config_load_toml_profile() { + write_config( + "\ +[profile.dev] +opt-level = 's' +lto = true +codegen-units=4 +debug = true +debug-assertions = true +rpath = true +panic = 'abort' +overflow-checks = true +incremental = true + +[profile.dev.build-override] +opt-level = 1 + +[profile.dev.overrides.bar] +codegen-units = 9 +", + ); + + let config = new_config(&[ + ("CARGO_PROFILE_DEV_CODEGEN_UNITS", "5"), + ("CARGO_PROFILE_DEV_BUILD_OVERRIDE_CODEGEN_UNITS", "11"), + ("CARGO_PROFILE_DEV_OVERRIDES_env_CODEGEN_UNITS", "13"), + ("CARGO_PROFILE_DEV_OVERRIDES_bar_OPT_LEVEL", "2"), + ]); + + // TODO: don't use actual tomlprofile + let p: toml::TomlProfile = config.get("profile.dev").unwrap(); + let mut overrides = collections::BTreeMap::new(); + let key = toml::ProfilePackageSpec::Spec(::cargo::core::PackageIdSpec::parse("bar").unwrap()); + let o_profile = toml::TomlProfile { + opt_level: Some(toml::TomlOptLevel("2".to_string())), + lto: None, + codegen_units: Some(9), + debug: None, + debug_assertions: None, + rpath: None, + panic: None, + overflow_checks: None, + incremental: None, + overrides: None, + build_override: None, + }; + overrides.insert(key, o_profile); + let key = toml::ProfilePackageSpec::Spec(::cargo::core::PackageIdSpec::parse("env").unwrap()); + let o_profile = toml::TomlProfile { + opt_level: None, + lto: None, + codegen_units: Some(13), + debug: None, + debug_assertions: None, + rpath: None, + panic: None, + overflow_checks: None, + incremental: None, + overrides: None, + build_override: None, + }; + overrides.insert(key, o_profile); + + assert_eq!( + p, + toml::TomlProfile { + opt_level: Some(toml::TomlOptLevel("s".to_string())), + lto: Some(toml::StringOrBool::Bool(true)), + codegen_units: Some(5), + debug: Some(toml::U32OrBool::Bool(true)), + debug_assertions: Some(true), + rpath: Some(true), + panic: Some("abort".to_string()), + overflow_checks: Some(true), + incremental: Some(true), + overrides: Some(overrides), + build_override: Some(Box::new(toml::TomlProfile { + opt_level: Some(toml::TomlOptLevel("1".to_string())), + lto: None, + codegen_units: Some(11), + debug: None, + debug_assertions: None, + rpath: None, + panic: None, + overflow_checks: None, + incremental: None, + overrides: None, + build_override: None + })) + } + ); +} + +#[test] +fn config_deserialize_any() { + // Some tests to exercise deserialize_any for deserializers that need to + // be told the format. + write_config( + "\ +a = true +b = ['b'] +c = ['c'] +", + ); + + let config = new_config(&[ + ("CARGO_ENVB", "false"), + ("CARGO_C", "['d']"), + ("CARGO_ENVL", "['a', 'b']"), + ]); + + let a = config.get::("a").unwrap(); + match a { + VSOB::VecString(_) => panic!("expected bool"), + VSOB::Bool(b) => assert_eq!(b, true), + } + let b = config.get::("b").unwrap(); + match b { + VSOB::VecString(l) => assert_eq!(l, vec!["b".to_string()]), + VSOB::Bool(_) => panic!("expected list"), + } + let c = config.get::("c").unwrap(); + match c { + VSOB::VecString(l) => assert_eq!(l, vec!["c".to_string(), "d".to_string()]), + VSOB::Bool(_) => panic!("expected list"), + } + let envb = config.get::("envb").unwrap(); + match envb { + VSOB::VecString(_) => panic!("expected bool"), + VSOB::Bool(b) => assert_eq!(b, false), + } + let envl = config.get::("envl").unwrap(); + match envl { + VSOB::VecString(l) => assert_eq!(l, vec!["a".to_string(), "b".to_string()]), + VSOB::Bool(_) => panic!("expected list"), + } +} + +#[test] +fn config_toml_errors() { + write_config( + "\ +[profile.dev] +opt-level = 'foo' +", + ); + + let config = new_config(&[]); + + assert_error( + config.get::("profile.dev").unwrap_err(), + "error in [..][/].cargo[/]config: \ + could not load config key `profile.dev.opt-level`: \ + must be an integer, `z`, or `s`, but found: foo", + ); + + let config = new_config(&[("CARGO_PROFILE_DEV_OPT_LEVEL", "asdf")]); + + assert_error( + config.get::("profile.dev").unwrap_err(), + "error in environment variable `CARGO_PROFILE_DEV_OPT_LEVEL`: \ + could not load config key `profile.dev.opt-level`: \ + must be an integer, `z`, or `s`, but found: asdf", + ); +} + +#[test] +fn load_nested() { + write_config( + "\ +[nest.foo] +f1 = 1 +f2 = 2 +[nest.bar] +asdf = 3 +", + ); + + let config = new_config(&[ + ("CARGO_NEST_foo_f2", "3"), + ("CARGO_NESTE_foo_f1", "1"), + ("CARGO_NESTE_foo_f2", "3"), + ("CARGO_NESTE_bar_asdf", "3"), + ]); + + type Nested = collections::HashMap>; + + let n: Nested = config.get("nest").unwrap(); + let mut expected = collections::HashMap::new(); + let mut foo = collections::HashMap::new(); + foo.insert("f1".to_string(), 1); + foo.insert("f2".to_string(), 3); + expected.insert("foo".to_string(), foo); + let mut bar = collections::HashMap::new(); + bar.insert("asdf".to_string(), 3); + expected.insert("bar".to_string(), bar); + assert_eq!(n, expected); + + let n: Nested = config.get("neste").unwrap(); + assert_eq!(n, expected); +} + +#[test] +fn get_errors() { + write_config( + "\ +[S] +f1 = 123 +f2 = 'asdf' +big = 123456789 +", + ); + + let config = new_config(&[("CARGO_E_S", "asdf"), ("CARGO_E_BIG", "123456789")]); + assert_error( + config.get::("foo").unwrap_err(), + "missing config key `foo`", + ); + assert_error( + config.get::("foo.bar").unwrap_err(), + "missing config key `foo.bar`", + ); + assert_error( + config.get::("S.f2").unwrap_err(), + "error in [..][/].cargo[/]config: `S.f2` expected an integer, but found a string", + ); + assert_error( + config.get::("S.big").unwrap_err(), + "error in [..][/].cargo[/]config: `S.big` is too large (min/max 0/255), found 123456789", + ); + + // Environment variable type errors. + assert_error( + config.get::("e.s").unwrap_err(), + "error in environment variable `CARGO_E_S`: invalid digit found in string", + ); + assert_error( + config.get::("e.big").unwrap_err(), + "error in environment variable `CARGO_E_BIG`: \ + `e.big` is too large (min/max -128/127), found 123456789", + ); + + #[derive(Debug, Deserialize)] + struct S { + f1: i64, + f2: String, + f3: i64, + big: i64, + } + assert_error( + config.get::("S").unwrap_err(), + "missing config key `S.f3`", + ); +} + +#[test] +fn config_get_option() { + write_config( + "\ +[foo] +f1 = 1 +", + ); + + let config = new_config(&[("CARGO_BAR_ASDF", "3")]); + + assert_eq!(config.get::>("a").unwrap(), None); + assert_eq!(config.get::>("a.b").unwrap(), None); + assert_eq!(config.get::>("foo.f1").unwrap(), Some(1)); + assert_eq!(config.get::>("bar.asdf").unwrap(), Some(3)); + assert_eq!(config.get::>("bar.zzzz").unwrap(), None); +} + +#[test] +fn config_bad_toml() { + write_config("asdf"); + let config = new_config(&[]); + assert_error( + config.get::("foo").unwrap_err(), + "\ +could not load Cargo configuration +Caused by: + could not parse TOML configuration in `[..][/].cargo[/]config` +Caused by: + could not parse input as TOML +Caused by: + expected an equals, found eof at line 1", + ); +} + +#[test] +fn config_get_list() { + write_config( + "\ +l1 = [] +l2 = ['one', 'two'] +l3 = 123 +l4 = ['one', 'two'] + +[nested] +l = ['x'] + +[nested2] +l = ['y'] + +[nested-empty] +", + ); + + type L = Vec; + + let config = new_config(&[ + ("CARGO_L4", "['three', 'four']"), + ("CARGO_L5", "['a']"), + ("CARGO_ENV_EMPTY", "[]"), + ("CARGO_ENV_BLANK", ""), + ("CARGO_ENV_NUM", "1"), + ("CARGO_ENV_NUM_LIST", "[1]"), + ("CARGO_ENV_TEXT", "asdf"), + ("CARGO_LEPAIR", "['a', 'b']"), + ("CARGO_NESTED2_L", "['z']"), + ("CARGO_NESTEDE_L", "['env']"), + ("CARGO_BAD_ENV", "[zzz]"), + ]); + + assert_eq!(config.get::("unset").unwrap(), vec![] as Vec); + assert_eq!(config.get::("l1").unwrap(), vec![] as Vec); + assert_eq!(config.get::("l2").unwrap(), vec!["one", "two"]); + assert_error( + config.get::("l3").unwrap_err(), + "\ +invalid configuration for key `l3` +expected a list, but found a integer for `l3` in [..][/].cargo[/]config", + ); + assert_eq!( + config.get::("l4").unwrap(), + vec!["one", "two", "three", "four"] + ); + assert_eq!(config.get::("l5").unwrap(), vec!["a"]); + assert_eq!(config.get::("env-empty").unwrap(), vec![] as Vec); + assert_error( + config.get::("env-blank").unwrap_err(), + "error in environment variable `CARGO_ENV_BLANK`: \ + should have TOML list syntax, found ``", + ); + assert_error( + config.get::("env-num").unwrap_err(), + "error in environment variable `CARGO_ENV_NUM`: \ + should have TOML list syntax, found `1`", + ); + assert_error( + config.get::("env-num-list").unwrap_err(), + "error in environment variable `CARGO_ENV_NUM_LIST`: \ + expected string, found integer", + ); + assert_error( + config.get::("env-text").unwrap_err(), + "error in environment variable `CARGO_ENV_TEXT`: \ + should have TOML list syntax, found `asdf`", + ); + // "invalid number" here isn't the best error, but I think it's just toml.rs. + assert_error( + config.get::("bad-env").unwrap_err(), + "error in environment variable `CARGO_BAD_ENV`: \ + could not parse TOML list: invalid number at line 1", + ); + + // Try some other sequence-like types. + assert_eq!( + config + .get::<(String, String, String, String)>("l4") + .unwrap(), + ( + "one".to_string(), + "two".to_string(), + "three".to_string(), + "four".to_string() + ) + ); + assert_eq!(config.get::<(String,)>("l5").unwrap(), ("a".to_string(),)); + + // Tuple struct + #[derive(Debug, Deserialize, Eq, PartialEq)] + struct TupS(String, String); + assert_eq!( + config.get::("lepair").unwrap(), + TupS("a".to_string(), "b".to_string()) + ); + + // Nested with an option. + #[derive(Debug, Deserialize, Eq, PartialEq)] + struct S { + l: Option>, + } + assert_eq!(config.get::("nested-empty").unwrap(), S { l: None }); + assert_eq!( + config.get::("nested").unwrap(), + S { + l: Some(vec!["x".to_string()]), + } + ); + assert_eq!( + config.get::("nested2").unwrap(), + S { + l: Some(vec!["y".to_string(), "z".to_string()]), + } + ); + assert_eq!( + config.get::("nestede").unwrap(), + S { + l: Some(vec!["env".to_string()]), + } + ); +} + +#[test] +fn config_get_other_types() { + write_config( + "\ +ns = 123 +ns2 = 456 +", + ); + + let config = new_config(&[("CARGO_NSE", "987"), ("CARGO_NS2", "654")]); + + #[derive(Debug, Deserialize, Eq, PartialEq)] + struct NewS(i32); + assert_eq!(config.get::("ns").unwrap(), NewS(123)); + assert_eq!(config.get::("ns2").unwrap(), NewS(654)); + assert_eq!(config.get::("nse").unwrap(), NewS(987)); + assert_error( + config.get::("unset").unwrap_err(), + "missing config key `unset`", + ); +} + +#[test] +fn config_relative_path() { + write_config(&format!( + "\ +p1 = 'foo/bar' +p2 = '../abc' +p3 = 'b/c' +abs = '{}' +", + paths::home().display(), + )); + + let config = new_config(&[("CARGO_EPATH", "a/b"), ("CARGO_P3", "d/e")]); + + assert_eq!( + config + .get::("p1") + .unwrap() + .path(), + paths::root().join("foo/bar") + ); + assert_eq!( + config + .get::("p2") + .unwrap() + .path(), + paths::root().join("../abc") + ); + assert_eq!( + config + .get::("p3") + .unwrap() + .path(), + paths::root().join("d/e") + ); + assert_eq!( + config + .get::("abs") + .unwrap() + .path(), + paths::home() + ); + assert_eq!( + config + .get::("epath") + .unwrap() + .path(), + paths::root().join("a/b") + ); +} From 5b44581a698427650f55e673115c8d9f055d7462 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 21 May 2018 12:42:43 -0700 Subject: [PATCH 2/7] Verify stderr output for warnings in test. --- tests/testsuite/config.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 88a6831b577..d569cb3668a 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -45,7 +45,8 @@ fn write_config(config: &str) { } fn new_config(env: &[(&str, &str)]) -> Config { - let shell = Shell::new(); + let output = Box::new(fs::File::create(paths::root().join("shell.out")).unwrap()); + let shell = Shell::from_write(output); let cwd = paths::root(); let homedir = paths::home(); let env = env @@ -108,15 +109,27 @@ unused = 456 struct S { f1: Option, } - // TODO: This currently does not verify the stderr output (not sure how). - // This prints the following: - // warning: unused key `S.unused` in config file `[..][/].cargo[/]config` + // This prints a warning (verified below). let s: S = config.get("S").unwrap(); assert_eq!(s, S { f1: None }); // This does not print anything, we cannot easily/reliably warn for // environment variables. let s: S = config.get("S2").unwrap(); assert_eq!(s, S { f1: None }); + + // Verify the warnings. + drop(config); // Paranoid about flushing the file. + let path = paths::root().join("shell.out"); + let output = fs::read_to_string(path).unwrap(); + let expected = "\ +warning: unused key `S.unused` in config file `[..][/].cargo[/]config` +"; + if !lines_match(expected, &output) { + panic!( + "Did not find expected:\n{}\nActual error:\n{}\n", + expected, output + ); + } } #[test] From 7c0b5fae8a966903cbf542c1ffa37020bd3b6f64 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 22 May 2018 15:06:06 -0700 Subject: [PATCH 3/7] Drop num_traits and rely on serde to handle numeric limits. --- src/cargo/lib.rs | 1 - src/cargo/util/config.rs | 56 ++++++++--------------------------- tests/testsuite/bad_config.rs | 4 ++- tests/testsuite/config.rs | 53 +++++++++++++++++++++++++++++++-- 4 files changed, 66 insertions(+), 48 deletions(-) diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index b05cc6ec059..e51dfe87a05 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -30,7 +30,6 @@ extern crate libgit2_sys; #[macro_use] extern crate log; extern crate num_cpus; -extern crate num_traits; extern crate same_file; extern crate semver; #[macro_use] diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index a97a713459d..aadddc84815 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -19,7 +19,6 @@ use curl::easy::Easy; use failure; use jobserver; use lazycell::LazyCell; -use num_traits; use serde::{de, de::IntoDeserializer, Serialize, Serializer}; use toml; @@ -494,49 +493,18 @@ impl Config { .map_err(|e| e.into()) } - fn get_integer(&self, key: &ConfigKey) -> Result>, ConfigError> - where - T: FromStr + num_traits::NumCast + num_traits::Bounded + num_traits::Zero + fmt::Display, - ::Err: fmt::Display, - { + fn get_integer(&self, key: &ConfigKey) -> Result>, ConfigError> { let config_key = key.to_config(); - let v = match self.get_env::(key)? { - Some(v) => v, + match self.get_env::(key)? { + Some(v) => Ok(Some(v)), None => match self.get_cv(&config_key)? { - Some(CV::Integer(i, path)) => Value { + Some(CV::Integer(i, path)) => Ok(Some(Value { val: i, definition: Definition::Path(path), - }, + })), Some(cv) => return Err(ConfigError::expected(&config_key, "an integer", &cv)), None => return Ok(None), }, - }; - // Attempt to cast to the correct type, otherwise return a helpful - // error message. - match num_traits::cast(v.val) { - Some(casted_v) => Ok(Some(Value { - val: casted_v, - definition: v.definition, - })), - None => { - if T::min_value().is_zero() && v.val < 0 { - Err(ConfigError::new( - format!("`{}` must be positive, found {}", config_key, v.val), - v.definition, - )) - } else { - Err(ConfigError::new( - format!( - "`{}` is too large (min/max {}/{}), found {}", - config_key, - T::min_value(), - T::max_value(), - v.val - ), - v.definition, - )) - } - } } } @@ -1043,14 +1011,14 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { } deserialize_method!(deserialize_bool, visit_bool, get_bool_priv); - deserialize_method!(deserialize_i8, visit_i8, get_integer); - deserialize_method!(deserialize_i16, visit_i16, get_integer); - deserialize_method!(deserialize_i32, visit_i32, get_integer); + deserialize_method!(deserialize_i8, visit_i64, get_integer); + deserialize_method!(deserialize_i16, visit_i64, get_integer); + deserialize_method!(deserialize_i32, visit_i64, get_integer); deserialize_method!(deserialize_i64, visit_i64, get_integer); - deserialize_method!(deserialize_u8, visit_u8, get_integer); - deserialize_method!(deserialize_u16, visit_u16, get_integer); - deserialize_method!(deserialize_u32, visit_u32, get_integer); - deserialize_method!(deserialize_u64, visit_u64, get_integer); + deserialize_method!(deserialize_u8, visit_i64, get_integer); + deserialize_method!(deserialize_u16, visit_i64, get_integer); + deserialize_method!(deserialize_u32, visit_i64, get_integer); + deserialize_method!(deserialize_u64, visit_i64, get_integer); deserialize_method!(deserialize_string, visit_string, get_string_priv); fn deserialize_option(self, visitor: V) -> Result diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index 45c3efd115d..985447530c8 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -241,7 +241,9 @@ fn bad_cargo_config_jobs() { execs() .with_status(101) .with_stderr("\ -[ERROR] error in [..].cargo[/]config: `build.jobs` must be positive, found -1 +[ERROR] error in [..].cargo[/]config: \ +could not load config key `build.jobs`: \ +invalid value: integer `-1`, expected u32 "), ); } diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index d569cb3668a..4ee42d9e226 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -361,7 +361,8 @@ big = 123456789 ); assert_error( config.get::("S.big").unwrap_err(), - "error in [..][/].cargo[/]config: `S.big` is too large (min/max 0/255), found 123456789", + "error in [..].cargo[/]config: could not load config key `S.big`: \ + invalid value: integer `123456789`, expected u8", ); // Environment variable type errors. @@ -372,7 +373,8 @@ big = 123456789 assert_error( config.get::("e.big").unwrap_err(), "error in environment variable `CARGO_E_BIG`: \ - `e.big` is too large (min/max -128/127), found 123456789", + could not load config key `e.big`: \ + invalid value: integer `123456789`, expected i8", ); #[derive(Debug, Deserialize)] @@ -620,3 +622,50 @@ abs = '{}' paths::root().join("a/b") ); } + +#[test] +fn config_get_integers() { + write_config( + "\ +npos = 123456789 +nneg = -123456789 +i64max = 9223372036854775807 +", + ); + + let config = new_config(&[ + ("CARGO_EPOS", "123456789"), + ("CARGO_ENEG", "-1"), + ("CARGO_EI64MAX", "9223372036854775807"), + ]); + + assert_eq!(config.get::("i64max").unwrap(), 9223372036854775807); + assert_eq!(config.get::("i64max").unwrap(), 9223372036854775807); + assert_eq!(config.get::("ei64max").unwrap(), 9223372036854775807); + assert_eq!(config.get::("ei64max").unwrap(), 9223372036854775807); + + assert_error( + config.get::("nneg").unwrap_err(), + "error in [..].cargo[/]config: \ + could not load config key `nneg`: \ + invalid value: integer `-123456789`, expected u32", + ); + assert_error( + config.get::("eneg").unwrap_err(), + "error in environment variable `CARGO_ENEG`: \ + could not load config key `eneg`: \ + invalid value: integer `-1`, expected u32", + ); + assert_error( + config.get::("npos").unwrap_err(), + "error in [..].cargo[/]config: \ + could not load config key `npos`: \ + invalid value: integer `123456789`, expected i8", + ); + assert_error( + config.get::("epos").unwrap_err(), + "error in environment variable `CARGO_EPOS`: \ + could not load config key `epos`: \ + invalid value: integer `123456789`, expected i8", + ); +} From bef90f314bb52d139bb89b317803fa1a2904cd4e Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 22 May 2018 15:54:31 -0700 Subject: [PATCH 4/7] Simplify net.retry config. --- src/cargo/util/config.rs | 7 ------- src/cargo/util/network.rs | 2 +- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index aadddc84815..47625aebef5 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -508,13 +508,6 @@ impl Config { } } - pub fn net_retry(&self) -> CargoResult { - match self.get::>("net.retry")? { - Some(v) => Ok(v as i64), - None => Ok(2), - } - } - // TODO: why is this pub? pub fn expected(&self, ty: &str, key: &str, val: CV) -> CargoResult { val.expected(ty, key) diff --git a/src/cargo/util/network.rs b/src/cargo/util/network.rs index e789a929d98..55b18a4cd0d 100644 --- a/src/cargo/util/network.rs +++ b/src/cargo/util/network.rs @@ -48,7 +48,7 @@ pub fn with_retry(config: &Config, mut callback: F) -> CargoResult where F: FnMut() -> CargoResult, { - let mut remaining = config.net_retry()?; + let mut remaining = config.get::>("net.retry")?.unwrap_or(2); loop { match callback() { Ok(ret) => return Ok(ret), From 4d53519606cfc411a031f1c8c374315fd5ad2961 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 22 May 2018 16:55:53 -0700 Subject: [PATCH 5/7] Cleanup, address some comments. --- src/cargo/util/config.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 47625aebef5..9a6cfea5e2a 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -246,7 +246,6 @@ impl Config { .map(AsRef::as_ref) } - // TODO: Why is this `pub`? pub fn values(&self) -> CargoResult<&HashMap> { self.values.try_borrow_with(|| self.load_values()) } @@ -508,8 +507,7 @@ impl Config { } } - // TODO: why is this pub? - pub fn expected(&self, ty: &str, key: &str, val: CV) -> CargoResult { + fn expected(&self, ty: &str, key: &str, val: CV) -> CargoResult { val.expected(ty, key) .map_err(|e| format_err!("invalid configuration for key `{}`\n{}", key, e)) } @@ -591,7 +589,6 @@ impl Config { !self.frozen && !self.locked } - // TODO: this was pub for RLS but may not be needed anymore? /// Loads configuration from the filesystem pub fn load_values(&self) -> CargoResult> { let mut cfg = CV::Table(HashMap::new(), PathBuf::from(".")); @@ -1195,8 +1192,6 @@ impl<'de, 'config> de::MapAccess<'de> for ConfigMapAccess<'config> { where V: de::DeserializeSeed<'de>, { - // TODO: Is it safe to assume next_value_seed is always called - // (exactly once) after next_key_seed? let next_key = self.next.take().expect("next field missing"); let next_key = self.key.join(next_key); seed.deserialize(Deserializer { @@ -1490,7 +1485,7 @@ impl ConfigValue { } } - pub fn expected(&self, wanted: &str, key: &str) -> CargoResult { + fn expected(&self, wanted: &str, key: &str) -> CargoResult { bail!( "expected a {}, but found a {} for `{}` in {}", wanted, From 154c787c720881209b4c6c581a2b4ce188b88bb3 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 23 May 2018 09:30:54 -0700 Subject: [PATCH 6/7] Add `-Z advanced-env` feature flag. --- src/cargo/core/features.rs | 2 + src/cargo/util/config.rs | 87 ++++++++++++++++++++------------------ tests/testsuite/config.rs | 9 ++++ 3 files changed, 58 insertions(+), 40 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index b83e242ce76..29e644be390 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -308,6 +308,7 @@ pub struct CliUnstable { pub avoid_dev_deps: bool, pub minimal_versions: bool, pub package_features: bool, + pub advanced_env: bool, } impl CliUnstable { @@ -342,6 +343,7 @@ impl CliUnstable { "avoid-dev-deps" => self.avoid_dev_deps = true, "minimal-versions" => self.minimal_versions = true, "package-features" => self.package_features = true, + "advanced-env" => self.advanced_env = true, _ => bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 9a6cfea5e2a..d62ebc1594f 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -965,7 +965,10 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { visitor.visit_bool(v.parse().unwrap()) } else if let Ok(v) = v.parse::() { visitor.visit_i64(v) - } else if v.starts_with("[") && v.ends_with("]") { + } else if self.config.cli_unstable().advanced_env + && v.starts_with("[") + && v.ends_with("]") + { visitor.visit_seq(ConfigSeqAccess::new(self.config, self.key.clone())?) } else { visitor.visit_string(v.clone()) @@ -1121,16 +1124,18 @@ impl<'config> ConfigMapAccess<'config> { set.insert(ConfigKeyPart::CasePart(key)); } } - // CARGO_PROFILE_DEV_OVERRIDES_ - let env_pattern = format!("{}_", key.to_env()); - for env_key in config.env.keys() { - if env_key.starts_with(&env_pattern) { - // CARGO_PROFILE_DEV_OVERRIDES_bar_OPT_LEVEL = 3 - let rest = &env_key[env_pattern.len()..]; - // rest = bar_OPT_LEVEL - let part = rest.splitn(2, "_").next().unwrap(); - // part = "bar" - set.insert(ConfigKeyPart::CasePart(part.to_string())); + if config.cli_unstable().advanced_env { + // CARGO_PROFILE_DEV_OVERRIDES_ + let env_pattern = format!("{}_", key.to_env()); + for env_key in config.env.keys() { + if env_key.starts_with(&env_pattern) { + // CARGO_PROFILE_DEV_OVERRIDES_bar_OPT_LEVEL = 3 + let rest = &env_key[env_pattern.len()..]; + // rest = bar_OPT_LEVEL + let part = rest.splitn(2, "_").next().unwrap(); + // part = "bar" + set.insert(ConfigKeyPart::CasePart(part.to_string())); + } } } Ok(ConfigMapAccess { @@ -1214,37 +1219,39 @@ impl ConfigSeqAccess { } } - // Parse an environment string as a TOML array. - let env_key = key.to_env(); - let def = Definition::Environment(env_key.clone()); - if let Some(v) = config.env.get(&env_key) { - if !(v.starts_with("[") && v.ends_with("]")) { - return Err(ConfigError::new( - format!("should have TOML list syntax, found `{}`", v), - def.clone(), - )); - } - let temp_key = key.last().to_env(); - let toml_s = format!("{}={}", temp_key, v); - let toml_v: toml::Value = toml::de::from_str(&toml_s).map_err(|e| { - ConfigError::new(format!("could not parse TOML list: {}", e), def.clone()) - })?; - let values = toml_v - .as_table() - .unwrap() - .get(&temp_key) - .unwrap() - .as_array() - .expect("env var was not array"); - for value in values { - // TODO: support other types - let s = value.as_str().ok_or_else(|| { - ConfigError::new( - format!("expected string, found {}", value.type_str()), + if config.cli_unstable().advanced_env { + // Parse an environment string as a TOML array. + let env_key = key.to_env(); + let def = Definition::Environment(env_key.clone()); + if let Some(v) = config.env.get(&env_key) { + if !(v.starts_with("[") && v.ends_with("]")) { + return Err(ConfigError::new( + format!("should have TOML list syntax, found `{}`", v), def.clone(), - ) + )); + } + let temp_key = key.last().to_env(); + let toml_s = format!("{}={}", temp_key, v); + let toml_v: toml::Value = toml::de::from_str(&toml_s).map_err(|e| { + ConfigError::new(format!("could not parse TOML list: {}", e), def.clone()) })?; - res.push((s.to_string(), def.clone())); + let values = toml_v + .as_table() + .unwrap() + .get(&temp_key) + .unwrap() + .as_array() + .expect("env var was not array"); + for value in values { + // TODO: support other types + let s = value.as_str().ok_or_else(|| { + ConfigError::new( + format!("expected string, found {}", value.type_str()), + def.clone(), + ) + })?; + res.push((s.to_string(), def.clone())); + } } } Ok(ConfigSeqAccess { diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 4ee42d9e226..f9d33e8c4cb 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -55,6 +55,15 @@ fn new_config(env: &[(&str, &str)]) -> Config { .collect(); let mut config = Config::new(shell, cwd, homedir); config.set_env(env); + config.configure( + 0, + None, + &None, + false, + false, + &None, + &["advanced-env".into()], + ).unwrap(); config } From b3db164b9294b7a085553dd70ceeedcebfcd733d Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 24 May 2018 15:14:13 -0700 Subject: [PATCH 7/7] Use `failure::Error` for ConfigError instead of a String. --- src/cargo/util/config.rs | 41 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index d62ebc1594f..a9ff2eeae9d 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -847,21 +847,21 @@ impl fmt::Display for ConfigKey { /// Internal error for serde errors. #[derive(Debug)] pub struct ConfigError { - message: String, + error: failure::Error, definition: Option, } impl ConfigError { fn new(message: String, definition: Definition) -> ConfigError { ConfigError { - message, + error: failure::err_msg(message), definition: Some(definition), } } fn expected(key: &str, expected: &str, found: &ConfigValue) -> ConfigError { ConfigError { - message: format!( + error: format_err!( "`{}` expected {}, but found a {}", key, expected, @@ -873,31 +873,42 @@ impl ConfigError { fn missing(key: String) -> ConfigError { ConfigError { - message: format!("missing config key `{}`", key), + error: format_err!("missing config key `{}`", key), definition: None, } } fn with_key_context(self, key: String, definition: Definition) -> ConfigError { ConfigError { - message: format!("could not load config key `{}`: {}", key, self), + error: format_err!("could not load config key `{}`: {}", key, self), definition: Some(definition), } } } impl std::error::Error for ConfigError { + // This can be removed once 1.27 is stable. fn description(&self) -> &str { - self.message.as_str() + "An error has occurred." } } +// Future Note: Currently we cannot override Fail::cause (due to +// specialization) so we have no way to return the underlying causes. In the +// future, once this limitation is lifted, this should instead implement +// `cause` and avoid doing the cause formatting here. impl fmt::Display for ConfigError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let message = self + .error + .causes() + .map(|e| e.to_string()) + .collect::>() + .join("\nCaused by:\n "); if let Some(ref definition) = self.definition { - write!(f, "error in {}: {}", definition, self.message) + write!(f, "error in {}: {}", definition, message) } else { - self.message.fmt(f) + message.fmt(f) } } } @@ -905,24 +916,16 @@ impl fmt::Display for ConfigError { impl de::Error for ConfigError { fn custom(msg: T) -> Self { ConfigError { - message: msg.to_string(), + error: failure::err_msg(msg.to_string()), definition: None, } } } -// TODO: AFAIK, you cannot override Fail::cause (due to specialization), so we -// have no way to bubble up the underlying error. For now, it just formats -// the underlying cause as a string. impl From for ConfigError { - fn from(e: failure::Error) -> Self { - let message = e - .causes() - .map(|e| e.to_string()) - .collect::>() - .join("\nCaused by:\n "); + fn from(error: failure::Error) -> Self { ConfigError { - message, + error, definition: None, } }