From 294ad9a11c2d4f980caead1f7ad4ce0e81d60406 Mon Sep 17 00:00:00 2001 From: Andreas Stenius <andreas.stenius@svenskaspel.se> Date: Tue, 13 Jul 2021 21:58:19 +0200 Subject: [PATCH 1/3] wip: do not leak non-goal subsystems as goals to the command line interface. Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] --- src/python/pants/engine/goal.py | 5 ++++ src/python/pants/engine/goal_test.py | 12 ++++++++++ src/python/pants/option/arg_splitter.py | 2 +- src/python/pants/option/arg_splitter_test.py | 24 ++++++++++++-------- src/python/pants/option/optionable.py | 7 +++++- src/python/pants/option/scope.py | 3 +++ src/python/pants/option/subsystem.py | 8 +++---- 7 files changed, 46 insertions(+), 15 deletions(-) diff --git a/src/python/pants/engine/goal.py b/src/python/pants/engine/goal.py index 15187b284de..3cfb2b27773 100644 --- a/src/python/pants/engine/goal.py +++ b/src/python/pants/engine/goal.py @@ -8,6 +8,7 @@ from typing_extensions import final +from pants.option.scope import ScopeInfo from pants.option.subsystem import Subsystem from pants.util.meta import classproperty @@ -37,6 +38,10 @@ def list(console: Console, list_subsystem: ListSubsystem) -> List: # it should declare the union types that must have members. required_union_implementations: Tuple[Type, ...] = () + @classmethod + def create_scope_info(cls, **scope_info_kwargs) -> ScopeInfo: + return super().create_scope_info(is_goal=True, **scope_info_kwargs) + @classproperty @abstractmethod def name(cls): diff --git a/src/python/pants/engine/goal_test.py b/src/python/pants/engine/goal_test.py index 8817782c7e2..c772ab6ac49 100644 --- a/src/python/pants/engine/goal_test.py +++ b/src/python/pants/engine/goal_test.py @@ -4,6 +4,7 @@ from pants.engine.console import Console from pants.engine.goal import Goal, GoalSubsystem, LineOriented from pants.engine.rules import goal_rule +from pants.option.scope import ScopeInfo from pants.testutil.option_util import create_goal_subsystem, create_options_bootstrapper from pants.testutil.rule_runner import mock_console, run_rule_with_mocks @@ -33,3 +34,14 @@ def output_rule(console: Console, options: OutputtingGoalOptions) -> OutputtingG ) assert result.exit_code == 0 assert stdio_reader.get_stdout() == "output...line oriented\n" + + +def test_goal_scope_flag() -> None: + class DummyGoal(GoalSubsystem): + name = "dummy" + + dummy = create_goal_subsystem(DummyGoal) + assert dummy.get_scope_info().is_goal is True + assert dummy.known_scope_infos() == { + ScopeInfo(scope="dummy", optionable_cls=DummyGoal, is_goal=True) + } diff --git a/src/python/pants/option/arg_splitter.py b/src/python/pants/option/arg_splitter.py index 53a3f5191ab..0c3bec413e6 100644 --- a/src/python/pants/option/arg_splitter.py +++ b/src/python/pants/option/arg_splitter.py @@ -84,7 +84,7 @@ class ArgSplitter: def __init__(self, known_scope_infos: Iterable[ScopeInfo], buildroot: str) -> None: self._buildroot = Path(buildroot) self._known_scope_infos = known_scope_infos - self._known_scopes = {si.scope for si in known_scope_infos} | { + self._known_scopes = {si.scope for si in known_scope_infos if si.is_goal} | { "version", "help", "help-advanced", diff --git a/src/python/pants/option/arg_splitter_test.py b/src/python/pants/option/arg_splitter_test.py index 0b5b5544ea1..264c2525c79 100644 --- a/src/python/pants/option/arg_splitter_test.py +++ b/src/python/pants/option/arg_splitter_test.py @@ -22,15 +22,18 @@ class ArgSplitterTest(unittest.TestCase): _known_scope_infos = [ - ScopeInfo("compile"), - ScopeInfo("compile.java"), - ScopeInfo("compile.scala"), - ScopeInfo("jvm"), - ScopeInfo("jvm.test.junit"), - ScopeInfo("reporting"), - ScopeInfo("test"), - ScopeInfo("test.junit"), - ] + ScopeInfo(scope, is_goal=True) + for scope in [ + "compile", + "compile.java", + "compile.scala", + "jvm", + "jvm.test.junit", + "reporting", + "test", + "test.junit", + ] + ] + [ScopeInfo("hidden", is_goal=False)] @staticmethod def assert_valid_split( @@ -384,3 +387,6 @@ def test_no_goal_detection(self) -> None: splitter = ArgSplitter(ArgSplitterTest._known_scope_infos, buildroot=os.getcwd()) splitter.split_args(shlex.split("./pants foo/bar:baz")) self.assertTrue(isinstance(splitter.help_request, NoGoalHelp)) + + def test_hidden_scope_is_unknown_goal(self) -> None: + self.assert_unknown_goal("./pants hidden", ["hidden"]) diff --git a/src/python/pants/option/optionable.py b/src/python/pants/option/optionable.py index b393e178b6b..dce1cb334de 100644 --- a/src/python/pants/option/optionable.py +++ b/src/python/pants/option/optionable.py @@ -94,12 +94,17 @@ def validate_scope_name_component(cls, s: str) -> None: f"dash-separated-words, with words consisting only of lower-case letters and digits." ) + @classmethod + def create_scope_info(cls, **scope_info_kwargs) -> ScopeInfo: + """One place to create scope info, to allow subclasses to inject custom scope args.""" + return ScopeInfo(**scope_info_kwargs) + @classmethod def get_scope_info(cls) -> ScopeInfo: """Returns a ScopeInfo instance representing this Optionable's options scope.""" if cls.options_scope is None: raise OptionsError(f"{cls.__name__} must set options_scope.") - return ScopeInfo(scope=cast(str, cls.options_scope), optionable_cls=cls) + return cls.create_scope_info(scope=cast(str, cls.options_scope), optionable_cls=cls) @classmethod def subscope(cls, scope) -> str: diff --git a/src/python/pants/option/scope.py b/src/python/pants/option/scope.py index 9304e9f62c1..b1894199cc3 100644 --- a/src/python/pants/option/scope.py +++ b/src/python/pants/option/scope.py @@ -34,6 +34,9 @@ class ScopeInfo: removal_version: Optional[str] = None removal_hint: Optional[str] = None + # command line goal scope flag + is_goal: bool = False + @property def description(self) -> str: return cast(str, getattr(self.optionable_cls, "help")) diff --git a/src/python/pants/option/subsystem.py b/src/python/pants/option/subsystem.py index a31111c36cb..010da9efecf 100644 --- a/src/python/pants/option/subsystem.py +++ b/src/python/pants/option/subsystem.py @@ -96,7 +96,7 @@ def get_scope_info(cls, subscope=None) -> ScopeInfo: if subscope is None: return super().get_scope_info() else: - return ScopeInfo(cls.subscope(subscope), cls) + return cls.create_scope_info(scope=cls.subscope(subscope), optionable_cls=cls) def __init__(self, scope: str, options: OptionValueContainer) -> None: super().__init__() @@ -191,9 +191,9 @@ def collect_scope_infos(optionable_cls, scoped_to, removal_version=None, removal if scoped_to == GLOBAL_SCOPE else optionable_cls.subscope(scoped_to) ) - scope_info = ScopeInfo( - scope, - optionable_cls, + scope_info = cls.create_scope_info( + scope=scope, + optionable_cls=optionable_cls, removal_version=removal_version, removal_hint=removal_hint, ) From 769496326f869f186a47558e8d3e7c653228c5ef Mon Sep 17 00:00:00 2001 From: Andreas Stenius <andreas.stenius@svenskaspel.se> Date: Wed, 14 Jul 2021 09:18:24 +0200 Subject: [PATCH 2/3] move scope goal filtering to limit unwanted side effects. Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/option/arg_splitter.py | 6 ++++-- src/python/pants/option/options_test.py | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/python/pants/option/arg_splitter.py b/src/python/pants/option/arg_splitter.py index 0c3bec413e6..1b9fbc2c70f 100644 --- a/src/python/pants/option/arg_splitter.py +++ b/src/python/pants/option/arg_splitter.py @@ -84,12 +84,13 @@ class ArgSplitter: def __init__(self, known_scope_infos: Iterable[ScopeInfo], buildroot: str) -> None: self._buildroot = Path(buildroot) self._known_scope_infos = known_scope_infos - self._known_scopes = {si.scope for si in known_scope_infos if si.is_goal} | { + self._known_scopes = {si.scope for si in known_scope_infos} | { "version", "help", "help-advanced", "help-all", } + self._known_goal_scopes = {si.scope for si in known_scope_infos if si.is_goal} self._unconsumed_args: List[ str ] = [] # In reverse order, for efficient popping off the end. @@ -171,7 +172,8 @@ def assign_flag_to_scope(flg: str, default_scope: str) -> None: while scope: if not self._check_for_help_request(scope.lower()): add_scope(scope) - goals.add(scope.partition(".")[0]) + if scope in self._known_goal_scopes: + goals.add(scope.partition(".")[0]) for flag in flags: assign_flag_to_scope(flag, scope) scope, flags = self._consume_scope() diff --git a/src/python/pants/option/options_test.py b/src/python/pants/option/options_test.py index f31981561d6..ef29b0db110 100644 --- a/src/python/pants/option/options_test.py +++ b/src/python/pants/option/options_test.py @@ -57,7 +57,7 @@ def global_scope() -> ScopeInfo: def task(scope: str) -> ScopeInfo: - return ScopeInfo(scope) + return ScopeInfo(scope, is_goal=True) def intermediate(scope: str) -> ScopeInfo: From 005c428e9d0f19ff50f2b873d20c8d61d26f4ac9 Mon Sep 17 00:00:00 2001 From: Andreas Stenius <andreas.stenius@svenskaspel.se> Date: Wed, 14 Jul 2021 15:36:25 +0200 Subject: [PATCH 3/3] add unknown goal to list. Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/option/arg_splitter.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/python/pants/option/arg_splitter.py b/src/python/pants/option/arg_splitter.py index 1b9fbc2c70f..8a09db04588 100644 --- a/src/python/pants/option/arg_splitter.py +++ b/src/python/pants/option/arg_splitter.py @@ -174,6 +174,8 @@ def assign_flag_to_scope(flg: str, default_scope: str) -> None: add_scope(scope) if scope in self._known_goal_scopes: goals.add(scope.partition(".")[0]) + else: + unknown_scopes.append(scope) for flag in flags: assign_flag_to_scope(flag, scope) scope, flags = self._consume_scope()