Skip to content

Commit

Permalink
Fix options parser discrepancy due to repeated evaluation of `.pants.…
Browse files Browse the repository at this point in the history
…rc` (#21118)

Allows config discovery in rust by not passing discovered config to
rust-based options parser.
(The exception is in bootstrapping, which is necessary to make
integration tests work)

fixes #21091
  • Loading branch information
lilatomic authored Jul 15, 2024
1 parent 7b2dcad commit b96aecf
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 2 deletions.
1 change: 1 addition & 0 deletions src/python/pants/help/help_info_extracter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ class BazLibrary(Target):
options = Options.create(
env={},
config=Config.load([]),
native_options_config_discovery=False,
known_scope_infos=[Global.get_scope_info(), Foo.get_scope_info(), Bar.get_scope_info()],
args=["./pants", "--backend-packages=['internal_plugins.releases']"],
bootstrap_option_values=None,
Expand Down
7 changes: 6 additions & 1 deletion src/python/pants/option/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ def create(
# We default to error to be strict in tests, but set explicitly in OptionsBootstrapper
# for user-facing behavior.
native_options_validation: NativeOptionsValidation = NativeOptionsValidation.error,
native_options_config_discovery: bool = True,
) -> Options:
"""Create an Options instance.
Expand All @@ -131,6 +132,7 @@ def create(
:param allow_unknown_options: Whether to ignore or error on unknown cmd-line flags.
:param native_options_validation: How to validate the native options parser against the
legacy Python parser.
:param native_options_config_discovery: Whether to discover config files in the native parser or use the ones supplied
"""
# We need parsers for all the intermediate scopes, so inherited option values
# can propagate through them.
Expand Down Expand Up @@ -163,7 +165,10 @@ def create(
parser_by_scope = {si.scope: Parser(env, config, si) for si in complete_known_scope_infos}
known_scope_to_info = {s.scope: s for s in complete_known_scope_infos}

native_parser = NativeOptionParser(args, env, config.sources(), allow_pantsrc=True)
config_to_pass = None if native_options_config_discovery else config.sources()
native_parser = NativeOptionParser(
args, env, config_sources=config_to_pass, allow_pantsrc=True
)

return cls(
builtin_goal=split_args.builtin_goal,
Expand Down
2 changes: 2 additions & 0 deletions src/python/pants/option/options_bootstrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def parse_bootstrap_options(
# We ignore validation to ensure bootstrapping succeeds.
# The bootstrap options will be validated anyway when we parse the full options.
native_options_validation=NativeOptionsValidation.ignore,
native_options_config_discovery=False,
)

for options_info in collect_options_info(BootstrapOptions):
Expand Down Expand Up @@ -259,6 +260,7 @@ def _full_options(
bootstrap_option_values=bootstrap_option_values,
allow_unknown_options=allow_unknown_options,
native_options_validation=bootstrap_option_values.native_options_validation,
native_options_config_discovery=False,
)

distinct_subsystem_classes: set[type[Subsystem]] = set()
Expand Down
3 changes: 3 additions & 0 deletions src/python/pants/option/options_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ def create_options(
env=env or {},
config=Config.load([FileContent("pants.toml", toml.dumps(config or {}).encode())]),
known_scope_infos=[*(ScopeInfo(scope) for scope in scopes), *(extra_scope_infos or ())],
native_options_config_discovery=False,
args=["./pants", *(args or ())],
)
register_fn(options)
Expand Down Expand Up @@ -487,6 +488,7 @@ def _parse(
options = Options.create(
env=env or {},
config=_create_config(config, config2),
native_options_config_discovery=False,
known_scope_infos=_known_scope_infos,
args=args,
bootstrap_option_values=bootstrap_option_values,
Expand Down Expand Up @@ -1122,6 +1124,7 @@ def test_passthru_args_not_interpreted():
config=_create_config(
{"consumer": {"shlexed": ["from config"], "string": ["from config"]}}
),
native_options_config_discovery=False,
known_scope_infos=[global_scope(), task("test"), subsystem("consumer")],
args=[
"./pants",
Expand Down
7 changes: 6 additions & 1 deletion src/rust/engine/options/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,8 @@ impl OptionParser {
include_derivation: bool,
buildroot: Option<BuildRoot>,
) -> Result<OptionParser, String> {
let has_provided_configs = config_sources.is_some();

let buildroot = buildroot.unwrap_or(BuildRoot::find()?);
let buildroot_string = buildroot.convert_to_string()?;
let fromfile_expander = FromfileExpander::relative_to(buildroot);
Expand Down Expand Up @@ -379,7 +381,10 @@ impl OptionParser {
passthrough_args: None,
};

if allow_pantsrc && parser.parse_bool(&option_id!("pantsrc"), true)?.value {
if allow_pantsrc
&& parser.parse_bool(&option_id!("pantsrc"), true)?.value
&& !has_provided_configs
{
for rcfile in parser
.parse_string_list(
&option_id!("pantsrc", "files"),
Expand Down
22 changes: 22 additions & 0 deletions src/rust/engine/options/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::{
option_id, Args, BuildRoot, DictEdit, DictEditAction, Env, ListEdit, ListEditAction,
OptionParser, Source, Val,
};
use itertools::Itertools;
use maplit::hashmap;
use std::collections::HashMap;
use std::fs::File;
Expand Down Expand Up @@ -555,3 +556,24 @@ fn test_parse_dict_options() {
"",
);
}

#[test]
fn test_do_not_load_pantsrc_if_configs_passed() {
fn mk_args() -> Args {
Args::new(vec![])
}
fn mk_env() -> Env {
Env {
env: HashMap::new(),
}
}

let load_0 = OptionParser::new(mk_args(), mk_env(), Some(vec![]), true, true, None);

let found_sources = load_0.unwrap().sources;
println!("{:?}", found_sources.keys());
assert_eq!(
vec![Source::Env, Source::Flag],
found_sources.keys().cloned().collect_vec()
)
}

0 comments on commit b96aecf

Please sign in to comment.