From 9c78cd71e1e8b210e3a36856045b72cefd056435 Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Mon, 27 Jul 2020 19:15:32 -0700 Subject: [PATCH] Make GoalSubsystem subclass Subsystem. [ci skip-rust] [ci skip-build-wheels] --- src/python/pants/engine/goal.py | 19 +-- src/python/pants/option/options_test.py | 2 +- src/python/pants/subsystem/subsystem.py | 132 ++++++++++++++++++- src/python/pants/subsystem/subsystem_test.py | 5 +- 4 files changed, 130 insertions(+), 28 deletions(-) diff --git a/src/python/pants/engine/goal.py b/src/python/pants/engine/goal.py index 48651603e78d..cce7d6662df6 100644 --- a/src/python/pants/engine/goal.py +++ b/src/python/pants/engine/goal.py @@ -8,16 +8,14 @@ from typing_extensions import final -from pants.option.option_value_container import OptionValueContainer -from pants.option.optionable import Optionable -from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin +from pants.subsystem.subsystem import Subsystem from pants.util.meta import classproperty if TYPE_CHECKING: from pants.engine.console import Console -class GoalSubsystem(SubsystemClientMixin, Optionable): +class GoalSubsystem(Subsystem): """The Subsystem used by `Goal`s to register the external API, meaning the goal name, the help message, and any options. @@ -49,19 +47,6 @@ def name(cls): def options_scope(cls) -> str: # type: ignore[override] return cast(str, cls.name) - @final - def __init__(self, scope: str, scoped_options: OptionValueContainer) -> None: - # NB: This constructor is shaped to meet the contract of `Optionable(Factory).signature`. - super().__init__() - self._scope = scope - self._scoped_options = scoped_options - - @final - @property - def options(self) -> OptionValueContainer: - """Returns the option values.""" - return self._scoped_options - @dataclass(frozen=True) class Goal: diff --git a/src/python/pants/option/options_test.py b/src/python/pants/option/options_test.py index 1806d6ddeae5..9828ec496f69 100644 --- a/src/python/pants/option/options_test.py +++ b/src/python/pants/option/options_test.py @@ -1739,7 +1739,7 @@ class DummyOptionable1(Optionable): global_scope(), DummyOptionable1.get_scope_info(), # A deprecated, scoped dependency on `DummyOptionable1`. This - # imitates the construction of SubsystemClientMixin.known_scope_infos. + # imitates the construction of Subsystem.known_scope_infos. ScopeInfo( DummyOptionable1.subscope("sub"), DummyOptionable1, diff --git a/src/python/pants/subsystem/subsystem.py b/src/python/pants/subsystem/subsystem.py index 40c5736fe7ba..2627bbd64274 100644 --- a/src/python/pants/subsystem/subsystem.py +++ b/src/python/pants/subsystem/subsystem.py @@ -9,8 +9,9 @@ from pants.option.option_value_container import OptionValueContainer from pants.option.optionable import Optionable from pants.option.options import Options -from pants.option.scope import ScopeInfo -from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin, SubsystemDependency +from pants.option.scope import ScopeInfo, GLOBAL_SCOPE +from pants.subsystem.subsystem_client_mixin import SubsystemDependency +from pants.util.ordered_set import OrderedSet logger = logging.getLogger(__name__) @@ -22,7 +23,7 @@ class SubsystemError(Exception): _S = TypeVar("_S", bound="Subsystem") -class Subsystem(SubsystemClientMixin, Optionable): +class Subsystem(Optionable): """A separable piece of functionality that may be reused across multiple tasks or other code. Subsystems encapsulate the configuration and initialization of things like JVMs, @@ -40,7 +41,7 @@ class Subsystem(SubsystemClientMixin, Optionable): For example, the global artifact cache options would be in scope `cache`, but the compile.java task can override those options in scope `cache.compile.java`. - Subsystems may depend on other subsystems, and therefore mix in SubsystemClientMixin. + Subsystems may depend on other subsystems. :API: public """ @@ -60,7 +61,7 @@ def scoped(cls, optionable, removal_version=None, removal_hint=None): :param removal_version: An optional deprecation version for this scoped Subsystem dependency. :param removal_hint: An optional hint to accompany a deprecation removal_version. - Return value is suitable for use in SubsystemClientMixin.subsystem_dependencies(). + Return value is suitable for use in Subsystem.subsystem_dependencies(). """ return SubsystemDependency(cls, optionable.options_scope, removal_version, removal_hint) @@ -143,8 +144,6 @@ def reset(cls, reset_options: bool = True) -> None: def __init__(self, scope: str, scoped_options: OptionValueContainer) -> None: """Note: A subsystem has no access to options in scopes other than its own. - TODO: We'd like that to be true of Tasks some day. Subsystems will help with that. - Code should call scoped_instance() or global_instance() to get a subsystem instance. It should not invoke this constructor directly. @@ -228,3 +227,122 @@ def get_streaming_workunit_callbacks(subsystem_names: Iterable[str]) -> List[Cal continue return callables + + @classmethod + def subsystem_dependencies(cls): + """The subsystems this object uses. + + Override to specify your subsystem dependencies. Always add them to your superclass's value. + + Note: Do not call this directly to retrieve dependencies. See subsystem_dependencies_iter(). + + :return: A tuple of SubsystemDependency instances. + In the common case where you're an optionable and you want to get an instance scoped + to you, call subsystem_cls.scoped(cls) to get an appropriate SubsystemDependency. + As a convenience, you may also provide just a subsystem_cls, which is shorthand for + SubsystemDependency(subsystem_cls, GLOBAL SCOPE) and indicates that we want to use + the global instance of that subsystem. + """ + return tuple() + + @classmethod + def subsystem_dependencies_iter(cls): + """Iterate over the direct subsystem dependencies of this Optionable.""" + for dep in cls.subsystem_dependencies(): + if isinstance(dep, SubsystemDependency): + yield dep + else: + yield SubsystemDependency( + dep, GLOBAL_SCOPE, removal_version=None, removal_hint=None + ) + + @classmethod + def subsystem_closure_iter(cls): + """Iterate over the transitive closure of subsystem dependencies of this Optionable. + + :rtype: :class:`collections.Iterator` of :class:`SubsystemDependency` + :raises: :class:`pants.subsystem.subsystem_client_mixin.Subsystem.CycleException` + if a dependency cycle is detected. + """ + seen = set() + dep_path = OrderedSet() + + def iter_subsystem_closure(subsystem_cls): + if subsystem_cls in dep_path: + raise cls.CycleException(list(dep_path) + [subsystem_cls]) + dep_path.add(subsystem_cls) + + for dep in subsystem_cls.subsystem_dependencies_iter(): + if dep not in seen: + seen.add(dep) + yield dep + for d in iter_subsystem_closure(dep.subsystem_cls): + yield d + + dep_path.remove(subsystem_cls) + + for dep in iter_subsystem_closure(cls): + yield dep + + class CycleException(Exception): + """Thrown when a circular subsystem dependency is detected.""" + + def __init__(self, cycle): + message = "Cycle detected:\n\t{}".format( + " ->\n\t".join( + "{} scope: {}".format(optionable_cls, optionable_cls.options_scope) + for optionable_cls in cycle + ) + ) + super().__init__(message) + + @classmethod + def known_scope_infos(cls): + """Yield ScopeInfo for all known scopes for this optionable, in no particular order. + + :rtype: set of :class:`pants.option.scope.ScopeInfo` + :raises: :class:`pants.subsystem.subsystem_client_mixin.Subsystem.CycleException` + if a dependency cycle is detected. + """ + known_scope_infos = set() + optionables_path = ( + OrderedSet() + ) # To check for cycles at the Optionable level, ignoring scope. + + def collect_scope_infos(optionable_cls, scoped_to, removal_version=None, removal_hint=None): + if optionable_cls in optionables_path: + raise cls.CycleException(list(optionables_path) + [optionable_cls]) + optionables_path.add(optionable_cls) + + scope = ( + optionable_cls.options_scope + if scoped_to == GLOBAL_SCOPE + else optionable_cls.subscope(scoped_to) + ) + scope_info = ScopeInfo( + scope, optionable_cls, removal_version=removal_version, removal_hint=removal_hint, + ) + + if scope_info not in known_scope_infos: + known_scope_infos.add(scope_info) + for dep in scope_info.optionable_cls.subsystem_dependencies_iter(): + # A subsystem always exists at its global scope (for the purpose of options + # registration and specification), even if in practice we only use it scoped to + # some other scope. + # + # NB: We do not apply deprecations to this implicit global copy of the scope, because if + # the intention was to deprecate the entire scope, that could be accomplished by + # deprecating all options in the scope. + collect_scope_infos(dep.subsystem_cls, GLOBAL_SCOPE) + if not dep.is_global(): + collect_scope_infos( + dep.subsystem_cls, + scope, + removal_version=dep.removal_version, + removal_hint=dep.removal_hint, + ) + + optionables_path.remove(scope_info.optionable_cls) + + collect_scope_infos(cls, GLOBAL_SCOPE) + return known_scope_infos diff --git a/src/python/pants/subsystem/subsystem_test.py b/src/python/pants/subsystem/subsystem_test.py index 093498bd9ded..2cd0b862623b 100644 --- a/src/python/pants/subsystem/subsystem_test.py +++ b/src/python/pants/subsystem/subsystem_test.py @@ -6,7 +6,6 @@ from pants.option.optionable import Optionable from pants.option.scope import ScopeInfo from pants.subsystem.subsystem import Subsystem -from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin from pants.testutil.test_base import TestBase @@ -146,7 +145,7 @@ def subsystem_dependencies(cls): return (SubsystemB,) for root in SubsystemA, SubsystemB, SubsystemC: - with self.assertRaises(SubsystemClientMixin.CycleException): + with self.assertRaises(Subsystem.CycleException): root.known_scope_infos() def test_scoping_complex(self) -> None: @@ -299,7 +298,7 @@ class SubsystemB(Subsystem): def subsystem_dependencies(cls): return (SubsystemA,) - with self.assertRaises(SubsystemClientMixin.CycleException): + with self.assertRaises(Subsystem.CycleException): list(SubsystemB.subsystem_closure_iter()) def test_get_streaming_workunit_callbacks(self) -> None: