From b96aecfda047f910419e35e244491d85108a54b5 Mon Sep 17 00:00:00 2001 From: Daniel Goldman Date: Sun, 14 Jul 2024 22:17:47 -0400 Subject: [PATCH] Fix options parser discrepancy due to repeated evaluation of `.pants.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 --- .../pants/help/help_info_extracter_test.py | 1 + src/python/pants/option/options.py | 7 +++++- .../pants/option/options_bootstrapper.py | 2 ++ src/python/pants/option/options_test.py | 3 +++ src/rust/engine/options/src/lib.rs | 7 +++++- src/rust/engine/options/src/tests.rs | 22 +++++++++++++++++++ 6 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/python/pants/help/help_info_extracter_test.py b/src/python/pants/help/help_info_extracter_test.py index ff248b44a8c..382a377d814 100644 --- a/src/python/pants/help/help_info_extracter_test.py +++ b/src/python/pants/help/help_info_extracter_test.py @@ -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, diff --git a/src/python/pants/option/options.py b/src/python/pants/option/options.py index 33a73af7fd0..81f31f7ab51 100644 --- a/src/python/pants/option/options.py +++ b/src/python/pants/option/options.py @@ -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. @@ -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. @@ -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, diff --git a/src/python/pants/option/options_bootstrapper.py b/src/python/pants/option/options_bootstrapper.py index 988e6b3b1e8..e4257d1547f 100644 --- a/src/python/pants/option/options_bootstrapper.py +++ b/src/python/pants/option/options_bootstrapper.py @@ -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): @@ -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() diff --git a/src/python/pants/option/options_test.py b/src/python/pants/option/options_test.py index 1b98c457e62..72c28074435 100644 --- a/src/python/pants/option/options_test.py +++ b/src/python/pants/option/options_test.py @@ -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) @@ -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, @@ -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", diff --git a/src/rust/engine/options/src/lib.rs b/src/rust/engine/options/src/lib.rs index 0b5abf290c6..3f30319bb30 100644 --- a/src/rust/engine/options/src/lib.rs +++ b/src/rust/engine/options/src/lib.rs @@ -281,6 +281,8 @@ impl OptionParser { include_derivation: bool, buildroot: Option, ) -> Result { + 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); @@ -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"), diff --git a/src/rust/engine/options/src/tests.rs b/src/rust/engine/options/src/tests.rs index 1713046bad7..4ab35d68ed1 100644 --- a/src/rust/engine/options/src/tests.rs +++ b/src/rust/engine/options/src/tests.rs @@ -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; @@ -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() + ) +}