diff --git a/src/rust/engine/options/src/args.rs b/src/rust/engine/options/src/args.rs index 47d9fee7ff1..e9c13f173af 100644 --- a/src/rust/engine/options/src/args.rs +++ b/src/rust/engine/options/src/args.rs @@ -143,9 +143,11 @@ impl OptionsSource for Args { self.get_list(id, parse_string_list) } - fn get_dict(&self, id: &OptionId) -> Result, String> { + fn get_dict(&self, id: &OptionId) -> Result>, String> { match self.find_flag(Self::arg_names(id, Negate::False))? { - Some((name, ref value, _)) => parse_dict(value).map(Some).map_err(|e| e.render(name)), + Some((name, ref value, _)) => parse_dict(value) + .map(|e| Some(vec![e])) + .map_err(|e| e.render(name)), None => Ok(None), } } diff --git a/src/rust/engine/options/src/config.rs b/src/rust/engine/options/src/config.rs index 702910f95f6..ef3d71794f1 100644 --- a/src/rust/engine/options/src/config.rs +++ b/src/rust/engine/options/src/config.rs @@ -2,8 +2,8 @@ // Licensed under the Apache License, Version 2.0 (see LICENSE). use std::collections::{HashMap, HashSet}; +use std::ffi::OsString; use std::fs; -use std::mem; use std::path::Path; use lazy_static::lazy_static; @@ -217,21 +217,132 @@ fn toml_table_to_dict(table: &Value) -> HashMap { } #[derive(Clone)] -pub(crate) struct Config { +struct ConfigSource { + #[allow(dead_code)] + path: Option, config: Value, } +impl ConfigSource { + fn option_name(id: &OptionId) -> String { + id.name("_", NameTransform::None) + } + + fn get_value(&self, id: &OptionId) -> Option<&Value> { + self.config + .get(id.scope()) + .and_then(|table| table.get(Self::option_name(id))) + } + + fn get_list( + &self, + id: &OptionId, + parse_list: fn(&str) -> Result>, ParseError>, + ) -> Result>, String> { + let mut list_edits = vec![]; + if let Some(table) = self.config.get(id.scope()) { + let option_name = Self::option_name(id); + if let Some(value) = table.get(&option_name) { + match value { + Value::Table(sub_table) => { + if sub_table.is_empty() + || !sub_table.keys().collect::>().is_subset( + &["add".to_owned(), "remove".to_owned()] + .iter() + .collect::>(), + ) + { + return Err(format!( + "Expected {option_name} to contain an 'add' element, a 'remove' element or both but found: {sub_table:?}" + )); + } + if let Some(add) = sub_table.get("add") { + list_edits.push(ListEdit { + action: ListEditAction::Add, + items: T::extract_list(&format!("{option_name}.add"), add)?, + }) + } + if let Some(remove) = sub_table.get("remove") { + list_edits.push(ListEdit { + action: ListEditAction::Remove, + items: T::extract_list(&format!("{option_name}.remove"), remove)?, + }) + } + } + Value::String(v) => { + list_edits.extend(parse_list(v).map_err(|e| e.render(option_name))?); + } + value => list_edits.push(ListEdit { + action: ListEditAction::Replace, + items: T::extract_list(&option_name, value)?, + }), + } + } + } + Ok(list_edits) + } + + fn get_dict(&self, id: &OptionId) -> Result, String> { + if let Some(table) = self.config.get(id.scope()) { + let option_name = Self::option_name(id); + if let Some(value) = table.get(&option_name) { + match value { + Value::Table(sub_table) => { + if let Some(add) = sub_table.get("add") { + if sub_table.len() == 1 && add.is_table() { + return Ok(Some(DictEdit { + action: DictEditAction::Add, + items: toml_table_to_dict(add), + })); + } + } + return Ok(Some(DictEdit { + action: DictEditAction::Replace, + items: toml_table_to_dict(value), + })); + } + Value::String(v) => { + return Ok(Some(parse_dict(v).map_err(|e| e.render(option_name))?)); + } + _ => { + return Err(format!( + "Expected {option_name} to be a toml table or Python dict, but given {value}." + )); + } + } + } + } + Ok(None) + } +} + +#[derive(Clone)] +pub(crate) struct Config { + sources: Vec, +} + impl Config { - pub(crate) fn default() -> Config { + pub(crate) fn parse>( + files: &[P], + seed_values: &InterpolationMap, + ) -> Result { + let mut sources = vec![]; + for file in files { + sources.push(Self::parse_source(file, seed_values)?); + } + Ok(Config { sources }) + } + + pub(crate) fn merge(self, other: Config) -> Config { Config { - config: Value::Table(Table::new()), + sources: self.sources.into_iter().chain(other.sources).collect(), } } - pub(crate) fn parse>( + fn parse_source>( file: P, seed_values: &InterpolationMap, - ) -> Result { + ) -> Result { let config_contents = fs::read_to_string(&file).map_err(|e| { format!( "Failed to read config file {}: {}", @@ -308,31 +419,17 @@ impl Config { }; let new_table = Table::from_iter(new_sections?); - Ok(Config { + Ok(ConfigSource { + path: Some(file.as_ref().as_os_str().into()), config: Value::Table(new_table), }) } - pub(crate) fn merged>( - files: &[P], - seed_values: &InterpolationMap, - ) -> Result { - files - .iter() - .map(|f| Config::parse(f, seed_values)) - .try_fold(Config::default(), |config, parse_result| { - parse_result.map(|parsed| config.merge(parsed)) - }) - } - - fn option_name(id: &OptionId) -> String { - id.name("_", NameTransform::None) - } - fn get_value(&self, id: &OptionId) -> Option<&Value> { - self.config - .get(id.scope()) - .and_then(|table| table.get(Self::option_name(id))) + self.sources + .iter() + .rev() + .find_map(|source| source.get_value(id)) } fn get_list( @@ -340,69 +437,11 @@ impl Config { id: &OptionId, parse_list: fn(&str) -> Result>, ParseError>, ) -> Result>>, String> { - if let Some(table) = self.config.get(id.scope()) { - let option_name = Self::option_name(id); - let mut list_edits = vec![]; - if let Some(value) = table.get(&option_name) { - match value { - Value::Table(sub_table) => { - if sub_table.is_empty() - || !sub_table.keys().collect::>().is_subset( - &["add".to_owned(), "remove".to_owned()] - .iter() - .collect::>(), - ) - { - return Err(format!( - "Expected {option_name} to contain an 'add' element, a 'remove' element or both but found: {sub_table:?}" - )); - } - if let Some(add) = sub_table.get("add") { - list_edits.push(ListEdit { - action: ListEditAction::Add, - items: T::extract_list(&format!("{option_name}.add"), add)?, - }) - } - if let Some(remove) = sub_table.get("remove") { - list_edits.push(ListEdit { - action: ListEditAction::Remove, - items: T::extract_list(&format!("{option_name}.remove"), remove)?, - }) - } - } - Value::String(v) => { - list_edits.extend(parse_list(v).map_err(|e| e.render(option_name))?); - } - value => list_edits.push(ListEdit { - action: ListEditAction::Replace, - items: T::extract_list(&option_name, value)?, - }), - } - } - if !list_edits.is_empty() { - return Ok(Some(list_edits)); - } - } - Ok(None) - } - - pub(crate) fn merge(mut self, mut other: Config) -> Config { - let mut map = mem::take(self.config.as_table_mut().unwrap()); - let mut other = mem::take(other.config.as_table_mut().unwrap()); - // Merge overlapping sections. - for (scope, table) in &mut map { - if let Some(mut other_table) = other.remove(scope) { - table - .as_table_mut() - .unwrap() - .extend(mem::take(other_table.as_table_mut().unwrap())); - } - } - // And then extend non-overlapping sections. - map.extend(other); - Config { - config: Value::Table(map), + let mut edits: Vec> = vec![]; + for source in self.sources.iter() { + edits.append(&mut source.get_list(id, parse_list)?); } + Ok(Some(edits)) } } @@ -443,36 +482,13 @@ impl OptionsSource for Config { self.get_list(id, parse_string_list) } - fn get_dict(&self, id: &OptionId) -> Result, String> { - if let Some(table) = self.config.get(id.scope()) { - let option_name = Self::option_name(id); - if let Some(value) = table.get(&option_name) { - match value { - Value::Table(sub_table) => { - if let Some(add) = sub_table.get("add") { - if sub_table.len() == 1 && add.is_table() { - return Ok(Some(DictEdit { - action: DictEditAction::Add, - items: toml_table_to_dict(add), - })); - } - } - return Ok(Some(DictEdit { - action: DictEditAction::Replace, - items: toml_table_to_dict(value), - })); - } - Value::String(v) => { - return Ok(Some(parse_dict(v).map_err(|e| e.render(option_name))?)); - } - _ => { - return Err(format!( - "Expected {option_name} to be a toml table or Python dict, but given {value}." - )); - } - } + fn get_dict(&self, id: &OptionId) -> Result>, String> { + let mut edits = vec![]; + for source in self.sources.iter() { + if let Some(edit) = source.get_dict(id)? { + edits.push(edit); } } - Ok(None) + Ok(if edits.is_empty() { None } else { Some(edits) }) } } diff --git a/src/rust/engine/options/src/config_tests.rs b/src/rust/engine/options/src/config_tests.rs index 8fd9dbe504a..eb0d2b4072e 100644 --- a/src/rust/engine/options/src/config_tests.rs +++ b/src/rust/engine/options/src/config_tests.rs @@ -25,7 +25,7 @@ fn maybe_config>(file_contents: I) -> Resul path }) .collect::>(); - Config::merged( + Config::parse( &files, &HashMap::from([ ("seed1".to_string(), "seed1val".to_string()), @@ -56,24 +56,46 @@ fn test_display() { } #[test] -fn test_section_overlap() { - // Two files with the same section should result in merged content for that section. +fn test_multiple_sources() { let config = config([ "[section]\n\ - field1 = 'something'\n", + name = 'first'\n\ + field1 = 'something'\n\ + list='+[0,1]'", "[section]\n\ - field2 = 'something else'\n", + name = 'second'\n\ + field2 = 'something else'\n\ + list='-[0],+[2, 3]'", ]); - let assert_string = |expected: &str, id: OptionId| { - assert_eq!( - expected.to_owned(), - config.get_string(&id).unwrap().unwrap() - ) + let assert_string = |expected: &str, id: &OptionId| { + assert_eq!(expected.to_owned(), config.get_string(id).unwrap().unwrap()) }; - assert_string("something", option_id!(["section"], "field1")); - assert_string("something else", option_id!(["section"], "field2")); + assert_string("second", &option_id!(["section"], "name")); + assert_string("something", &option_id!(["section"], "field1")); + assert_string("something else", &option_id!(["section"], "field2")); + + assert_eq!( + vec![ + ListEdit { + action: ListEditAction::Add, + items: vec![0, 1] + }, + ListEdit { + action: ListEditAction::Remove, + items: vec![0] + }, + ListEdit { + action: ListEditAction::Add, + items: vec![2, 3] + }, + ], + config + .get_int_list(&option_id!(["section"], "list")) + .unwrap() + .unwrap() + ) } #[test] diff --git a/src/rust/engine/options/src/env.rs b/src/rust/engine/options/src/env.rs index 0e817db751b..5ad96f9e1d4 100644 --- a/src/rust/engine/options/src/env.rs +++ b/src/rust/engine/options/src/env.rs @@ -135,10 +135,10 @@ impl OptionsSource for Env { self.get_list(id, parse_string_list) } - fn get_dict(&self, id: &OptionId) -> Result, String> { + fn get_dict(&self, id: &OptionId) -> Result>, String> { if let Some(value) = self.get_string(id)? { parse_dict(&value) - .map(Some) + .map(|e| Some(vec![e])) .map_err(|e| e.render(self.display(id))) } else { Ok(None) diff --git a/src/rust/engine/options/src/lib.rs b/src/rust/engine/options/src/lib.rs index 37641c34f7b..ea9fce6bde2 100644 --- a/src/rust/engine/options/src/lib.rs +++ b/src/rust/engine/options/src/lib.rs @@ -178,7 +178,11 @@ pub(crate) trait OptionsSource { /// fn get_string_list(&self, id: &OptionId) -> Result>>, String>; - fn get_dict(&self, id: &OptionId) -> Result, String>; + /// + /// Get the dict option identified by `id` from this source. + /// Errors when this source has an option value for `id` but that value is not a dict. + /// + fn get_dict(&self, id: &OptionId) -> Result>, String>; } #[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] @@ -260,7 +264,7 @@ impl OptionParser { ("pants_distdir".to_string(), subdir("distdir", "dist")?), ]); - let mut config = Config::merged(&repo_config_files, &seed_values)?; + let mut config = Config::parse(&repo_config_files, &seed_values)?; sources.insert(Source::Config, Rc::new(config.clone())); parser = OptionParser { sources: sources.clone(), @@ -277,7 +281,7 @@ impl OptionParser { )? { let rcfile_path = Path::new(&rcfile); if rcfile_path.exists() { - let rc_config = Config::parse(rcfile_path, &seed_values)?; + let rc_config = Config::parse(&[rcfile_path], &seed_values)?; config = config.merge(rc_config); } }