Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify OptionsBootstrapper #21784

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions pants-plugins/internal_plugins/releases/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
from pants.engine.rules import Get, collect_rules, goal_rule, rule
from pants.engine.target import Target
from pants.engine.unions import UnionRule
from pants.option.config import Config
from pants.option.options_bootstrapper import OptionsBootstrapper
from pants.util.frozendict import FrozenDict
from pants.util.strutil import softwrap
from pants.version import VERSION

Expand Down Expand Up @@ -131,10 +131,9 @@ async def check_default_tools(
SessionValues(
{
OptionsBootstrapper: OptionsBootstrapper(
tuple(),
("./pants",),
args,
Config(tuple()),
args=args,
env=FrozenDict(),
Copy link
Contributor Author

@benjyw benjyw Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

allow_pantsrc=False,
)
}
),
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/bin/daemon_pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def single_daemonized_run(
start_time = float(env_start_time) if env_start_time else time.time()

options_bootstrapper = OptionsBootstrapper.create(
env=env, args=args, allow_pantsrc=True
args=args, env=env, allow_pantsrc=True
)

# Run using the pre-warmed Session.
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/bin/local_pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def create(

# Verify configs.
if global_bootstrap_options.verify_config:
options.verify_configs(options_bootstrapper.config)
options.verify_configs()

# If we're running with the daemon, we'll be handed a warmed Scheduler, which we use
# to initialize a session here.
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/bin/pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def run(self, start_time: float) -> ExitCode:
self.scrub_pythonpath()

options_bootstrapper = OptionsBootstrapper.create(
env=self.env, args=self.args, allow_pantsrc=True
args=self.args, env=self.env, allow_pantsrc=True
)
with warnings.catch_warnings(record=True):
bootstrap_options = options_bootstrapper.bootstrap_options
Expand Down
7 changes: 2 additions & 5 deletions src/python/pants/help/help_info_extracter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from pants.engine.target import IntField, RegisteredTargetTypes, StringField, Target
from pants.engine.unions import UnionMembership
from pants.help.help_info_extracter import HelpInfoExtracter, pretty_print_type_hint, to_help_str
from pants.option.config import Config
from pants.option.global_options import GlobalOptions, LogLevelOption
from pants.option.native_options import NativeOptionParser
from pants.option.option_types import BoolOption, IntListOption, OptionInfo, StrListOption
Expand Down Expand Up @@ -260,12 +259,10 @@ class BazLibrary(Target):
config_path = "pants.test.toml"
config_source = SimpleNamespace(path=config_path, content=b"[GLOBAL]\nopt1 = '+[99]'")
options = Options.create(
args=["./pants", "--backend-packages=['internal_plugins.releases']"],
env={"PANTS_OPT1": "88"},
config=Config.load([config_source]),
native_options_config_discovery=False,
config_sources=[config_source],
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,
include_derivation=True,
)
Global.register_options_on_scope(options, UnionMembership({}))
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/init/options_initializer.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def _initialize_build_configuration(
2. is expensive to call, because it might resolve plugins from the network
"""

bootstrap_options = options_bootstrapper.get_bootstrap_options().for_global_scope()
bootstrap_options = options_bootstrapper.bootstrap_options.for_global_scope()

# Add any extra paths to python path (e.g., for loading extra source backends).
for path in bootstrap_options.pythonpath:
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/option/global_options_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ def test_invalidation_globs() -> None:
# empty entry (i.e.: a relative path for the current directory) doesn't cause an error.
suffix = "something-ridiculous"
ob = OptionsBootstrapper.create(
env={}, args=[f"--pythonpath=../{suffix}", "--pythonpath="], allow_pantsrc=False
args=[f"--pythonpath=../{suffix}", "--pythonpath="], env={}, allow_pantsrc=False
)
globs = GlobalOptions.compute_pantsd_invalidation_globs(
get_buildroot(), ob.bootstrap_options.for_global_scope()
Expand Down
49 changes: 18 additions & 31 deletions src/python/pants/option/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

from pants.option.errors import (
ConfigValidationError,
MutuallyExclusiveOptionError,
Expand Down Expand Up @@ -116,26 +116,26 @@ def complete_scopes(cls, scope_infos: Iterable[ScopeInfo]) -> FrozenOrderedSet[S
@classmethod
def create(
cls,
env: Mapping[str, str],
config: Config,
known_scope_infos: Iterable[ScopeInfo],
*,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To force callsites to be readable...

args: Sequence[str],
bootstrap_option_values: OptionValueContainer | None = None,
env: Mapping[str, str],
config_sources: Sequence[ConfigSource] | None,
known_scope_infos: Sequence[ScopeInfo],
extra_specs: Sequence[str] = tuple(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor Author

@benjyw benjyw Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

include_derivation: bool = False,
) -> Options:
"""Create an Options instance.

:param args: a list of cmd-line args; defaults to `sys.argv` if None is supplied.
:param env: a dict of environment variables.
:param config: data from a config file.
:param config_sources: sources of config data.
:param known_scope_infos: ScopeInfos for all scopes that may be encountered.
:param args: a list of cmd-line args; defaults to `sys.argv` if None is supplied.
:param bootstrap_option_values: An optional namespace containing the values of bootstrap
options. We can use these values when registering other options.
:param extra_specs: Extra specs to add to those specified in the args (e.g., from --spec-files).
:param allow_unknown_options: Whether to ignore or error on unknown cmd-line flags.
:param native_options_config_discovery: Whether to discover config files in the native
parser or use the ones supplied.
:param allow_pantsrc: Whether to read config from local .rc files. Typically
disabled in tests, for hermeticity.
:param include_derivation: Whether to gather option value derivation information.
"""
# We need registrars for all the intermediate scopes, so inherited option values
Expand All @@ -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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was where we failed to plumb this setting to the top, now remedied.

config_sources=config_sources,
allow_pantsrc=allow_pantsrc,
include_derivation=include_derivation,
known_scopes_to_flags=known_scope_to_flags,
)

splitter = ArgSplitter(complete_known_scope_infos, get_buildroot())
# We take the cli alias-expanded args[1:] from the native parser.
split_args = splitter.split_args([args[0], *native_parser.get_args()])
split_args.specs.extend(extra_specs)

if split_args.passthru and len(split_args.goals) > 1:
raise cls.AmbiguousPassthroughError(
Expand All @@ -177,15 +177,6 @@ def create(
)
)

if bootstrap_option_values:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, this logic is now in the OptionsBootstrapper

spec_files = bootstrap_option_values.spec_files
if spec_files:
for spec_file in spec_files:
with open(spec_file) as f:
split_args.specs.extend(
[line for line in [line.strip() for line in f] if line]
)

return cls(
builtin_or_auxiliary_goal=split_args.builtin_or_auxiliary_goal,
goals=split_args.goals,
Expand Down Expand Up @@ -278,7 +269,7 @@ def known_scope_to_scoped_args(self) -> dict[str, frozenset[str]]:
def scope_to_flags(self) -> dict[str, list[str]]:
return self._scope_to_flags

def verify_configs(self, global_config: Config) -> None:
def verify_configs(self) -> None:
"""Verify all loaded configs have correct scopes and options."""

section_to_valid_options = {}
Expand All @@ -300,7 +291,6 @@ def verify_configs(self, global_config: Config) -> None:
"""
)
)
global_config.verify(section_to_valid_options)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an oversight - config verification has been done in Rust for a while now, but I hadn't removed the python side call...


def get_args(self) -> tuple[str, ...]:
return self._native_parser.get_args()
Expand Down Expand Up @@ -459,14 +449,11 @@ def for_scope(
)
)
setattr(builder, dest, RankedValue(rank, val))
native_values = builder.build()

# Check for any deprecation conditions, which are evaluated using `self._flag_matchers`.
if check_deprecations:
native_values_builder = native_values.to_builder()
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 builder.build()

def get_fingerprintable_for_scope(
self,
Expand Down
Loading
Loading