Simplify OptionsBootstrapper #21784
Simplify OptionsBootstrapper #21784
Conversation
benjyw
force-pushed
the
bootstrap_options
branch
2 times, most recently
from
9d27c4c to
edd5756
Compare
benjyw
force-pushed
the
bootstrap_options
branch
from
edd5756 to
b9e769f
Compare
|
Reviewers: Thanks for looking at this. I have added comments in GH to make the review easier to stomach. |
benjyw
force-pushed
the
bootstrap_options
branch
from
b9e769f to
b1ed362
Compare
| args, | ||
| Config(tuple()), | ||
| args=args, | ||
| env=FrozenDict(), |
There was a problem hiding this comment.
The order of fields was changed to be consistently "args then env", as it is elsewhere. Note that the type of env was changed be a mapping instead of the less obvious list of pairs.
| @@ -12,7 +12,7 @@ | |||
| from pants.base.build_environment import get_buildroot | |||
| from pants.base.deprecated import warn_or_error | |||
| from pants.option.arg_splitter import ArgSplitter | |||
| from pants.option.config import Config | |||
| from pants.option.config import ConfigSource | |||
There was a problem hiding this comment.
We no longer parse config in Python, so we only traffic in ConfigSource, which gets passed to Rust.
A followup change will delete the Python Config class and its tests, but this change was already gnarly enough without adding that in.
| env: Mapping[str, str], | ||
| config: Config, | ||
| known_scope_infos: Iterable[ScopeInfo], | ||
| *, |
There was a problem hiding this comment.
To force callsites to be readable...
| env: Mapping[str, str], | ||
| config_sources: Sequence[ConfigSource] | None, | ||
| known_scope_infos: Sequence[ScopeInfo], | ||
| extra_specs: Sequence[str] = tuple(), |
There was a problem hiding this comment.
Instead of the Options class knowing about bootstrap options and finding the extra specs there (via the --spec-files option), we now pass those in from the OptionsBootstrapper, so only it needs to know about bootstrap options.
| allow_unknown_options: bool = False, | ||
| native_options_config_discovery: bool = True, | ||
| allow_pantsrc: bool = True, |
There was a problem hiding this comment.
Previously we were not plumbing allow_pantsrc all the way from the top, now we do.
OTOH we no longer need to set native_options_config_discovery to False in some tests, as passing explicit config_sources (that are not None) has the same effect. None triggers the config discovery logic in Rust.
| @@ -150,19 +150,19 @@ def create( | |||
| scope: registrar.known_scoped_args for scope, registrar in registrar_by_scope.items() | |||
| } | |||
|
|
|||
| config_to_pass = None if native_options_config_discovery else config.sources() | |||
There was a problem hiding this comment.
native_options_config_discovery is no more - as mentioned above, this is now done by passing None for config sources. And since we no longer have any python-side config file discovery logic, #21091 is no longer an issue.
| native_parser = NativeOptionParser( | ||
| args[1:], # The native parser expects args without the sys.argv[0] binary name. | ||
| env, | ||
| config_sources=config_to_pass, | ||
| allow_pantsrc=True, |
There was a problem hiding this comment.
This was where we failed to plumb this setting to the top, now remedied.
| @@ -177,15 +177,6 @@ def create( | |||
| ) | |||
| ) | |||
|
|
|||
| if bootstrap_option_values: | |||
There was a problem hiding this comment.
As mentioned, this logic is now in the OptionsBootstrapper
| @@ -300,7 +291,6 @@ def verify_configs(self, global_config: Config) -> None: | |||
| """ | |||
| ) | |||
| ) | |||
| global_config.verify(section_to_valid_options) | |||
There was a problem hiding this comment.
This was an oversight - config verification has been done in Rust for a while now, but I hadn't removed the python side call...
| self._check_and_apply_deprecations(scope, native_values_builder) | ||
| native_values = native_values_builder.build() | ||
| return native_values | ||
| self._check_and_apply_deprecations(scope, builder) |
There was a problem hiding this comment.
This is just some local streamlining that could have been done sooner. We don't need to build an OptionsValueContainer and then call to_builder() on it, when we already have a builder in hand...
| return f"OptionsBootstrapper(args={args}, env={env}, config={self.config})" | ||
|
|
||
| @staticmethod | ||
| def get_config_file_paths(env, args) -> list[str]: |
There was a problem hiding this comment.
This chicken-and-egg problem no longer exists. Or rather, it is handled much more elegantly in the Rust code. So this entire special-casing monstrosity can go.
| ) | ||
| bootstrap_options = cls._create_bootstrap_options(args, env, allow_pantsrc) | ||
|
|
||
| # We need to set this env var to allow various static help strings to reference the |
There was a problem hiding this comment.
I'm not sure we still need this feature. In the age of scie-pants I expect the script name to ~always be pants. But one thing at a time.
|
|
||
| bargs = cls._get_bootstrap_args(args) | ||
|
|
||
| config_file_paths = cls.get_config_file_paths(env=env, args=args) |
There was a problem hiding this comment.
This whole bootstrapping sequence is now implemented in Rust, and is completely unnecessary in Python.
| union_membership, | ||
| allow_unknown_options=allow_unknown_options, | ||
| ) | ||
|
|
||
| def full_options( | ||
| self, build_configuration: BuildConfiguration, union_membership: UnionMembership | ||
| ) -> Options: | ||
| global_bootstrap_options = self.get_bootstrap_options().for_global_scope() |
There was a problem hiding this comment.
This logic has moved down a few lines, and uses regular global scope options.
| @@ -39,9 +39,9 @@ def assert_bootstrap_options( | |||
| fp.write(f"{k} = {repr(v)}\n") | |||
| fp.close() | |||
|
|
|||
| args = [*self._config_path(fp.name), *(args or [])] | |||
| args = ["pants", *self._config_path(fp.name), *(args or [])] | |||
There was a problem hiding this comment.
The OptionsBootstrapper now uniformly requires that the args start with argv[0], so...
|
|
||
| vals = parse_options("main", "args", "-lwarn", "--", "-lerror") | ||
| assert LogLevel.WARN == vals.level | ||
|
|
||
| vals = parse_options("main", "args", "--", "-lerror") | ||
| assert LogLevel.INFO == vals.level | ||
|
|
||
| def test_bootstrap_options_explicit_config_path(self) -> None: |
There was a problem hiding this comment.
This was testing the bespoke --pants-config-files parsing logic, which was deleted. However since this option is consulted in the rust bootstrap sequence, I added extra testing for that there (see below). That new test is less meticulous than this one, however, since it's read via regular option parsing code that is heavily tested already.
| @@ -422,3 +368,26 @@ def munge(bin_name: str) -> str: | |||
| assert munge(os.path.join(build_root, "bin", "pants")) == "./bin/pants" | |||
| assert munge("/foo/pants") == "pants" | |||
| assert munge("/foo/bar/pants") == "pants" | |||
|
|
|||
|
|
|||
| def test_file_spec_args() -> None: | |||
There was a problem hiding this comment.
Now that the --spec-file handling code has moved to OptionsBootstrapper, its test correspondingly moves here.
| @@ -1058,35 +1052,13 @@ def test_is_known_scope() -> None: | |||
| assert not options.is_known_scope("nonexistent_scope") | |||
|
|
|||
|
|
|||
| def test_file_spec_args() -> None: | |||
There was a problem hiding this comment.
Moved to options_bootstrapper_test.py
benjyw
force-pushed
the
bootstrap_options
branch
from
b1ed362 to
e021a78
Compare
|
Reviewers- apologies for the size on this one. I have added many comments to aid in review. Thanks!! |
|
CI passes, which reassures me that nothing has fallen between the seats. |
|
Also note that none of this is public API. |
Now that options parsing is done in Rust, much of the logic
in the OptionsBootstrapper was unneeded. This change
removes much of that defunct logic, particularly around
config file discovery, and creation of intermediate
"bootstrap options".
Unfortunately this logic was still pretty entangled into the
bootstrap sequence, so this change is slightly intricate.