From 022814d268f5cb97a32fe6f15f9132c93d025df6 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 26 Sep 2019 13:59:05 -0700 Subject: [PATCH 01/20] Move `config.rs` to `config/mod.rs` --- src/cargo/util/{config.rs => config/mod.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/cargo/util/{config.rs => config/mod.rs} (100%) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config/mod.rs similarity index 100% rename from src/cargo/util/config.rs rename to src/cargo/util/config/mod.rs From 0f73320ee7629719e2717a267d46e28550e55802 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 26 Sep 2019 14:09:17 -0700 Subject: [PATCH 02/20] Extra serde config support to a separate file --- src/cargo/util/config/de.rs | 355 ++++++++++++++++++++++++++++++++++ src/cargo/util/config/mod.rs | 358 +---------------------------------- 2 files changed, 361 insertions(+), 352 deletions(-) create mode 100644 src/cargo/util/config/de.rs diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs new file mode 100644 index 00000000000..aa0c92c8cd5 --- /dev/null +++ b/src/cargo/util/config/de.rs @@ -0,0 +1,355 @@ +//! Support for deserializing configuration via `serde` + +use crate::util::config::{Config, ConfigError, ConfigKey, ConfigKeyPart}; +use crate::util::config::{ConfigValue as CV, Value, Definition}; +use std::path::PathBuf; +use serde::{de, de::IntoDeserializer}; +use std::collections::HashSet; +use std::vec; + +/// Serde deserializer used to convert config values to a target type using +/// `Config::get`. +pub(crate) struct Deserializer<'config> { + pub(crate) config: &'config Config, + pub(crate) 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 self.config.cli_unstable().advanced_env + && v.starts_with('[') + && v.ends_with(']') + { + visitor.visit_seq(ConfigSeqAccess::new(self.config, &self.key)?) + } 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)?), + 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_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_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 + 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. + serde::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)); + } + } + 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 { + 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>, + { + 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))); + } + } + + 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, + )); + } + 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 error? + Some((value, _def)) => seed.deserialize(value.into_deserializer()).map(Some), + None => Ok(None), + } + } +} diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index af49d052a3c..6fb2f8503b6 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -12,12 +12,10 @@ use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::Once; use std::time::Instant; -use std::vec; use curl::easy::Easy; use lazycell::LazyCell; use serde::Deserialize; -use serde::{de, de::IntoDeserializer}; use url::Url; use self::ConfigValue as CV; @@ -32,6 +30,9 @@ use crate::util::Rustc; use crate::util::{paths, validate_package_name, FileLock}; use crate::util::{IntoUrl, IntoUrlWithBase}; +mod de; +use de::Deserializer; + /// Configuration information for cargo. This is not specific to a build, it is information /// relating to cargo itself. /// @@ -924,7 +925,7 @@ impl Config { // 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 { + pub fn get<'de, T: serde::de::Deserialize<'de>>(&self, key: &str) -> CargoResult { let d = Deserializer { config: self, key: ConfigKey::from_str(key), @@ -1041,7 +1042,7 @@ impl ConfigKeyPart { /// Key for a configuration variable. #[derive(Debug, Clone)] -struct ConfigKey(Vec); +pub(crate) struct ConfigKey(Vec); impl ConfigKey { fn from_str(key: &str) -> ConfigKey { @@ -1147,7 +1148,7 @@ impl fmt::Display for ConfigError { } } -impl de::Error for ConfigError { +impl serde::de::Error for ConfigError { fn custom(msg: T) -> Self { ConfigError { error: failure::err_msg(msg.to_string()), @@ -1165,353 +1166,6 @@ impl From for ConfigError { } } -/// 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 self.config.cli_unstable().advanced_env - && v.starts_with('[') - && v.ends_with(']') - { - visitor.visit_seq(ConfigSeqAccess::new(self.config, &self.key)?) - } 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)?), - 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_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_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 - 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. - serde::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)); - } - } - 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 { - 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>, - { - 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))); - } - } - - 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, - )); - } - 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 error? - 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. From c0baf844e3446d6f59e86ca17027a0441987dfb9 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 11:34:29 -0700 Subject: [PATCH 03/20] Refactor `ConfigKey` to its own file Also make it a little less allocation-heavy by tweaking the API to encourage incremental building of the key and incremental destruction as we walk throughout the configuration tree. --- src/cargo/util/config/de.rs | 201 +++++++++++++++++++++------------ src/cargo/util/config/key.rs | 68 +++++++++++ src/cargo/util/config/mod.rs | 161 ++++++-------------------- src/cargo/util/config/value.rs | 157 +++++++++++++++++++++++++ tests/testsuite/config.rs | 2 +- 5 files changed, 389 insertions(+), 200 deletions(-) create mode 100644 src/cargo/util/config/key.rs create mode 100644 src/cargo/util/config/value.rs diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index aa0c92c8cd5..42a9f55a148 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -1,14 +1,16 @@ //! Support for deserializing configuration via `serde` -use crate::util::config::{Config, ConfigError, ConfigKey, ConfigKeyPart}; -use crate::util::config::{ConfigValue as CV, Value, Definition}; -use std::path::PathBuf; +use crate::util::config::value; +use crate::util::config::{Config, ConfigError, ConfigKey}; +use crate::util::config::{ConfigValue as CV, Definition, Value}; use serde::{de, de::IntoDeserializer}; use std::collections::HashSet; +use std::path::PathBuf; use std::vec; /// Serde deserializer used to convert config values to a target type using /// `Config::get`. +#[derive(Clone)] pub(crate) struct Deserializer<'config> { pub(crate) config: &'config Config, pub(crate) key: ConfigKey, @@ -21,10 +23,10 @@ macro_rules! deserialize_method { V: de::Visitor<'de>, { let v = self.config.$getter(&self.key)?.ok_or_else(|| - ConfigError::missing(&self.key.to_config()))?; + ConfigError::missing(&self.key))?; let Value{val, definition} = v; let res: Result = visitor.$visit(val); - res.map_err(|e| e.with_key_context(&self.key.to_config(), definition)) + res.map_err(|e| e.with_key_context(&self.key, definition)) } } } @@ -39,7 +41,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { // 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()) { + if let Some(v) = self.config.env.get(self.key.as_env_key()) { let res: Result = if v == "true" || v == "false" { visitor.visit_bool(v.parse().unwrap()) } else if let Ok(v) = v.parse::() { @@ -48,38 +50,34 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { && v.starts_with('[') && v.ends_with(']') { - visitor.visit_seq(ConfigSeqAccess::new(self.config, &self.key)?) + visitor.visit_seq(ConfigSeqAccess::new(self.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()), + &self.key, + Definition::Environment(self.key.as_env_key().to_string()), ) }); } - let o_cv = self.config.get_cv(&self.key.to_config())?; + let o_cv = self.config.get_cv(self.key.as_config_key())?; 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)?), - path, - ), + CV::List(_, path) => (visitor.visit_seq(ConfigSeqAccess::new(self.clone())?), path), CV::Table(_, path) => ( - visitor.visit_map(ConfigMapAccess::new_map(self.config, self.key.clone())?), + visitor.visit_map(ConfigMapAccess::new_map(self.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))); + return res.map_err(|e| e.with_key_context(&self.key, Definition::Path(path))); } - Err(ConfigError::missing(&self.key.to_config())) + Err(ConfigError::missing(&self.key)) } deserialize_method!(deserialize_bool, visit_bool, get_bool_priv); @@ -107,35 +105,41 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { fn deserialize_struct( self, - _name: &'static str, + 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)?) + if name == value::NAME && fields == value::FIELDS { + return visitor.visit_map(ValueDeserializer { + hits: 0, + deserializer: self, + }); + } + visitor.visit_map(ConfigMapAccess::new_struct(self, fields)?) } fn deserialize_map(self, visitor: V) -> Result where V: de::Visitor<'de>, { - visitor.visit_map(ConfigMapAccess::new_map(self.config, self.key)?) + visitor.visit_map(ConfigMapAccess::new_map(self)?) } fn deserialize_seq(self, visitor: V) -> Result where V: de::Visitor<'de>, { - visitor.visit_seq(ConfigSeqAccess::new(self.config, &self.key)?) + visitor.visit_seq(ConfigSeqAccess::new(self)?) } fn deserialize_tuple(self, _len: usize, visitor: V) -> Result where V: de::Visitor<'de>, { - visitor.visit_seq(ConfigSeqAccess::new(self.config, &self.key)?) + visitor.visit_seq(ConfigSeqAccess::new(self)?) } fn deserialize_tuple_struct( @@ -147,7 +151,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { where V: de::Visitor<'de>, { - visitor.visit_seq(ConfigSeqAccess::new(self.config, &self.key)?) + visitor.visit_seq(ConfigSeqAccess::new(self)?) } fn deserialize_newtype_struct( @@ -169,7 +173,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { .to_string(); visitor.visit_newtype_struct(path.into_deserializer()) } - None => Err(ConfigError::missing(&self.key.to_config())), + None => Err(ConfigError::missing(&self.key)), } } else { visitor.visit_newtype_struct(self) @@ -185,70 +189,76 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { } struct ConfigMapAccess<'config> { - config: &'config Config, - key: ConfigKey, - set_iter: as IntoIterator>::IntoIter, - next: Option, + de: Deserializer<'config>, + set_iter: as IntoIterator>::IntoIter, + next: Option, +} + +#[derive(PartialEq, Eq, Hash)] +enum KeyKind { + Normal(String), + CaseSensitive(String), } impl<'config> ConfigMapAccess<'config> { - fn new_map( - config: &'config Config, - key: ConfigKey, - ) -> Result, ConfigError> { + fn new_map(de: Deserializer<'config>) -> Result, ConfigError> { let mut set = HashSet::new(); - if let Some(mut v) = config.get_table(&key.to_config())? { + if let Some(mut v) = de.config.get_table(de.key.as_config_key())? { // `v: Value>` for (key, _value) in v.val.drain() { - set.insert(ConfigKeyPart::CasePart(key)); + set.insert(KeyKind::CaseSensitive(key)); } } - if config.cli_unstable().advanced_env { + if de.config.cli_unstable().advanced_env { // `CARGO_PROFILE_DEV_OVERRIDES_` - let env_pattern = format!("{}_", key.to_env()); - for env_key in config.env.keys() { + let env_pattern = format!("{}_", de.key.as_env_key()); + for env_key in de.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())); + set.insert(KeyKind::CaseSensitive(part.to_string())); } } } Ok(ConfigMapAccess { - config, - key, + de, set_iter: set.into_iter(), next: None, }) } fn new_struct( - config: &'config Config, - key: ConfigKey, + de: Deserializer<'config>, fields: &'static [&'static str], ) -> Result, ConfigError> { let mut set = HashSet::new(); for field in fields { - set.insert(ConfigKeyPart::Part(field.to_string())); + set.insert(KeyKind::Normal(field.to_string())); } - if let Some(mut v) = config.get_table(&key.to_config())? { + + // Assume that if we're deserializing a struct it exhaustively lists all + // possible fields on this key that we're *supposed* to use, so take + // this opportunity to warn about any keys that aren't recognized as + // fields and warn about them. + if let Some(mut v) = de.config.get_table(de.key.as_config_key())? { 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() - ))?; + if set.contains(&KeyKind::Normal(t_key.to_string())) { + continue; } + de.config.shell().warn(format!( + "unused key `{}.{}` in config file `{}`", + de.key.as_config_key(), + t_key, + value.definition_path().display() + ))?; } } + Ok(ConfigMapAccess { - config, - key, + de, set_iter: set.into_iter(), next: None, }) @@ -264,9 +274,12 @@ impl<'de, 'config> de::MapAccess<'de> for ConfigMapAccess<'config> { { match self.set_iter.next() { Some(key) => { - let de_key = key.to_config(); + let name = match &key { + KeyKind::Normal(s) | KeyKind::CaseSensitive(s) => s.as_str(), + }; + let result = seed.deserialize(name.into_deserializer()).map(Some); self.next = Some(key); - seed.deserialize(de_key.into_deserializer()).map(Some) + return result; } None => Ok(None), } @@ -276,12 +289,16 @@ impl<'de, 'config> de::MapAccess<'de> for ConfigMapAccess<'config> { where V: de::DeserializeSeed<'de>, { - 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, - }) + match self.next.take().expect("next field missing") { + KeyKind::Normal(key) => self.de.key.push(&key), + KeyKind::CaseSensitive(key) => self.de.key.push_sensitive(&key), + } + let result = seed.deserialize(Deserializer { + config: self.de.config, + key: self.de.key.clone(), + }); + self.de.key.pop(); + return result; } } @@ -290,34 +307,32 @@ struct ConfigSeqAccess { } impl ConfigSeqAccess { - fn new(config: &Config, key: &ConfigKey) -> Result { + fn new(de: Deserializer<'_>) -> Result { let mut res = Vec::new(); - if let Some(v) = config.get_list(&key.to_config())? { + if let Some(v) = de.config.get_list(de.key.as_config_key())? { for (s, path) in v.val { res.push((s, Definition::Path(path))); } } - if config.cli_unstable().advanced_env { + if de.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 let Some(v) = de.config.env.get(de.key.as_env_key()) { + let def = Definition::Environment(de.key.as_env_key().to_string()); if !(v.starts_with('[') && v.ends_with(']')) { return Err(ConfigError::new( format!("should have TOML list syntax, found `{}`", v), def, )); } - let temp_key = key.last().to_env(); - let toml_s = format!("{}={}", temp_key, v); + let toml_s = format!("value={}", 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) + .get("value") .unwrap() .as_array() .expect("env var was not array"); @@ -353,3 +368,47 @@ impl<'de> de::SeqAccess<'de> for ConfigSeqAccess { } } } + +struct ValueDeserializer<'config> { + hits: u32, + deserializer: Deserializer<'config>, +} + +impl<'de, 'config> de::MapAccess<'de> for ValueDeserializer<'config> { + type Error = ConfigError; + + fn next_key_seed(&mut self, seed: K) -> Result, Self::Error> + where + K: de::DeserializeSeed<'de>, + { + self.hits += 1; + match self.hits { + 1 => seed + .deserialize(value::VALUE_FIELD.into_deserializer()) + .map(Some), + 2 => seed + .deserialize(value::DEFINITION_FIELD.into_deserializer()) + .map(Some), + _ => Ok(None), + } + } + + fn next_value_seed(&mut self, seed: V) -> Result + where + V: de::DeserializeSeed<'de>, + { + if self.hits == 1 { + seed.deserialize(Deserializer { + config: self.deserializer.config, + key: self.deserializer.key.clone(), + }) + } else { + // let env = self.deserializer.key.to_env(); + // if self.deserializer.config.env.contains_key(&env) { + // } else { + // } + // if let someself.deserializer.config.get_env(&self.deserializer.key) + panic!() + } + } +} diff --git a/src/cargo/util/config/key.rs b/src/cargo/util/config/key.rs new file mode 100644 index 00000000000..2a371626200 --- /dev/null +++ b/src/cargo/util/config/key.rs @@ -0,0 +1,68 @@ +use std::fmt; + +/// Key for a configuration variable. +#[derive(Debug, Clone)] +pub struct ConfigKey { + env: String, + config: String, + parts: Vec<(usize, usize)>, +} + +impl ConfigKey { + pub fn new() -> ConfigKey { + ConfigKey { + env: "CARGO".to_string(), + config: String::new(), + parts: Vec::new(), + } + } + + pub fn from_str(key: &str) -> ConfigKey { + let mut cfg = ConfigKey::new(); + for part in key.split('.') { + cfg.push(part); + } + return cfg; + } + + pub fn push(&mut self, name: &str) { + let env = name.replace("-", "_").to_uppercase(); + self._push(&env, name); + } + + pub fn push_sensitive(&mut self, name: &str) { + self._push(name, name); + } + + fn _push(&mut self, env: &str, config: &str) { + self.parts.push((self.env.len(), self.config.len())); + + self.env.push_str("_"); + self.env.push_str(env); + + if !self.config.is_empty() { + self.config.push_str("."); + } + self.config.push_str(config); + } + + pub fn pop(&mut self) { + let (env, config) = self.parts.pop().unwrap(); + self.env.truncate(env); + self.config.truncate(config); + } + + pub fn as_env_key(&self) -> &str { + &self.env + } + + pub fn as_config_key(&self) -> &str { + &self.config + } +} + +impl fmt::Display for ConfigKey { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_config_key().fmt(f) + } +} diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 6fb2f8503b6..2059bb7f057 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -33,6 +33,12 @@ use crate::util::{IntoUrl, IntoUrlWithBase}; mod de; use de::Deserializer; +mod value; +pub use value::{Definition, OptValue, Value}; + +mod key; +use key::ConfigKey; + /// Configuration information for cargo. This is not specific to a build, it is information /// relating to cargo itself. /// @@ -380,10 +386,9 @@ impl Config { T: FromStr, ::Err: fmt::Display, { - let key = key.to_env(); - match self.env.get(&key) { + match self.env.get(key.as_env_key()) { Some(value) => { - let definition = Definition::Environment(key); + let definition = Definition::Environment(key.as_env_key().to_string()); Ok(Some(Value { val: value .parse() @@ -396,15 +401,14 @@ impl Config { } fn has_key(&self, key: &ConfigKey) -> bool { - let env_key = key.to_env(); - if self.env.get(&env_key).is_some() { + if self.env.get(key.as_env_key()).is_some() { return true; } - let env_pattern = format!("{}_", env_key); + let env_pattern = format!("{}_", key.as_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 let Ok(o_cv) = self.get_cv(key.as_config_key()) { if o_cv.is_some() { return true; } @@ -421,14 +425,13 @@ impl Config { 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)?; + let o_cv = self.get_cv(key.as_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)), + Some(cv) => Err(ConfigError::expected(key.as_config_key(), "a string", &cv)), None => Ok(None), } } @@ -444,14 +447,17 @@ impl Config { 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)?; + let o_cv = self.get_cv(key.as_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)), + Some(cv) => Err(ConfigError::expected( + key.as_config_key(), + "true/false", + &cv, + )), None => Ok(None), } } @@ -548,15 +554,18 @@ impl Config { } fn get_integer(&self, key: &ConfigKey) -> Result, ConfigError> { - let config_key = key.to_config(); match self.get_env::(key)? { Some(v) => Ok(Some(v)), - None => match self.get_cv(&config_key)? { + None => match self.get_cv(key.as_config_key())? { Some(CV::Integer(i, path)) => Ok(Some(Value { val: i, definition: Definition::Path(path), })), - Some(cv) => Err(ConfigError::expected(&config_key, "an integer", &cv)), + Some(cv) => Err(ConfigError::expected( + key.as_config_key(), + "an integer", + &cv, + )), None => Ok(None), }, } @@ -1012,83 +1021,6 @@ impl Config { pub fn release_package_cache_lock(&self) {} } -/// 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), -} - -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)] -pub(crate) 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 { @@ -1116,16 +1048,20 @@ impl ConfigError { } } - fn missing(key: &str) -> ConfigError { + fn missing(key: &ConfigKey) -> ConfigError { ConfigError { - error: failure::format_err!("missing config key `{}`", key), + error: failure::format_err!("missing config key `{}`", key.as_config_key()), definition: None, } } - fn with_key_context(self, key: &str, definition: Definition) -> ConfigError { + fn with_key_context(self, key: &ConfigKey, definition: Definition) -> ConfigError { ConfigError { - error: failure::format_err!("could not load config key `{}`: {}", key, self), + error: failure::format_err!( + "could not load config key `{}`: {}", + key.as_config_key(), + self + ), definition: Some(definition), } } @@ -1187,19 +1123,6 @@ pub enum ConfigValue { Boolean(bool, PathBuf), } -pub struct Value { - pub val: T, - pub definition: Definition, -} - -pub type OptValue = Option>; - -#[derive(Clone, Debug)] -pub enum Definition { - Path(PathBuf), - Environment(String), -} - impl fmt::Debug for ConfigValue { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match *self { @@ -1381,24 +1304,6 @@ impl ConfigValue { } } -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(), - } - } -} - -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(ref key) => write!(f, "environment variable `{}`", key), - } - } -} - pub fn homedir(cwd: &Path) -> Option { ::home::cargo_home_with_cwd(cwd).ok() } diff --git a/src/cargo/util/config/value.rs b/src/cargo/util/config/value.rs new file mode 100644 index 00000000000..dd652e8ccdd --- /dev/null +++ b/src/cargo/util/config/value.rs @@ -0,0 +1,157 @@ +use crate::util::config::Config; +use serde::de; +use std::fmt; +use std::marker; +use std::path::{Path, PathBuf}; + +pub struct Value { + pub val: T, + pub definition: Definition, +} + +pub type OptValue = Option>; + +pub(crate) const VALUE_FIELD: &str = "$__cargo_private_value"; +pub(crate) const DEFINITION_FIELD: &str = "$__cargo_private_definition"; +pub(crate) const NAME: &str = "$__cargo_private_Value"; +pub(crate) static FIELDS: [&str; 2] = [VALUE_FIELD, DEFINITION_FIELD]; + +#[derive(Clone, Debug)] +pub enum Definition { + Path(PathBuf), + Environment(String), +} + +impl Definition { + pub fn root<'a>(&'a self, config: &'a Config) -> &'a Path { + match self { + Definition::Path(p) => p.parent().unwrap().parent().unwrap(), + Definition::Environment(_) => config.cwd(), + } + } +} + +impl fmt::Display for Definition { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Definition::Path(p) => p.display().fmt(f), + Definition::Environment(key) => write!(f, "environment variable `{}`", key), + } + } +} + +impl<'de, T> de::Deserialize<'de> for Value +where + T: de::Deserialize<'de>, +{ + fn deserialize(deserializer: D) -> Result, D::Error> + where + D: de::Deserializer<'de>, + { + struct ValueVisitor { + _marker: marker::PhantomData, + } + + impl<'de, T> de::Visitor<'de> for ValueVisitor + where + T: de::Deserialize<'de>, + { + type Value = Value; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("a value") + } + + fn visit_map(self, mut visitor: V) -> Result, V::Error> + where + V: de::MapAccess<'de>, + { + let value = visitor.next_key::()?; + if value.is_none() { + return Err(de::Error::custom("value not found")); + } + let val: T = visitor.next_value()?; + + let definition = visitor.next_key::()?; + if definition.is_none() { + return Err(de::Error::custom("definition not found")); + } + let definition: Definition = visitor.next_value()?; + Ok(Value { val, definition }) + } + } + + deserializer.deserialize_struct( + NAME, + &FIELDS, + ValueVisitor { + _marker: marker::PhantomData, + }, + ) + } +} + +struct FieldVisitor { + expected: &'static str, +} + +impl<'de> de::Visitor<'de> for FieldVisitor { + type Value = (); + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("a valid value field") + } + + fn visit_str(self, s: &str) -> Result<(), E> + where + E: de::Error, + { + if s == self.expected { + Ok(()) + } else { + Err(de::Error::custom("expected field with custom name")) + } + } +} + +struct ValueKey; + +impl<'de> de::Deserialize<'de> for ValueKey { + fn deserialize(deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + deserializer.deserialize_identifier(FieldVisitor { + expected: VALUE_FIELD, + })?; + Ok(ValueKey) + } +} + +struct DefinitionKey; + +impl<'de> de::Deserialize<'de> for DefinitionKey { + fn deserialize(deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + deserializer.deserialize_identifier(FieldVisitor { + expected: DEFINITION_FIELD, + })?; + Ok(DefinitionKey) + } +} + +impl<'de> de::Deserialize<'de> for Definition { + fn deserialize(deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + let (discr, value) = <(u32, String)>::deserialize(deserializer)?; + if discr == 0 { + Ok(Definition::Path(value.into())) + } else { + Ok(Definition::Environment(value)) + } + } +} diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index c06b76d81c4..34141427849 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -643,7 +643,7 @@ expected a list, but found a integer for `l3` in [..]/.cargo/config", 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 column 10", + could not parse TOML list: invalid number at line 1 column 8", ); // Try some other sequence-like types. From 37ace8886a51e075a1faf567b21a931abf682f53 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 12:29:01 -0700 Subject: [PATCH 04/20] Finish implementing `Value`, use it in helpers Rewrite helpers like `get_bool` to use `get::>>` instead of duplicating the logic that's already with the typed access of configuration. This is more along the effort to centralize all deserialization of configuration into typed values instead of using ad-hoc accessors in a number of locations. --- src/cargo/util/config/de.rs | 119 ++++++++++++++++++++++------------ src/cargo/util/config/mod.rs | 76 +++++++++------------- src/cargo/util/config/path.rs | 16 +++++ tests/testsuite/bad_config.rs | 4 +- tests/testsuite/config.rs | 11 ++-- 5 files changed, 132 insertions(+), 94 deletions(-) create mode 100644 src/cargo/util/config/path.rs diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index 42a9f55a148..75653c0e447 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -113,10 +113,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { V: de::Visitor<'de>, { if name == value::NAME && fields == value::FIELDS { - return visitor.visit_map(ValueDeserializer { - hits: 0, - deserializer: self, - }); + return visitor.visit_map(ValueDeserializer { hits: 0, de: self }); } visitor.visit_map(ConfigMapAccess::new_struct(self, fields)?) } @@ -154,37 +151,11 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { visitor.visit_seq(ConfigSeqAccess::new(self)?) } - 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)), - } - } else { - visitor.visit_newtype_struct(self) - } - } - // 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 + enum identifier ignored_any newtype_struct } } @@ -371,7 +342,7 @@ impl<'de> de::SeqAccess<'de> for ConfigSeqAccess { struct ValueDeserializer<'config> { hits: u32, - deserializer: Deserializer<'config>, + de: Deserializer<'config>, } impl<'de, 'config> de::MapAccess<'de> for ValueDeserializer<'config> { @@ -397,18 +368,80 @@ impl<'de, 'config> de::MapAccess<'de> for ValueDeserializer<'config> { where V: de::DeserializeSeed<'de>, { + macro_rules! bail { + ($($t:tt)*) => (return Err(failure::format_err!($($t)*).into())) + } + + // If this is the first time around we deserialize the `value` field + // which is the actual deserializer if self.hits == 1 { - seed.deserialize(Deserializer { - config: self.deserializer.config, - key: self.deserializer.key.clone(), - }) - } else { - // let env = self.deserializer.key.to_env(); - // if self.deserializer.config.env.contains_key(&env) { - // } else { - // } - // if let someself.deserializer.config.get_env(&self.deserializer.key) - panic!() + return seed.deserialize(self.de.clone()); + } + + // ... otherwise we're deserializing the `definition` field, so we need + // to figure out where the field we just deserialized was defined at. + let env = self.de.key.as_env_key(); + if self.de.config.env.contains_key(env) { + return seed.deserialize(Tuple2Deserializer(1i32, env)); + } + let val = match self.de.config.get_cv(self.de.key.as_config_key())? { + Some(val) => val, + None => bail!("failed to find definition of `{}`", self.de.key), + }; + let path = match val.definition_path().to_str() { + Some(s) => s, + None => bail!("failed to convert {:?} to utf-8", val.definition_path()), + }; + seed.deserialize(Tuple2Deserializer(0i32, path)) + } +} + +struct Tuple2Deserializer(T, U); + +impl<'de, T, U> de::Deserializer<'de> for Tuple2Deserializer +where + T: IntoDeserializer<'de, ConfigError>, + U: IntoDeserializer<'de, ConfigError>, +{ + type Error = ConfigError; + + fn deserialize_any(self, visitor: V) -> Result + where + V: de::Visitor<'de>, + { + struct SeqVisitor { + first: Option, + second: Option, } + impl<'de, T, U> de::SeqAccess<'de> for SeqVisitor + where + T: IntoDeserializer<'de, ConfigError>, + U: IntoDeserializer<'de, ConfigError>, + { + type Error = ConfigError; + fn next_element_seed(&mut self, seed: K) -> Result, Self::Error> + where + K: de::DeserializeSeed<'de>, + { + if let Some(first) = self.first.take() { + return seed.deserialize(first.into_deserializer()).map(Some); + } + if let Some(second) = self.second.take() { + return seed.deserialize(second.into_deserializer()).map(Some); + } + Ok(None) + } + } + + visitor.visit_seq(SeqVisitor { + first: Some(self.0), + second: Some(self.1), + }) + } + + serde::forward_to_deserialize_any! { + bool u8 u16 u32 u64 i8 i16 i32 i64 f32 f64 char str string seq + bytes byte_buf map struct option unit newtype_struct + ignored_any unit_struct tuple_struct tuple enum identifier } } diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 2059bb7f057..34dc189de13 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -39,6 +39,9 @@ pub use value::{Definition, OptValue, Value}; mod key; use key::ConfigKey; +mod path; +pub use path::ConfigRelativePath; + /// Configuration information for cargo. This is not specific to a build, it is information /// relating to cargo itself. /// @@ -417,8 +420,7 @@ impl Config { } pub fn get_string(&self, key: &str) -> CargoResult> { - self.get_string_priv(&ConfigKey::from_str(key)) - .map_err(|e| e.into()) + self.get::>>(key) } fn get_string_priv(&self, key: &ConfigKey) -> Result, ConfigError> { @@ -439,8 +441,7 @@ impl Config { } pub fn get_bool(&self, key: &str) -> CargoResult> { - self.get_bool_priv(&ConfigKey::from_str(key)) - .map_err(|e| e.into()) + self.get::>>(key) } fn get_bool_priv(&self, key: &ConfigKey) -> Result, ConfigError> { @@ -464,6 +465,15 @@ impl Config { } } + pub fn get_path(&self, key: &str) -> CargoResult> { + self.get::>>(key).map(|v| { + v.map(|v| Value { + val: v.val.resolve(self), + definition: v.definition, + }) + }) + } + fn string_to_path(&self, value: String, definition: &Definition) -> PathBuf { let is_path = value.contains('/') || (cfg!(windows) && value.contains('\\')); if is_path { @@ -474,17 +484,6 @@ impl Config { } } - pub fn get_path(&self, key: &str) -> CargoResult> { - if let Some(val) = self.get_string(key)? { - Ok(Some(Value { - val: self.string_to_path(val.val, &val.definition), - definition: val.definition, - })) - } else { - Ok(None) - } - } - pub fn get_path_and_args(&self, key: &str) -> CargoResult)>> { if let Some(mut val) = self.get_list_or_split_string(key)? { if !val.val.is_empty() { @@ -514,24 +513,26 @@ impl Config { } pub fn get_list_or_split_string(&self, key: &str) -> CargoResult>> { - 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, - })); + #[derive(Deserialize)] + #[serde(untagged)] + enum Target { + String(String), + List(Vec), } - 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), - })), - Some(CV::String(i, path)) => Ok(Some(Value { - val: i.split(' ').map(str::to_string).collect(), - definition: Definition::Path(path), - })), - Some(val) => self.expected("list or string", key, &val), + match self.get::>>(key)? { None => Ok(None), + Some(Value { + val: Target::String(s), + definition, + }) => Ok(Some(Value { + val: s.split(' ').map(str::to_string).collect(), + definition, + })), + Some(Value { + val: Target::List(val), + definition, + }) => Ok(Some(Value { val, definition })), } } @@ -549,8 +550,7 @@ impl Config { // Recommended to 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> { - self.get_integer(&ConfigKey::from_str(key)) - .map_err(|e| e.into()) + self.get::>>(key) } fn get_integer(&self, key: &ConfigKey) -> Result, ConfigError> { @@ -1102,18 +1102,6 @@ impl From for ConfigError { } } -/// 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 - } -} - #[derive(Eq, PartialEq, Clone)] pub enum ConfigValue { Integer(i64, PathBuf), diff --git a/src/cargo/util/config/path.rs b/src/cargo/util/config/path.rs new file mode 100644 index 00000000000..116b5b45b28 --- /dev/null +++ b/src/cargo/util/config/path.rs @@ -0,0 +1,16 @@ +use crate::util::config::{Config, Value}; +use serde::Deserialize; +use std::path::PathBuf; + +/// 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(Deserialize)] +#[serde(transparent)] +pub struct ConfigRelativePath(Value); + +impl ConfigRelativePath { + pub fn resolve(self, config: &Config) -> PathBuf { + config.string_to_path(self.0.val, &self.0.definition) + } +} diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index eb522c0ba87..af448e70b16 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -17,8 +17,8 @@ fn bad1() { .with_status(101) .with_stderr( "\ -[ERROR] expected table for configuration key `target.nonexistent-target`, \ -but found string in [..]config +[ERROR] invalid configuration for key `target.nonexistent-target` +expected a table, but found a string for `[..]` in [..]config ", ) .run(); diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 34141427849..36b48cc96bb 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -706,6 +706,7 @@ ns2 = 456 let config = new_config(&[("CARGO_NSE", "987"), ("CARGO_NS2", "654")]); #[derive(Debug, Deserialize, Eq, PartialEq)] + #[serde(transparent)] struct NewS(i32); assert_eq!(config.get::("ns").unwrap(), NewS(123)); assert_eq!(config.get::("ns2").unwrap(), NewS(654)); @@ -734,35 +735,35 @@ abs = '{}' config .get::("p1") .unwrap() - .path(), + .resolve(&config), paths::root().join("foo/bar") ); assert_eq!( config .get::("p2") .unwrap() - .path(), + .resolve(&config), paths::root().join("../abc") ); assert_eq!( config .get::("p3") .unwrap() - .path(), + .resolve(&config), paths::root().join("d/e") ); assert_eq!( config .get::("abs") .unwrap() - .path(), + .resolve(&config), paths::home() ); assert_eq!( config .get::("epath") .unwrap() - .path(), + .resolve(&config), paths::root().join("a/b") ); } From 5bba42615a0941cdfdb835fa0b65dfa9074918f0 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 12:33:28 -0700 Subject: [PATCH 05/20] Remove a usage of `get_list` This callsite doesn't need the full power of `get_list`, knowing the definition path of each element along the list. --- src/bin/cargo/main.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs index 9ed135cf123..3fbffa70f02 100644 --- a/src/bin/cargo/main.rs +++ b/src/bin/cargo/main.rs @@ -60,9 +60,7 @@ fn aliased_command(config: &Config, command: &str) -> CargoResult None, - Err(_) => config - .get_list(&alias_name)? - .map(|record| record.val.iter().map(|s| s.0.to_string()).collect()), + Err(_) => config.get::>>(&alias_name)?, }; let result = user_alias.or_else(|| match command { "b" => Some(vec!["build".to_string()]), From 2357bb041f476704285e19f12de0ad49c144441e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 12:51:44 -0700 Subject: [PATCH 06/20] Centralize HTTP configuration in one struct Gives us one nice place to access and document all HTTP-related configuration --- src/cargo/core/package.rs | 4 +-- src/cargo/ops/registry.rs | 50 +++++++++++++--------------------- src/cargo/util/config/mod.rs | 37 +++++++++++++++++++++---- src/cargo/util/config/path.rs | 2 +- src/cargo/util/config/value.rs | 12 ++++++++ 5 files changed, 64 insertions(+), 41 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index a64e4d84e46..8a3035c8f18 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -376,9 +376,7 @@ impl<'cfg> PackageSet<'cfg> { // that it's buggy, and we've empirically seen that it's buggy with HTTP // proxies. let mut multi = Multi::new(); - let multiplexing = config - .get::>("http.multiplexing")? - .unwrap_or(true); + let multiplexing = config.http_config()?.multiplexing.unwrap_or(true); multi .pipelining(false, multiplexing) .chain_err(|| "failed to enable multiplexing/pipelining in curl")?; diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index d326664de90..0de43750e9e 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -408,34 +408,26 @@ pub fn http_handle_and_timeout(config: &Config) -> CargoResult<(Easy, HttpTimeou } pub fn needs_custom_http_transport(config: &Config) -> CargoResult { - let proxy_exists = http_proxy_exists(config)?; - let timeout = HttpTimeout::new(config)?.is_non_default(); - let cainfo = config.get_path("http.cainfo")?; - let check_revoke = config.get_bool("http.check-revoke")?; - let user_agent = config.get_string("http.user-agent")?; - let ssl_version = config.get::>("http.ssl-version")?; - - Ok(proxy_exists - || timeout - || cainfo.is_some() - || check_revoke.is_some() - || user_agent.is_some() - || ssl_version.is_some()) + Ok(http_proxy_exists(config)? + || *config.http_config()? != Default::default() + || env::var_os("HTTP_TIMEOUT").is_some()) } /// Configure a libcurl http handle with the defaults options for Cargo pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult { + let http = config.http_config()?; if let Some(proxy) = http_proxy(config)? { handle.proxy(&proxy)?; } - if let Some(cainfo) = config.get_path("http.cainfo")? { - handle.cainfo(&cainfo.val)?; + if let Some(cainfo) = http.cainfo.clone() { + let cainfo = cainfo.resolve(config); + handle.cainfo(&cainfo)?; } - if let Some(check) = config.get_bool("http.check-revoke")? { - handle.ssl_options(SslOpt::new().no_revoke(!check.val))?; + if let Some(check) = http.check_revoke { + handle.ssl_options(SslOpt::new().no_revoke(!check))?; } - if let Some(user_agent) = config.get_string("http.user-agent")? { - handle.useragent(&user_agent.val)?; + if let Some(user_agent) = &http.user_agent { + handle.useragent(user_agent)?; } else { handle.useragent(&version().to_string())?; } @@ -456,7 +448,7 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult< }; Ok(version) } - if let Some(ssl_version) = config.get::>("http.ssl-version")? { + if let Some(ssl_version) = &http_config.ssl_version { match ssl_version { SslVersionConfig::Single(s) => { let version = to_ssl_version(s.as_str())?; @@ -472,7 +464,7 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult< } } - if let Some(true) = config.get::>("http.debug")? { + if let Some(true) = http.debug { handle.verbose(true)?; handle.debug_function(|kind, data| { let (prefix, level) = match kind { @@ -513,11 +505,10 @@ pub struct HttpTimeout { impl HttpTimeout { pub fn new(config: &Config) -> CargoResult { - let low_speed_limit = config - .get::>("http.low-speed-limit")? - .unwrap_or(10); + let config = config.http_config()?; + let low_speed_limit = config.low_speed_limit.unwrap_or(10); let seconds = config - .get::>("http.timeout")? + .timeout .or_else(|| env::var("HTTP_TIMEOUT").ok().and_then(|s| s.parse().ok())) .unwrap_or(30); Ok(HttpTimeout { @@ -526,10 +517,6 @@ impl HttpTimeout { }) } - fn is_non_default(&self) -> bool { - self.dur != Duration::new(30, 0) || self.low_speed_limit != 10 - } - pub fn configure(&self, handle: &mut Easy) -> CargoResult<()> { // The timeout option for libcurl by default times out the entire // transfer, but we probably don't want this. Instead we only set @@ -548,8 +535,9 @@ impl HttpTimeout { /// Favor cargo's `http.proxy`, then git's `http.proxy`. Proxies specified /// via environment variables are picked up by libcurl. fn http_proxy(config: &Config) -> CargoResult> { - if let Some(s) = config.get_string("http.proxy")? { - return Ok(Some(s.val)); + let http = config.http_config()?; + if let Some(s) = &http.proxy { + return Ok(Some(s.clone())); } if let Ok(cfg) = git2::Config::open_default() { if let Ok(s) = cfg.get_str("http.proxy") { diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 34dc189de13..dbdc2310c9b 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -94,6 +94,8 @@ pub struct Config { /// Lock, if held, of the global package cache along with the number of /// acquisitions so far. package_cache_lock: RefCell, usize)>>, + /// HTTP configuration for Cargo + http_config: LazyCell, } impl Config { @@ -152,6 +154,7 @@ impl Config { profiles: LazyCell::new(), updated_sources: LazyCell::new(), package_cache_lock: RefCell::new(None), + http_config: LazyCell::new(), } } @@ -916,6 +919,11 @@ impl Config { Ok(http) } + pub fn http_config(&self) -> CargoResult<&CargoHttpConfig> { + self.http_config + .try_borrow_with(|| Ok(self.get::("http")?)) + } + pub fn crates_io_source_id(&self, f: F) -> CargoResult where F: FnMut() -> CargoResult, @@ -1402,6 +1410,29 @@ pub fn clippy_driver() -> PathBuf { .into() } +#[derive(Clone, Debug, Deserialize)] +pub struct SslVersionConfigRange { + pub min: Option, + pub max: Option, +} + +#[derive(Debug, Default, Deserialize, PartialEq)] +pub struct CargoHttpConfig { + pub proxy: Option, + #[serde(rename = "low-speed-limit")] + pub low_speed_limit: Option, + pub timeout: Option, + pub cainfo: Option, + #[serde(rename = "check-revoke")] + pub check_revoke: Option, + #[serde(rename = "user-agent")] + pub user_agent: Option, + pub debug: Option, + pub multiplexing: Option, + #[serde(rename = "ssl-version")] + pub ssl_version: Option, +} + /// Configuration for `ssl-version` in `http` section /// There are two ways to configure: /// @@ -1421,9 +1452,3 @@ pub enum SslVersionConfig { Single(String), Range(SslVersionConfigRange), } - -#[derive(Clone, Debug, Deserialize)] -pub struct SslVersionConfigRange { - pub min: Option, - pub max: Option, -} diff --git a/src/cargo/util/config/path.rs b/src/cargo/util/config/path.rs index 116b5b45b28..e831f8710de 100644 --- a/src/cargo/util/config/path.rs +++ b/src/cargo/util/config/path.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; /// 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(Deserialize)] +#[derive(Debug, Deserialize, PartialEq, Clone)] #[serde(transparent)] pub struct ConfigRelativePath(Value); diff --git a/src/cargo/util/config/value.rs b/src/cargo/util/config/value.rs index dd652e8ccdd..893360baa56 100644 --- a/src/cargo/util/config/value.rs +++ b/src/cargo/util/config/value.rs @@ -2,8 +2,10 @@ use crate::util::config::Config; use serde::de; use std::fmt; use std::marker; +use std::mem; use std::path::{Path, PathBuf}; +#[derive(Debug, PartialEq, Clone)] pub struct Value { pub val: T, pub definition: Definition, @@ -31,6 +33,16 @@ impl Definition { } } +impl PartialEq for Definition { + fn eq(&self, other: &Definition) -> bool { + // configuration values are equivalent no matter where they're defined, + // but they need to be defined in the same location. For example if + // they're defined in the environment that's different than being + // defined in a file due to path interepretations. + mem::discriminant(self) == mem::discriminant(other) + } +} + impl fmt::Display for Definition { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { From cbda5322a0a44343c44710c995a07dca3d937f5a Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 12:55:46 -0700 Subject: [PATCH 07/20] Access `term` config through a deserialized type Going through and removing users of raw `get_*` functions! --- src/cargo/util/config/mod.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index dbdc2310c9b..24115383fd9 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -593,13 +593,18 @@ impl Config { let extra_verbose = verbose >= 2; let verbose = if verbose == 0 { None } else { Some(true) }; + #[derive(Deserialize, Default)] + struct TermConfig { + verbose: Option, + color: Option, + } + // Ignore errors in the configuration files. - let cfg_verbose = self.get_bool("term.verbose").unwrap_or(None).map(|v| v.val); - let cfg_color = self.get_string("term.color").unwrap_or(None).map(|v| v.val); + let cfg = self.get::("term").unwrap_or_default(); - let color = color.as_ref().or_else(|| cfg_color.as_ref()); + let color = color.as_ref().or_else(|| cfg.color.as_ref()); - let verbosity = match (verbose, cfg_verbose, quiet) { + let verbosity = match (verbose, cfg.verbose, quiet) { (Some(true), _, None) | (None, Some(true), None) => Verbosity::Verbose, // Command line takes precedence over configuration, so ignore the From 8d659063e5229d32bcf3701af9ceb48ec9630721 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 13:00:53 -0700 Subject: [PATCH 08/20] Consolidate `net` configuration into a typed structure Less need for `get_bool` and friends! --- src/cargo/sources/git/utils.rs | 6 ++---- src/cargo/util/config/mod.rs | 22 +++++++++++++++++++--- src/cargo/util/network.rs | 2 +- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 7afb17cd57c..c4183c57344 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -707,10 +707,8 @@ pub fn fetch( // repositories instead of `libgit2`-the-library. This should make more // flavors of authentication possible while also still giving us all the // speed and portability of using `libgit2`. - if let Some(val) = config.get_bool("net.git-fetch-with-cli")? { - if val.val { - return fetch_with_cli(repo, url, refspec, config); - } + if let Some(true) = config.net_config()?.git_fetch_with_cli { + return fetch_with_cli(repo, url, refspec, config); } debug!("doing a fetch for {}", url); diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 24115383fd9..77065d058b7 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -94,8 +94,9 @@ pub struct Config { /// Lock, if held, of the global package cache along with the number of /// acquisitions so far. package_cache_lock: RefCell, usize)>>, - /// HTTP configuration for Cargo + /// Cached configuration parsed by Cargo http_config: LazyCell, + net_config: LazyCell, } impl Config { @@ -155,6 +156,7 @@ impl Config { updated_sources: LazyCell::new(), package_cache_lock: RefCell::new(None), http_config: LazyCell::new(), + net_config: LazyCell::new(), } } @@ -638,8 +640,9 @@ impl Config { self.locked = locked; self.offline = offline || self - .get::>("net.offline") - .unwrap_or(None) + .net_config() + .ok() + .and_then(|n| n.offline) .unwrap_or(false); self.target_dir = cli_target_dir; self.cli_flags.parse(unstable_flags)?; @@ -929,6 +932,11 @@ impl Config { .try_borrow_with(|| Ok(self.get::("http")?)) } + pub fn net_config(&self) -> CargoResult<&CargoNetConfig> { + self.net_config + .try_borrow_with(|| Ok(self.get::("net")?)) + } + pub fn crates_io_source_id(&self, f: F) -> CargoResult where F: FnMut() -> CargoResult, @@ -1457,3 +1465,11 @@ pub enum SslVersionConfig { Single(String), Range(SslVersionConfigRange), } + +#[derive(Debug, Deserialize)] +pub struct CargoNetConfig { + pub retry: Option, + pub offline: Option, + #[serde(rename = "git-fetch-with-cli")] + pub git_fetch_with_cli: Option, +} diff --git a/src/cargo/util/network.rs b/src/cargo/util/network.rs index a927e9e044f..7637c1c2a46 100644 --- a/src/cargo/util/network.rs +++ b/src/cargo/util/network.rs @@ -15,7 +15,7 @@ impl<'a> Retry<'a> { pub fn new(config: &'a Config) -> CargoResult> { Ok(Retry { config, - remaining: config.get::>("net.retry")?.unwrap_or(2), + remaining: config.net_config()?.retry.unwrap_or(2), }) } From a458cd82e23d3e41d82a1d86ad522918ba13320e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 13:01:25 -0700 Subject: [PATCH 09/20] Remove no-longer-needed `Config::get_bool` --- src/cargo/util/config/mod.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 77065d058b7..4b17d2570cf 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -445,10 +445,6 @@ impl Config { } } - pub fn get_bool(&self, key: &str) -> CargoResult> { - self.get::>>(key) - } - fn get_bool_priv(&self, key: &ConfigKey) -> Result, ConfigError> { match self.get_env(key)? { Some(v) => Ok(Some(v)), From 09d9165d272d97f3b80f03cbd07b5a5b7da6c21d Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 13:02:50 -0700 Subject: [PATCH 10/20] Remove dead `Config::get_i64` method --- src/cargo/util/config/mod.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 4b17d2570cf..da4db8801ab 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -548,12 +548,6 @@ impl Config { } } - // Recommended to 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> { - self.get::>>(key) - } - fn get_integer(&self, key: &ConfigKey) -> Result, ConfigError> { match self.get_env::(key)? { Some(v) => Ok(Some(v)), From d7d8ca1e12de61d6aa20648f6d65b0120555538f Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 14:19:48 -0700 Subject: [PATCH 11/20] Consolidate `build` key configuration Add a typed structure which lists all `build` key configuration throughout Cargo. --- src/cargo/core/compiler/build_config.rs | 14 ++-- .../compiler/build_context/target_info.rs | 23 +++--- src/cargo/core/compiler/context/mod.rs | 5 +- src/cargo/core/compiler/output_depinfo.rs | 4 +- src/cargo/core/profiles.rs | 2 +- src/cargo/ops/registry.rs | 2 +- src/cargo/util/config/mod.rs | 82 +++++++++++++------ src/cargo/util/config/path.rs | 21 ++++- src/cargo/util/errors.rs | 2 +- tests/testsuite/config.rs | 13 +-- tests/testsuite/workspaces.rs | 2 + 11 files changed, 115 insertions(+), 55 deletions(-) diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index 893d9e08183..eb8d1f81a9a 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -62,15 +62,16 @@ impl BuildConfig { requested_target: &Option, mode: CompileMode, ) -> CargoResult { + let cfg = config.build_config()?; let requested_kind = match requested_target { Some(s) => CompileKind::Target(CompileTarget::new(s)?), - None => match config.get_string("build.target")? { - Some(cfg) => { - let value = if cfg.val.ends_with(".json") { - let path = cfg.definition.root(config).join(&cfg.val); + None => match &cfg.target { + Some(val) => { + let value = if val.raw_value().ends_with(".json") { + let path = val.clone().resolve_path(config); path.to_str().expect("must be utf-8 in toml").to_string() } else { - cfg.val + val.raw_value().to_string() }; CompileKind::Target(CompileTarget::new(&value)?) } @@ -88,8 +89,7 @@ impl BuildConfig { its environment, ignoring the `-j` parameter", )?; } - let cfg_jobs: Option = config.get("build.jobs")?; - let jobs = jobs.or(cfg_jobs).unwrap_or(::num_cpus::get() as u32); + let jobs = jobs.or(cfg.jobs).unwrap_or(::num_cpus::get() as u32); Ok(BuildConfig { requested_kind, diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs index 5aa084252df..40d53e977d5 100644 --- a/src/cargo/core/compiler/build_context/target_info.rs +++ b/src/cargo/core/compiler/build_context/target_info.rs @@ -7,6 +7,7 @@ use std::str::{self, FromStr}; use crate::core::compiler::CompileKind; use crate::core::TargetKind; use crate::util::{CargoResult, CargoResultExt, Config, ProcessBuilder, Rustc}; +use crate::util::config::StringList; use cargo_platform::{Cfg, CfgExpr}; /// Information about the platform target gleaned from querying rustc. @@ -427,9 +428,8 @@ fn env_args( CompileKind::Target(target) => target.short_name(), }; let key = format!("target.{}.{}", target, name); - if let Some(args) = config.get_list_or_split_string(&key)? { - let args = args.val.into_iter(); - rustflags.extend(args); + if let Some(args) = config.get::>(&key)? { + rustflags.extend(args.as_slice().iter().cloned()); } // ...including target.'cfg(...)'.rustflags if let Some(target_cfg) = target_cfg { @@ -450,9 +450,8 @@ fn env_args( for n in cfgs { let key = format!("target.{}.{}", n, name); - if let Some(args) = config.get_list_or_split_string(&key)? { - let args = args.val.into_iter(); - rustflags.extend(args); + if let Some(args) = config.get::>(&key)? { + rustflags.extend(args.as_slice().iter().cloned()); } } } @@ -463,10 +462,14 @@ fn env_args( } // Then the `build.rustflags` value. - let key = format!("build.{}", name); - if let Some(args) = config.get_list_or_split_string(&key)? { - let args = args.val.into_iter(); - return Ok(args.collect()); + let build = config.build_config()?; + let list = if name == "rustflags" { + &build.rustflags + } else { + &build.rustdocflags + }; + if let Some(list) = list { + return Ok(list.as_slice().to_vec()) } Ok(Vec::new()) diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index fae64dbb7c0..b4257751215 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -99,10 +99,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { } }; - let pipelining = bcx - .config - .get::>("build.pipelining")? - .unwrap_or(true); + let pipelining = bcx.config.build_config()?.pipelining.unwrap_or(true); Ok(Self { bcx, diff --git a/src/cargo/core/compiler/output_depinfo.rs b/src/cargo/core/compiler/output_depinfo.rs index a7acc8c4e08..5f2edd864f6 100644 --- a/src/cargo/core/compiler/output_depinfo.rs +++ b/src/cargo/core/compiler/output_depinfo.rs @@ -81,10 +81,10 @@ pub fn output_depinfo<'a, 'b>(cx: &mut Context<'a, 'b>, unit: &Unit<'a>) -> Carg let mut visited = HashSet::new(); let success = add_deps_for_unit(&mut deps, cx, unit, &mut visited).is_ok(); let basedir_string; - let basedir = match bcx.config.get_path("build.dep-info-basedir")? { + let basedir = match bcx.config.build_config()?.dep_info_basedir.clone() { Some(value) => { basedir_string = value - .val + .resolve_path(bcx.config) .as_os_str() .to_str() .ok_or_else(|| internal("build.dep-info-basedir path not utf-8"))? diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 2428285f13b..fef773f4a22 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -38,7 +38,7 @@ impl Profiles { let incremental = match env::var_os("CARGO_INCREMENTAL") { Some(v) => Some(v == "1"), - None => config.get::>("build.incremental")?, + None => config.build_config()?.incremental, }; if !features.is_enabled(Feature::named_profiles()) { diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 0de43750e9e..447beeb5f3c 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -420,7 +420,7 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult< handle.proxy(&proxy)?; } if let Some(cainfo) = http.cainfo.clone() { - let cainfo = cainfo.resolve(config); + let cainfo = cainfo.resolve_path(config); handle.cainfo(&cainfo)?; } if let Some(check) = http.check_revoke { diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index da4db8801ab..3af60ef131e 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -97,6 +97,7 @@ pub struct Config { /// Cached configuration parsed by Cargo http_config: LazyCell, net_config: LazyCell, + build_config: LazyCell, } impl Config { @@ -157,6 +158,7 @@ impl Config { package_cache_lock: RefCell::new(None), http_config: LazyCell::new(), net_config: LazyCell::new(), + build_config: LazyCell::new(), } } @@ -338,12 +340,12 @@ impl Config { } pub fn target_dir(&self) -> CargoResult> { - if let Some(ref dir) = self.target_dir { + if let Some(dir) = &self.target_dir { Ok(Some(dir.clone())) } else if let Some(dir) = env::var_os("CARGO_TARGET_DIR") { Ok(Some(Filesystem::new(self.cwd.join(dir)))) - } else if let Some(val) = self.get_path("build.target-dir")? { - let val = self.cwd.join(val.val); + } else if let Some(val) = &self.build_config()?.target_dir { + let val = val.resolve_path(self); Ok(Some(Filesystem::new(val))) } else { Ok(None) @@ -469,7 +471,7 @@ impl Config { pub fn get_path(&self, key: &str) -> CargoResult> { self.get::>>(key).map(|v| { v.map(|v| Value { - val: v.val.resolve(self), + val: v.val.resolve_program(self), definition: v.definition, }) }) @@ -513,27 +515,13 @@ impl Config { } } - pub fn get_list_or_split_string(&self, key: &str) -> CargoResult>> { - #[derive(Deserialize)] - #[serde(untagged)] - enum Target { - String(String), - List(Vec), - } - - match self.get::>>(key)? { + fn get_list_or_split_string(&self, key: &str) -> CargoResult>> { + match self.get::>>(key)? { None => Ok(None), - Some(Value { - val: Target::String(s), - definition, - }) => Ok(Some(Value { - val: s.split(' ').map(str::to_string).collect(), - definition, + Some(val) => Ok(Some(Value { + val: val.val.list, + definition: val.definition, })), - Some(Value { - val: Target::List(val), - definition, - }) => Ok(Some(Value { val, definition })), } } @@ -927,6 +915,11 @@ impl Config { .try_borrow_with(|| Ok(self.get::("net")?)) } + pub fn build_config(&self) -> CargoResult<&CargoBuildConfig> { + self.build_config + .try_borrow_with(|| Ok(self.get::("build")?)) + } + pub fn crates_io_source_id(&self, f: F) -> CargoResult where F: FnMut() -> CargoResult, @@ -1463,3 +1456,46 @@ pub struct CargoNetConfig { #[serde(rename = "git-fetch-with-cli")] pub git_fetch_with_cli: Option, } + +#[derive(Debug, Deserialize)] +pub struct CargoBuildConfig { + pub pipelining: Option, + #[serde(rename = "dep-info-basedir")] + pub dep_info_basedir: Option, + #[serde(rename = "target-dir")] + pub target_dir: Option, + pub incremental: Option, + pub target: Option, + pub jobs: Option, + pub rustflags: Option, + pub rustdocflags: Option, +} + +#[derive(Debug)] +pub struct StringList { + list: Vec, +} + +impl StringList { + pub fn as_slice(&self) -> &[String] { + &self.list + } +} + +impl<'de> serde::de::Deserialize<'de> for StringList { + fn deserialize>(d: D) -> Result { + #[derive(Deserialize)] + #[serde(untagged)] + enum Target { + String(String), + List(Vec), + } + + Ok(match Target::deserialize(d)? { + Target::String(s) => StringList { + list: s.split_whitespace().map(str::to_string).collect(), + }, + Target::List(list) => StringList { list }, + }) + } +} diff --git a/src/cargo/util/config/path.rs b/src/cargo/util/config/path.rs index e831f8710de..4a689f2667b 100644 --- a/src/cargo/util/config/path.rs +++ b/src/cargo/util/config/path.rs @@ -10,7 +10,26 @@ use std::path::PathBuf; pub struct ConfigRelativePath(Value); impl ConfigRelativePath { - pub fn resolve(self, config: &Config) -> PathBuf { + /// Returns the raw underlying configuration value for this key. + pub fn raw_value(&self) -> &str { + &self.0.val + } + + /// Resolves this configuration-relative path to an absolute path. + /// + /// This will always return an absolute path where it's relative to the + /// location for configuration for this value. + pub fn resolve_path(&self, config: &Config) -> PathBuf { + self.0.definition.root(config).join(&self.0.val) + } + + /// Resolves this configuration-relative path to either an absolute path or + /// something appropriate to execute from `PATH`. + /// + /// Values which don't look like a filesystem path (don't contain `/` or + /// `\`) will be returned as-is, and everything else will fall through to an + /// absolute path. + pub fn resolve_program(self, config: &Config) -> PathBuf { config.string_to_path(self.0.val, &self.0.definition) } } diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index 1abba82b3ff..8ba0a5e6477 100644 --- a/src/cargo/util/errors.rs +++ b/src/cargo/util/errors.rs @@ -394,5 +394,5 @@ pub fn display_causes(error: &Error) -> String { .iter_chain() .map(|e| e.to_string()) .collect::>() - .join("\nCaused by:\n ") + .join("\n\nCaused by:\n ") } diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 36b48cc96bb..3632b4d37f8 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -560,10 +560,13 @@ fn config_bad_toml() { 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 column 5", ); @@ -735,35 +738,35 @@ abs = '{}' config .get::("p1") .unwrap() - .resolve(&config), + .resolve_path(&config), paths::root().join("foo/bar") ); assert_eq!( config .get::("p2") .unwrap() - .resolve(&config), + .resolve_path(&config), paths::root().join("../abc") ); assert_eq!( config .get::("p3") .unwrap() - .resolve(&config), + .resolve_path(&config), paths::root().join("d/e") ); assert_eq!( config .get::("abs") .unwrap() - .resolve(&config), + .resolve_path(&config), paths::home() ); assert_eq!( config .get::("epath") .unwrap() - .resolve(&config), + .resolve_path(&config), paths::root().join("a/b") ); } diff --git a/tests/testsuite/workspaces.rs b/tests/testsuite/workspaces.rs index a86955f908b..d3444e1aab2 100644 --- a/tests/testsuite/workspaces.rs +++ b/tests/testsuite/workspaces.rs @@ -1061,8 +1061,10 @@ fn new_warning_with_corrupt_ws() { [WARNING] compiling this new crate may not work due to invalid workspace configuration failed to parse manifest at `[..]foo/Cargo.toml` + Caused by: could not parse input as TOML + Caused by: expected an equals, found eof at line 1 column 5 Created binary (application) `bar` package From 44a31d676de445b602c6ebce340cda71cf5c5728 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 27 Sep 2019 14:24:15 -0700 Subject: [PATCH 12/20] Simplify `cargo-new` configuration reading No need for lots of extra helpers/parsing when using serde! --- src/cargo/ops/cargo_new.rs | 73 +++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 36 deletions(-) diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index 512eed7e3b3..e7c3facb031 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/src/cargo/ops/cargo_new.rs @@ -1,17 +1,18 @@ +use crate::core::{compiler, Workspace}; +use crate::util::errors::{self, CargoResult, CargoResultExt}; +use crate::util::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo}; +use crate::util::{paths, validate_package_name, Config}; +use git2::Config as GitConfig; +use git2::Repository as GitRepository; +use serde::de; +use serde::Deserialize; use std::collections::BTreeMap; use std::env; use std::fmt; use std::fs; use std::io::{BufRead, BufReader, ErrorKind}; use std::path::{Path, PathBuf}; - -use git2::Config as GitConfig; -use git2::Repository as GitRepository; - -use crate::core::{compiler, Workspace}; -use crate::util::errors::{self, CargoResult, CargoResultExt}; -use crate::util::{existing_vcs_repo, internal, FossilRepo, GitRepo, HgRepo, PijulRepo}; -use crate::util::{paths, validate_package_name, Config}; +use std::str::FromStr; use toml; @@ -24,6 +25,31 @@ pub enum VersionControl { NoVcs, } +impl FromStr for VersionControl { + type Err = failure::Error; + + fn from_str(s: &str) -> Result { + match s { + "git" => Ok(VersionControl::Git), + "hg" => Ok(VersionControl::Hg), + "pijul" => Ok(VersionControl::Pijul), + "fossil" => Ok(VersionControl::Fossil), + "none" => Ok(VersionControl::NoVcs), + other => failure::bail!("unknown vcs specification: `{}`", other), + } + } +} + +impl<'de> de::Deserialize<'de> for VersionControl { + fn deserialize(deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + FromStr::from_str(&s).map_err(de::Error::custom) + } +} + #[derive(Debug)] pub struct NewOptions { pub version_control: Option, @@ -102,9 +128,11 @@ impl NewOptions { } } +#[derive(Deserialize)] struct CargoNewConfig { name: Option, email: Option, + #[serde(rename = "vcs")] version_control: Option, } @@ -543,7 +571,7 @@ fn init_vcs(path: &Path, vcs: VersionControl, config: &Config) -> CargoResult<() fn mk(config: &Config, opts: &MkOptions<'_>) -> CargoResult<()> { let path = opts.path; let name = opts.name; - let cfg = global_config(config)?; + let cfg = config.get::("cargo-new")?; // Using the push method with two arguments ensures that the entries for // both `ignore` and `hgignore` are in sync. @@ -752,30 +780,3 @@ fn discover_author() -> CargoResult<(String, Option)> { Ok((name, email)) } - -fn global_config(config: &Config) -> CargoResult { - let name = config.get_string("cargo-new.name")?.map(|s| s.val); - let email = config.get_string("cargo-new.email")?.map(|s| s.val); - let vcs = config.get_string("cargo-new.vcs")?; - - let vcs = match vcs.as_ref().map(|p| (&p.val[..], &p.definition)) { - Some(("git", _)) => Some(VersionControl::Git), - Some(("hg", _)) => Some(VersionControl::Hg), - Some(("pijul", _)) => Some(VersionControl::Pijul), - Some(("none", _)) => Some(VersionControl::NoVcs), - Some((s, p)) => { - return Err(internal(format!( - "invalid configuration for key \ - `cargo-new.vcs`, unknown vcs `{}` \ - (found in {})", - s, p - ))); - } - None => None, - }; - Ok(CargoNewConfig { - name, - email, - version_control: vcs, - }) -} From fe7b5fc6decff0e76a27a43776af196c9aef7d18 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 3 Oct 2019 12:17:18 -0700 Subject: [PATCH 13/20] Handle rebase conflicts --- src/cargo/ops/registry.rs | 12 +++++++----- src/cargo/util/config/mod.rs | 14 +++++++------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 447beeb5f3c..fe7e68eaa05 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -448,17 +448,19 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult< }; Ok(version) } - if let Some(ssl_version) = &http_config.ssl_version { + if let Some(ssl_version) = &http.ssl_version { match ssl_version { SslVersionConfig::Single(s) => { let version = to_ssl_version(s.as_str())?; handle.ssl_version(version)?; } SslVersionConfig::Range(SslVersionConfigRange { min, max }) => { - let min_version = - min.map_or(Ok(SslVersion::Default), |s| to_ssl_version(s.as_str()))?; - let max_version = - max.map_or(Ok(SslVersion::Default), |s| to_ssl_version(s.as_str()))?; + let min_version = min + .as_ref() + .map_or(Ok(SslVersion::Default), |s| to_ssl_version(s))?; + let max_version = max + .as_ref() + .map_or(Ok(SslVersion::Default), |s| to_ssl_version(s))?; handle.ssl_min_max_version(min_version, max_version)?; } } diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 3af60ef131e..26d88c006fc 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1406,12 +1406,6 @@ pub fn clippy_driver() -> PathBuf { .into() } -#[derive(Clone, Debug, Deserialize)] -pub struct SslVersionConfigRange { - pub min: Option, - pub max: Option, -} - #[derive(Debug, Default, Deserialize, PartialEq)] pub struct CargoHttpConfig { pub proxy: Option, @@ -1442,13 +1436,19 @@ pub struct CargoHttpConfig { /// ssl-version.min = "tlsv1.2" /// ssl-version.max = "tlsv1.3" /// ``` -#[derive(Clone, Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize, PartialEq)] #[serde(untagged)] pub enum SslVersionConfig { Single(String), Range(SslVersionConfigRange), } +#[derive(Clone, Debug, Deserialize, PartialEq)] +pub struct SslVersionConfigRange { + pub min: Option, + pub max: Option, +} + #[derive(Debug, Deserialize)] pub struct CargoNetConfig { pub retry: Option, From 3a6cd74434cc1fdd57266740d4827ed162580005 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 3 Oct 2019 12:18:48 -0700 Subject: [PATCH 14/20] Run rustfmt --- src/cargo/core/compiler/build_context/target_info.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs index 40d53e977d5..3c23d2d8b75 100644 --- a/src/cargo/core/compiler/build_context/target_info.rs +++ b/src/cargo/core/compiler/build_context/target_info.rs @@ -6,8 +6,8 @@ use std::str::{self, FromStr}; use crate::core::compiler::CompileKind; use crate::core::TargetKind; -use crate::util::{CargoResult, CargoResultExt, Config, ProcessBuilder, Rustc}; use crate::util::config::StringList; +use crate::util::{CargoResult, CargoResultExt, Config, ProcessBuilder, Rustc}; use cargo_platform::{Cfg, CfgExpr}; /// Information about the platform target gleaned from querying rustc. @@ -469,7 +469,7 @@ fn env_args( &build.rustdocflags }; if let Some(list) = list { - return Ok(list.as_slice().to_vec()) + return Ok(list.as_slice().to_vec()); } Ok(Vec::new()) From dab42bbbc7fa2fc2aee8f2c4e21f4ec70a1a521b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 7 Oct 2019 16:52:07 -0700 Subject: [PATCH 15/20] Use `#[serde(rename_all)]` instead of listing them all --- src/cargo/util/config/mod.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 26d88c006fc..194b342db9b 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1407,19 +1407,16 @@ pub fn clippy_driver() -> PathBuf { } #[derive(Debug, Default, Deserialize, PartialEq)] +#[serde(rename_all = "kebab-case")] pub struct CargoHttpConfig { pub proxy: Option, - #[serde(rename = "low-speed-limit")] pub low_speed_limit: Option, pub timeout: Option, pub cainfo: Option, - #[serde(rename = "check-revoke")] pub check_revoke: Option, - #[serde(rename = "user-agent")] pub user_agent: Option, pub debug: Option, pub multiplexing: Option, - #[serde(rename = "ssl-version")] pub ssl_version: Option, } @@ -1450,19 +1447,18 @@ pub struct SslVersionConfigRange { } #[derive(Debug, Deserialize)] +#[serde(rename_all = "kebab-case")] pub struct CargoNetConfig { pub retry: Option, pub offline: Option, - #[serde(rename = "git-fetch-with-cli")] pub git_fetch_with_cli: Option, } #[derive(Debug, Deserialize)] +#[serde(rename_all = "kebab-case")] pub struct CargoBuildConfig { pub pipelining: Option, - #[serde(rename = "dep-info-basedir")] pub dep_info_basedir: Option, - #[serde(rename = "target-dir")] pub target_dir: Option, pub incremental: Option, pub target: Option, From a0d94ffd02dddc9c510967bc4171639ec1128cd1 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 7 Oct 2019 16:53:36 -0700 Subject: [PATCH 16/20] Add documentation for `StringList` --- src/cargo/util/config/mod.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 194b342db9b..435dd2a5f02 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1467,6 +1467,16 @@ pub struct CargoBuildConfig { pub rustdocflags: Option, } +/// A type to deserialize a list of strings from a toml file. +/// +/// Supports deserializing either a whitespace-separated list of arguments in a +/// single string or a string list itself. For example these deserialize to +/// equivalent values: +/// +/// ``` +/// a = 'a b c' +/// b = ['a', 'b', 'c'] +/// ``` #[derive(Debug)] pub struct StringList { list: Vec, From 0a9a46eb24aebe9fa716f031240b289ecb90e7e5 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 7 Oct 2019 16:54:18 -0700 Subject: [PATCH 17/20] Remove unnecessary clone --- src/cargo/ops/registry.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index fe7e68eaa05..2e44e79a629 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -419,7 +419,7 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult< if let Some(proxy) = http_proxy(config)? { handle.proxy(&proxy)?; } - if let Some(cainfo) = http.cainfo.clone() { + if let Some(cainfo) = &http.cainfo { let cainfo = cainfo.resolve_path(config); handle.cainfo(&cainfo)?; } From 2387e1a867ddd19443921f581db056bf3e5a4305 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 7 Oct 2019 17:02:18 -0700 Subject: [PATCH 18/20] Document the new `ConfigKey` implementation --- src/cargo/util/config/key.rs | 41 ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/cargo/util/config/key.rs b/src/cargo/util/config/key.rs index 2a371626200..09aefe9ec99 100644 --- a/src/cargo/util/config/key.rs +++ b/src/cargo/util/config/key.rs @@ -1,14 +1,34 @@ use std::fmt; /// Key for a configuration variable. +/// +/// This type represents a configuration variable that we're looking up in +/// Cargo's configuration. This structure simultaneously keeps track of a +/// corresponding environment variable name as well as a TOML config name. The +/// intention here is that this is built up and torn down over time efficiently, +/// avoiding clones and such as possible. #[derive(Debug, Clone)] pub struct ConfigKey { + // The current environment variable this configuration key maps to. This is + // updated with `push` methods and looks like `CARGO_FOO_BAR` for pushing + // `foo` and then `bar`. env: String, + // The current environment variable this configuration key maps to. This is + // updated with `push` methods and looks like `foo.bar` for pushing + // `foo` and then `bar`. config: String, + // This is used to keep track of how many sub-keys have been pushed on this + // `ConfigKey`. Each element of this vector is a new sub-key pushed onto + // this `ConfigKey`. Each element is a pair of `usize` where the first item + // is an index into `env` and the second item is an index into `config`. + // These indices are used on `pop` to truncate `env` and `config` to rewind + // back to the previous `ConfigKey` state before a `push`. parts: Vec<(usize, usize)>, } impl ConfigKey { + /// Creates a new blank configuration key which is ready to get built up by + /// using `push` and `push_sensitive`. pub fn new() -> ConfigKey { ConfigKey { env: "CARGO".to_string(), @@ -17,6 +37,10 @@ impl ConfigKey { } } + /// Creates a `ConfigKey` from the `key` specified. + /// + /// The `key` specified is expected to be a period-separated toml + /// configuration key. pub fn from_str(key: &str) -> ConfigKey { let mut cfg = ConfigKey::new(); for part in key.split('.') { @@ -25,11 +49,22 @@ impl ConfigKey { return cfg; } + /// Pushes a new sub-key on this `ConfigKey`. This sub-key should be + /// equivalent to accessing a sub-table in TOML. + /// + /// Note that this considers `name` to be case-insensitive, meaning that the + /// corrseponding toml key is appended with this `name` as-is and the + /// corresponding env key is appended with `name` after transforming it to + /// uppercase characters. pub fn push(&mut self, name: &str) { let env = name.replace("-", "_").to_uppercase(); self._push(&env, name); } + /// Performs the same function as `push` except that the corresponding + /// environment variable does not get the uppercase letters of `name` but + /// instead `name` is pushed raw onto the corresponding environment + /// variable. pub fn push_sensitive(&mut self, name: &str) { self._push(name, name); } @@ -46,16 +81,22 @@ impl ConfigKey { self.config.push_str(config); } + /// Rewinds this `ConfigKey` back to the state it was at before the last + /// `push` method being called. pub fn pop(&mut self) { let (env, config) = self.parts.pop().unwrap(); self.env.truncate(env); self.config.truncate(config); } + /// Returns the corresponding environment variable key for this + /// configuration value. pub fn as_env_key(&self) -> &str { &self.env } + /// Returns the corresponding TOML (period-separated) key for this + /// configuration value. pub fn as_config_key(&self) -> &str { &self.config } From e0667f58310655d99a3bc453f9a15d3d16a33c00 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 7 Oct 2019 17:11:30 -0700 Subject: [PATCH 19/20] Add comments of `Value`'s deserialization protocol --- src/cargo/util/config/de.rs | 14 ++++++++++++++ src/cargo/util/config/value.rs | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index 75653c0e447..4471727db0c 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -112,6 +112,10 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { where V: de::Visitor<'de>, { + // Match on the magical struct name/field names that are passed in to + // detect when we're deserializing `Value`. + // + // See more comments in `value.rs` for the protocol used here. if name == value::NAME && fields == value::FIELDS { return visitor.visit_map(ValueDeserializer { hits: 0, de: self }); } @@ -340,6 +344,13 @@ impl<'de> de::SeqAccess<'de> for ConfigSeqAccess { } } +/// This is a deserializer that deserializes into a `Value` for +/// configuration. +/// +/// This is a special deserializer because it deserializes one of its struct +/// fields into the location that this configuration value was defined in. +/// +/// See more comments in `value.rs` for the protocol used here. struct ValueDeserializer<'config> { hits: u32, de: Deserializer<'config>, @@ -396,6 +407,9 @@ impl<'de, 'config> de::MapAccess<'de> for ValueDeserializer<'config> { } } +/// A deserializer which takes two values and deserializes into a tuple of those +/// two values. This is similar to types like `StrDeserializer` in upstream +/// serde itself. struct Tuple2Deserializer(T, U); impl<'de, T, U> de::Deserializer<'de> for Tuple2Deserializer diff --git a/src/cargo/util/config/value.rs b/src/cargo/util/config/value.rs index 893360baa56..0a428046259 100644 --- a/src/cargo/util/config/value.rs +++ b/src/cargo/util/config/value.rs @@ -1,3 +1,13 @@ +//! Deserialization of a `Value` type which tracks where it was deserialized +//! from. +//! +//! Often Cargo wants to report semantic error information or other sorts of +//! error information about configuration keys but it also may wish to indicate +//! as an error context where the key was defined as well (to help user +//! debugging). The `Value` type here can be used to deserialize a `T` value +//! from configuration, but also record where it was deserialized from when it +//! was read. + use crate::util::config::Config; use serde::de; use std::fmt; @@ -5,14 +15,37 @@ use std::marker; use std::mem; use std::path::{Path, PathBuf}; +/// A type which can be deserialized as a configuration value which records +/// where it was deserialized from. #[derive(Debug, PartialEq, Clone)] pub struct Value { + /// The inner value that was deserialized. pub val: T, + /// The location where `val` was defined in configuration (e.g. file it was + /// defined in, env var etc). pub definition: Definition, } pub type OptValue = Option>; +// Deserializing `Value` is pretty special, and serde doesn't have built-in +// support for this operation. To implement this we extend serde's "data model" +// a bit. We configure deserialization of `Value` to basically only work with +// our one deserializer using configuration. +// +// We define that `Value` deserialization asks the deserializer for a very +// special struct name and struct field names. In doing so the deserializer will +// recognize this and synthesize a magical value for the `definition` field when +// we deserialize it. This protocol is how we're able to have a channel of +// information flowing from the configuration deserializer into the +// deserialization implementation here. +// +// You'll want to also check out the implementation of `ValueDeserializer` in +// `de.rs`. Also note that the names below are intended to be invalid Rust +// identifiers to avoid how they might conflict with other valid structures. +// Finally the `definition` field is transmitted as a tuple of i32/string, which +// is effectively a tagged union of `Definition` itself. + pub(crate) const VALUE_FIELD: &str = "$__cargo_private_value"; pub(crate) const DEFINITION_FIELD: &str = "$__cargo_private_definition"; pub(crate) const NAME: &str = "$__cargo_private_Value"; From 9a12e48c8dc6655782842d69c05ecaa37be838fd Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 9 Oct 2019 09:54:57 -0700 Subject: [PATCH 20/20] More review comments --- src/cargo/util/config/key.rs | 2 +- src/cargo/util/config/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/config/key.rs b/src/cargo/util/config/key.rs index 09aefe9ec99..9fabe9a00fb 100644 --- a/src/cargo/util/config/key.rs +++ b/src/cargo/util/config/key.rs @@ -13,7 +13,7 @@ pub struct ConfigKey { // updated with `push` methods and looks like `CARGO_FOO_BAR` for pushing // `foo` and then `bar`. env: String, - // The current environment variable this configuration key maps to. This is + // The current toml key this configuration key maps to. This is // updated with `push` methods and looks like `foo.bar` for pushing // `foo` and then `bar`. config: String, diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 435dd2a5f02..62e00737c49 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1473,7 +1473,7 @@ pub struct CargoBuildConfig { /// single string or a string list itself. For example these deserialize to /// equivalent values: /// -/// ``` +/// ```toml /// a = 'a b c' /// b = ['a', 'b', 'c'] /// ```