diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index ff9d89ddf52..d071de17e51 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -4,15 +4,22 @@ 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::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, +pub(super) struct Deserializer<'config> { + pub(super) config: &'config Config, + /// The current key being deserialized. + pub(super) key: ConfigKey, + /// Whether or not this key part is allowed to be an inner table. For + /// example, `profile.dev.build-override` needs to check if + /// CARGO_PROFILE_DEV_BUILD_OVERRIDE_ prefixes exist. But + /// CARGO_BUILD_TARGET should not check for prefixes because it would + /// collide with CARGO_BUILD_TARGET_DIR. See `ConfigMapAccess` for + /// details. + pub(super) env_prefix_ok: bool, } macro_rules! deserialize_method { @@ -109,7 +116,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { where V: de::Visitor<'de>, { - if self.config.has_key(&self.key) { + if self.config.has_key(&self.key, self.env_prefix_ok) { visitor.visit_some(self) } else { // Treat missing values as `None`. @@ -179,8 +186,10 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> { struct ConfigMapAccess<'config> { de: Deserializer<'config>, - set_iter: as IntoIterator>::IntoIter, - next: Option, + /// The fields that this map should deserialize. + fields: Vec, + /// Current field being deserialized. + field_index: usize, } #[derive(Debug, PartialEq, Eq, Hash)] @@ -191,31 +200,31 @@ enum KeyKind { impl<'config> ConfigMapAccess<'config> { fn new_map(de: Deserializer<'config>) -> Result, ConfigError> { - let mut set = HashSet::new(); + let mut fields = Vec::new(); if let Some(mut v) = de.config.get_table(&de.key)? { // `v: Value>` for (key, _value) in v.val.drain() { - set.insert(KeyKind::CaseSensitive(key)); + fields.push(KeyKind::CaseSensitive(key)); } } if de.config.cli_unstable().advanced_env { // `CARGO_PROFILE_DEV_PACKAGE_` - let env_pattern = format!("{}_", de.key.as_env_key()); + let env_prefix = format!("{}_", de.key.as_env_key()); for env_key in de.config.env.keys() { - if env_key.starts_with(&env_pattern) { + if env_key.starts_with(&env_prefix) { // `CARGO_PROFILE_DEV_PACKAGE_bar_OPT_LEVEL = 3` - let rest = &env_key[env_pattern.len()..]; + let rest = &env_key[env_prefix.len()..]; // `rest = bar_OPT_LEVEL` let part = rest.splitn(2, '_').next().unwrap(); // `part = "bar"` - set.insert(KeyKind::CaseSensitive(part.to_string())); + fields.push(KeyKind::CaseSensitive(part.to_string())); } } } Ok(ConfigMapAccess { de, - set_iter: set.into_iter(), - next: None, + fields, + field_index: 0, }) } @@ -223,10 +232,10 @@ impl<'config> ConfigMapAccess<'config> { de: Deserializer<'config>, fields: &'static [&'static str], ) -> Result, ConfigError> { - let mut set = HashSet::new(); - for field in fields { - set.insert(KeyKind::Normal(field.to_string())); - } + let fields: Vec = fields + .iter() + .map(|field| KeyKind::Normal(field.to_string())) + .collect(); // 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 @@ -234,7 +243,10 @@ impl<'config> ConfigMapAccess<'config> { // fields and warn about them. if let Some(mut v) = de.config.get_table(&de.key)? { for (t_key, value) in v.val.drain() { - if set.contains(&KeyKind::Normal(t_key.to_string())) { + if fields.iter().any(|k| match k { + KeyKind::Normal(s) => s == &t_key, + KeyKind::CaseSensitive(s) => s == &t_key, + }) { continue; } de.config.shell().warn(format!( @@ -248,8 +260,8 @@ impl<'config> ConfigMapAccess<'config> { Ok(ConfigMapAccess { de, - set_iter: set.into_iter(), - next: None, + fields, + field_index: 0, }) } } @@ -261,30 +273,61 @@ impl<'de, 'config> de::MapAccess<'de> for ConfigMapAccess<'config> { where K: de::DeserializeSeed<'de>, { - match self.set_iter.next() { - Some(key) => { - 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); - result - } - None => Ok(None), + if self.field_index >= self.fields.len() { + return Ok(None); } + let field = match &self.fields[self.field_index] { + KeyKind::Normal(s) | KeyKind::CaseSensitive(s) => s.as_str(), + }; + seed.deserialize(field.into_deserializer()).map(Some) } fn next_value_seed(&mut self, seed: V) -> Result where V: de::DeserializeSeed<'de>, { - 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 field = &self.fields[self.field_index]; + self.field_index += 1; + // Set this as the current key in the deserializer. + let field = match field { + KeyKind::Normal(field) => { + self.de.key.push(&field); + field + } + KeyKind::CaseSensitive(field) => { + self.de.key.push_sensitive(&field); + field + } + }; + // Env vars that are a prefix of another with a dash/underscore cannot + // be supported by our serde implementation, so check for them here. + // Example: + // CARGO_BUILD_TARGET + // CARGO_BUILD_TARGET_DIR + // or + // CARGO_PROFILE_DEV_DEBUG + // CARGO_PROFILE_DEV_DEBUG_ASSERTIONS + // The `deserialize_option` method does not know the type of the field. + // If the type is an Option (like + // `profile.dev.build-override`), then it needs to check for env vars + // starting with CARGO_FOO_BAR_. This is a problem for keys like + // CARGO_BUILD_TARGET because checking for a prefix would incorrectly + // match CARGO_BUILD_TARGET_DIR. `deserialize_option` would have no + // choice but to call `visit_some()` which would then fail if + // CARGO_BUILD_TARGET isn't set. So we check for these prefixes and + // disallow them here. + let env_prefix = format!("{}_", field).replace('-', "_"); + let env_prefix_ok = !self.fields.iter().any(|field| { + let field = match field { + KeyKind::Normal(s) | KeyKind::CaseSensitive(s) => s.as_str(), + }; + field.replace('-', "_").starts_with(&env_prefix) + }); + let result = seed.deserialize(Deserializer { config: self.de.config, key: self.de.key.clone(), + env_prefix_ok, }); self.de.key.pop(); result diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index e74c86273e6..eddd19f445e 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -35,6 +35,12 @@ //! tables (because Cargo must fetch all of them), so those do not support //! environment variables. //! +//! Try to avoid keys that are a prefix of another with a dash/underscore. For +//! example `build.target` and `build.target-dir`. This is OK if these are not +//! structs/maps, but if it is a struct or map, then it will not be able to +//! read the environment variable due to ambiguity. (See `ConfigMapAccess` for +//! more details.) +//! //! ## Internal API //! //! Internally config values are stored with the `ConfigValue` type after they @@ -520,13 +526,16 @@ impl Config { } } - fn has_key(&self, key: &ConfigKey) -> bool { - if self.env.get(key.as_env_key()).is_some() { + fn has_key(&self, key: &ConfigKey, env_prefix_ok: bool) -> bool { + if self.env.contains_key(key.as_env_key()) { return true; } - let env_pattern = format!("{}_", key.as_env_key()); - if self.env.keys().any(|k| k.starts_with(&env_pattern)) { - return true; + // See ConfigMapAccess for a description of this. + if env_prefix_ok { + let env_prefix = format!("{}_", key.as_env_key()); + if self.env.keys().any(|k| k.starts_with(&env_prefix)) { + return true; + } } if let Ok(o_cv) = self.get_cv(key) { if o_cv.is_some() { @@ -1101,6 +1110,7 @@ impl Config { let d = Deserializer { config: self, key: ConfigKey::from_str(key), + env_prefix_ok: true, }; T::deserialize(d).map_err(|e| e.into()) } diff --git a/src/cargo/util/config/value.rs b/src/cargo/util/config/value.rs index 09742d37369..a424ea4d290 100644 --- a/src/cargo/util/config/value.rs +++ b/src/cargo/util/config/value.rs @@ -51,10 +51,14 @@ 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]; +/// Location where a config value is defined. #[derive(Clone, Debug, Eq)] pub enum Definition { + /// Defined in a `.cargo/config`, includes the path to the file. Path(PathBuf), + /// Defined in an environment variable, includes the environment key. Environment(String), + /// Passed in on the command line. Cli, } diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 2459855b6e6..292b81a8590 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -2806,6 +2806,11 @@ fn custom_target_dir_env() { assert!(p.root().join("foo/target/debug").join(&exe_name).is_file()); assert!(p.root().join("target/debug").join(&exe_name).is_file()); + p.cargo("build") + .env("CARGO_BUILD_TARGET_DIR", "foo2/target") + .run(); + assert!(p.root().join("foo2/target/debug").join(&exe_name).is_file()); + fs::create_dir(p.root().join(".cargo")).unwrap(); File::create(p.root().join(".cargo/config")) .unwrap() diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index c618c31f5c2..ccdf2935674 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -424,6 +424,32 @@ lto = false ); } +#[cargo_test] +fn profile_env_var_prefix() { + // Check for a bug with collision on DEBUG vs DEBUG_ASSERTIONS. + let config = ConfigBuilder::new() + .env("CARGO_PROFILE_DEV_DEBUG_ASSERTIONS", "false") + .build(); + let p: toml::TomlProfile = config.get("profile.dev").unwrap(); + assert_eq!(p.debug_assertions, Some(false)); + assert_eq!(p.debug, None); + + let config = ConfigBuilder::new() + .env("CARGO_PROFILE_DEV_DEBUG", "1") + .build(); + let p: toml::TomlProfile = config.get("profile.dev").unwrap(); + assert_eq!(p.debug_assertions, None); + assert_eq!(p.debug, Some(toml::U32OrBool::U32(1))); + + let config = ConfigBuilder::new() + .env("CARGO_PROFILE_DEV_DEBUG_ASSERTIONS", "false") + .env("CARGO_PROFILE_DEV_DEBUG", "1") + .build(); + let p: toml::TomlProfile = config.get("profile.dev").unwrap(); + assert_eq!(p.debug_assertions, Some(false)); + assert_eq!(p.debug, Some(toml::U32OrBool::U32(1))); +} + #[cargo_test] fn config_deserialize_any() { // Some tests to exercise deserialize_any for deserializers that need to @@ -1103,3 +1129,53 @@ Caused by: expected string but found integer in list", ); } + +#[cargo_test] +fn struct_with_opt_inner_struct() { + // Struct with a key that is Option of another struct. + // Check that can be defined with environment variable. + #[derive(Deserialize)] + struct Inner { + value: Option, + } + #[derive(Deserialize)] + struct Foo { + inner: Option, + } + let config = ConfigBuilder::new() + .env("CARGO_FOO_INNER_VALUE", "12") + .build(); + let f: Foo = config.get("foo").unwrap(); + assert_eq!(f.inner.unwrap().value.unwrap(), 12); +} + +#[cargo_test] +fn overlapping_env_config() { + // Issue where one key is a prefix of another. + #[derive(Deserialize)] + #[serde(rename_all = "kebab-case")] + struct Ambig { + debug: Option, + debug_assertions: Option, + } + let config = ConfigBuilder::new() + .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true") + .build(); + + let s: Ambig = config.get("ambig").unwrap(); + assert_eq!(s.debug_assertions, Some(true)); + assert_eq!(s.debug, None); + + let config = ConfigBuilder::new().env("CARGO_AMBIG_DEBUG", "0").build(); + let s: Ambig = config.get("ambig").unwrap(); + assert_eq!(s.debug_assertions, None); + assert_eq!(s.debug, Some(0)); + + let config = ConfigBuilder::new() + .env("CARGO_AMBIG_DEBUG", "1") + .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true") + .build(); + let s: Ambig = config.get("ambig").unwrap(); + assert_eq!(s.debug_assertions, Some(true)); + assert_eq!(s.debug, Some(1)); +}