Skip to content

Commit

Permalink
Support parsing Config from memory in Rust. (#20875)
Browse files Browse the repository at this point in the history
Previously it required actual files on disk.

This change also includes a minor modification
to the equivalent logic in the Python parser, to
prepare for connecting the two: The _ConfigValues
struct now remembers the ConfigSource it was
parsed from, instead of just the path.
  • Loading branch information
benjyw authored May 6, 2024
1 parent 3de26e5 commit 1266495
Show file tree
Hide file tree
Showing 7 changed files with 81 additions and 51 deletions.
14 changes: 9 additions & 5 deletions src/python/pants/option/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def _parse_toml(
**normalized_seed_values,
**toml_values.get(DEFAULT_SECTION, {}),
}
return _ConfigValues(config_source.path, toml_values, seed_values)
return _ConfigValues(config_source, toml_values, seed_values)

def verify(self, section_to_valid_options: dict[str, set[str]]):
error_log = []
Expand Down Expand Up @@ -155,9 +155,9 @@ def get(self, section, option) -> list[str]:
available_vals.append(val)
return available_vals

def sources(self) -> list[str]:
"""Returns the sources of this config as a list of filenames."""
return [vals.path for vals in self.values]
def sources(self) -> list[ConfigSource]:
"""Returns the sources of this config."""
return [vals.source for vals in self.values]

def get_sources_for_option(self, section: str, option: str) -> list[str]:
"""Returns the path(s) to the source file(s) the given option was defined in."""
Expand All @@ -176,10 +176,14 @@ def get_sources_for_option(self, section: str, option: str) -> list[str]:
class _ConfigValues:
"""The parsed contents of a TOML config file."""

path: str
source: ConfigSource
section_to_values: dict[str, dict[str, Any]]
seed_values: dict[str, Any]

@property
def path(self) -> str:
return self.source.path

def _possibly_interpolate_value(
self,
raw_value: str,
Expand Down
18 changes: 9 additions & 9 deletions src/python/pants/option/config_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,14 +254,13 @@ def _compare(config: Config, expected: dict[str, dict[str, list[str]]]) -> None:

@pytest.mark.parametrize("filedata", [FILE_0, FILE_1])
def test_individual_file_parsing(filedata: ConfigFile) -> None:
file_contents = [FileContent("file.toml", filedata.content.encode())]
config = Config.load(
file_contents=[
FileContent("file.toml", filedata.content.encode()),
],
file_contents=file_contents,
seed_values=_seed_values,
env=_env,
)
assert ["file.toml"] == config.sources()
assert file_contents == config.sources()

_compare(config, filedata.expected_options)

Expand All @@ -273,15 +272,16 @@ def test_individual_file_parsing(filedata: ConfigFile) -> None:


def test_merged_config() -> None:
file_contents = [
FileContent("file0.toml", FILE_0.content.encode()),
FileContent("file1.toml", FILE_1.content.encode()),
]
config = Config.load(
file_contents=[
FileContent("file0.toml", FILE_0.content.encode()),
FileContent("file1.toml", FILE_1.content.encode()),
],
file_contents=file_contents,
seed_values=_seed_values,
env=_env,
)
assert ["file0.toml", "file1.toml"] == config.sources()
assert file_contents == config.sources()

_compare(config, _expected_combined_values)

Expand Down
9 changes: 4 additions & 5 deletions src/python/pants/option/options_bootstrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,18 +146,17 @@ def filecontent_for(path: str) -> FileContent:

# Now re-read the config, post-bootstrapping. Note the order: First whatever we bootstrapped
# from (typically pants.toml), then config override, then rcfiles.
full_config_paths = pre_bootstrap_config.sources()
full_config_sources = pre_bootstrap_config.sources()
if allow_pantsrc and bootstrap_option_values.pantsrc:
rcfiles = [
os.path.expanduser(str(rcfile))
for rcfile in bootstrap_option_values.pantsrc_files
]
existing_rcfiles = list(filter(os.path.exists, rcfiles))
full_config_paths.extend(existing_rcfiles)
existing_rcfiles = [filecontent_for(p) for p in filter(os.path.exists, rcfiles)]
full_config_sources.extend(existing_rcfiles)

full_config_files_products = [filecontent_for(p) for p in full_config_paths]
post_bootstrap_config = Config.load(
full_config_files_products,
full_config_sources,
seed_values=bootstrap_option_values.as_dict(),
env=env,
)
Expand Down
45 changes: 30 additions & 15 deletions src/rust/engine/options/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use std::collections::{HashMap, HashSet};
use std::fs;
use std::path::Path;
use std::path::{Path, PathBuf};

use lazy_static::lazy_static;
use regex::Regex;
Expand Down Expand Up @@ -229,27 +229,42 @@ fn toml_table_to_dict(table: &Value) -> HashMap<String, Val> {
}
}

#[derive(Clone, Debug)]
pub struct ConfigSource {
pub path: PathBuf,
pub content: String,
}

impl ConfigSource {
pub fn from_file<P: AsRef<Path>>(path: P) -> Result<ConfigSource, String> {
let content = fs::read_to_string(&path).map_err(|e| {
format!(
"Failed to read config file {}: {}",
path.as_ref().display(),
e
)
})?;
Ok(ConfigSource {
path: path.as_ref().to_path_buf(),
content,
})
}
}

#[derive(Clone)]
pub(crate) struct Config {
value: Value,
}

impl Config {
pub(crate) fn parse<P: AsRef<Path>>(
file: P,
pub(crate) fn parse(
config_source: &ConfigSource,
seed_values: &InterpolationMap,
) -> Result<Config, String> {
let config_contents = fs::read_to_string(&file).map_err(|e| {
format!(
"Failed to read config file {}: {}",
file.as_ref().display(),
e
)
})?;
let config = config_contents.parse::<Value>().map_err(|e| {
let config = config_source.content.parse::<Value>().map_err(|e| {
format!(
"Failed to parse config file {}: {}",
file.as_ref().display(),
config_source.path.display(),
e
)
})?;
Expand Down Expand Up @@ -281,7 +296,7 @@ impl Config {
return Err(format!(
"Expected the config file {} to contain tables per section, \
but section {} contained a {}: {}",
file.as_ref().display(),
config_source.path.display(),
section_name,
section.type_str(),
section
Expand All @@ -297,7 +312,7 @@ impl Config {
format!(
"{} in config file {}, section {}, key {}",
e.msg,
file.as_ref().display(),
config_source.path.display(),
section_name,
e.key
)
Expand All @@ -308,7 +323,7 @@ impl Config {

_ => Err(format!(
"Expected the config file {} to contain a table but contained a {}: {}",
file.as_ref().display(),
config_source.path.display(),
config.type_str(),
config
)),
Expand Down
4 changes: 2 additions & 2 deletions src/rust/engine/options/src/config_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::fmt::Debug;
use std::fs::File;
use std::io::Write;

use crate::config::interpolate_string;
use crate::config::{interpolate_string, ConfigSource};
use crate::{
option_id, DictEdit, DictEditAction, ListEdit, ListEditAction, OptionId, OptionsSource, Val,
};
Expand All @@ -26,7 +26,7 @@ fn maybe_config(file_content: &str) -> Result<ConfigReader, String> {
.write_all(file_content.as_bytes())
.unwrap();
Config::parse(
&path,
&ConfigSource::from_file(&path)?,
&HashMap::from([
("seed1".to_string(), "seed1val".to_string()),
("seed2".to_string(), "seed2val".to_string()),
Expand Down
31 changes: 20 additions & 11 deletions src/rust/engine/options/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ use serde::Deserialize;

pub use self::args::Args;
use self::args::ArgsReader;
pub use self::config::ConfigSource;
use self::config::{Config, ConfigReader};
pub use self::env::Env;
use self::env::EnvReader;
Expand Down Expand Up @@ -245,12 +246,12 @@ pub struct OptionParser {
}

impl OptionParser {
// If config_paths is None, we'll do config file discovery. Otherwise we'll use the provided paths.
// The latter case is useful for tests.
// If config_sources is None, we'll do config file discovery. Otherwise we'll use the
// provided sources. The latter case is useful for tests.
pub fn new(
args: Args,
env: Env,
config_paths: Option<Vec<&str>>,
config_sources: Option<Vec<ConfigSource>>,
allow_pantsrc: bool,
include_derivation: bool,
buildroot: Option<BuildRoot>,
Expand Down Expand Up @@ -296,16 +297,20 @@ impl OptionParser {
.to_string()
}

let repo_config_files = match config_paths {
Some(paths) => paths.iter().map(|s| s.to_string()).collect(),
let config_sources = match config_sources {
Some(cs) => cs,
None => {
let default_config_path = path_join(&buildroot_string, "pants.toml");
parser
let config_paths = parser
.parse_string_list(
&option_id!("pants", "config", "files"),
&[&default_config_path],
)?
.value
.value;
config_paths
.iter()
.map(|cp| ConfigSource::from_file(Path::new(&cp)))
.collect::<Result<Vec<_>, _>>()?
}
};

Expand All @@ -328,12 +333,15 @@ impl OptionParser {
]);

let mut ordinal: usize = 0;
for path in repo_config_files.iter() {
let config = Config::parse(path, &seed_values)?;
for config_source in config_sources {
let config = Config::parse(&config_source, &seed_values)?;
sources.insert(
Source::Config {
ordinal,
path: path_strip(&buildroot_string, path),
path: path_strip(
&buildroot_string,
config_source.path.to_string_lossy().as_ref(),
),
},
Rc::new(ConfigReader::new(config, fromfile_expander.clone())),
);
Expand All @@ -358,7 +366,8 @@ 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(&ConfigSource::from_file(rcfile_path)?, &seed_values)?;
sources.insert(
Source::Config {
ordinal,
Expand Down
11 changes: 7 additions & 4 deletions src/rust/engine/options/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright 2024 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

use crate::config::ConfigSource;
use crate::{
option_id, Args, BuildRoot, DictEdit, DictEditAction, Env, ListEdit, ListEditAction,
OptionParser, Source, Val,
Expand Down Expand Up @@ -60,10 +61,12 @@ fn with_setup(
.map(|(k, v)| (k.to_owned(), v.to_owned()))
.collect::<HashMap<_, _>>(),
},
Some(vec![
config_path.to_str().unwrap(),
extra_config_path.to_str().unwrap(),
]),
Some(
vec![config_path, extra_config_path]
.iter()
.map(|cp| ConfigSource::from_file(cp).unwrap())
.collect(),
),
false,
true,
Some(BuildRoot::find_from(buildroot.path()).unwrap()),
Expand Down

0 comments on commit 1266495

Please sign in to comment.