diff --git a/src/python/pants/build_graph/build_configuration.py b/src/python/pants/build_graph/build_configuration.py index 99ea52cb6f0..dc4c0db2817 100644 --- a/src/python/pants/build_graph/build_configuration.py +++ b/src/python/pants/build_graph/build_configuration.py @@ -3,30 +3,90 @@ import logging import typing +from collections import defaultdict from collections.abc import Iterable from dataclasses import dataclass, field -from typing import Any, Dict, Type, Union +from enum import Enum +from typing import Any, DefaultDict, Dict, Set, Type, Union, cast from pants.build_graph.build_file_aliases import BuildFileAliases +from pants.engine.goal import GoalSubsystem from pants.engine.rules import Rule, RuleIndex from pants.engine.target import Target from pants.engine.unions import UnionRule +from pants.goal.run_tracker import RunTracker +from pants.option.global_options import GlobalOptions from pants.option.optionable import Optionable +from pants.option.scope import normalize_scope +from pants.reporting.reporting import Reporting from pants.util.ordered_set import FrozenOrderedSet, OrderedSet +from pants.vcs.changed import Changed logger = logging.getLogger(__name__) +# No goal or target_type can have a name from this set, so that `./pants help ` +# is unambiguous. +_RESERVED_NAMES = {"global", "targets", "goals"} + + +# Subsystems used outside of any rule. +_GLOBAL_SUBSYSTEMS: FrozenOrderedSet[Type[Optionable]] = FrozenOrderedSet( + {GlobalOptions, Reporting, RunTracker, Changed} +) + + @dataclass(frozen=True) class BuildConfiguration: """Stores the types and helper functions exposed to BUILD files.""" registered_aliases: BuildFileAliases - optionables: FrozenOrderedSet[Optionable] + optionables: FrozenOrderedSet[Type[Optionable]] rules: FrozenOrderedSet[Rule] union_rules: FrozenOrderedSet[UnionRule] target_types: FrozenOrderedSet[Type[Target]] + @property + def all_optionables(self) -> FrozenOrderedSet[Type[Optionable]]: + """Return all optionables in the system: global and those registered via rule usage.""" + return _GLOBAL_SUBSYSTEMS | self.optionables + + def __post_init__(self) -> None: + class Category(Enum): + goal = "goal" + reserved_name = "reserved name" + subsystem = "subsystem" + target_type = "target type" + + name_to_categories: DefaultDict[str, Set[Category]] = defaultdict(set) + normalized_to_orig_name: Dict[str, str] = {} + + for opt in self.all_optionables: + scope = cast(str, opt.options_scope) + normalized_scope = normalize_scope(scope) + name_to_categories[normalized_scope].add( + Category.goal if issubclass(opt, GoalSubsystem) else Category.subsystem + ) + normalized_to_orig_name[normalized_scope] = scope + for tgt_type in self.target_types: + name_to_categories[normalize_scope(tgt_type.alias)].add(Category.target_type) + for reserved_name in _RESERVED_NAMES: + name_to_categories[normalize_scope(reserved_name)].add(Category.reserved_name) + + found_collision = False + for name, cats in name_to_categories.items(): + if len(cats) > 1: + scats = sorted(cat.value for cat in cats) + cats_str = ", ".join(f"a {cat}" for cat in scats[:-1]) + f" and a {scats[-1]}." + colliding_names = "`/`".join( + sorted({name, normalized_to_orig_name.get(name, name)}) + ) + logger.error(f"Naming collision: `{colliding_names}` is registered as {cats_str}") + found_collision = True + + if found_collision: + raise TypeError("Found naming collisions. See log for details.") + @dataclass class Builder: _exposed_object_by_alias: Dict[Any, Any] = field(default_factory=dict) @@ -42,8 +102,8 @@ def registered_aliases(self) -> BuildFileAliases: These returned aliases aren't so useful for actually parsing BUILD files. They are useful for generating online documentation. - :returns: A new BuildFileAliases instance containing this BuildConfiguration's registered alias - mappings. + :returns: A new BuildFileAliases instance containing this BuildConfiguration's + registered alias mappings. """ return BuildFileAliases( objects=self._exposed_object_by_alias.copy(), @@ -94,13 +154,12 @@ def _register_exposed_context_aware_object_factory( alias ] = context_aware_object_factory - def register_optionables(self, optionables): - """Registers the given subsystem types. - - :param optionables: The Optionable types to register. - :type optionables: :class:`collections.Iterable` containing - :class:`pants.option.optionable.Optionable` subclasses. - """ + # NB: We expect the parameter to be Iterable[Type[Optionable]], but we can't be confident + # in this because we pass whatever people put in their `register.py`s to this function; + # I.e., this is an impure function that reads from the outside world. So, we use the type + # hint `Any` and perform runtime type checking. + def register_optionables(self, optionables: Union[typing.Iterable[Type[Optionable]], Any]): + """Registers the given subsystem types.""" if not isinstance(optionables, Iterable): raise TypeError("The optionables must be an iterable, given {}".format(optionables)) optionables = tuple(optionables) @@ -137,17 +196,18 @@ def register_rules(self, rules): rule.output_type for rule in self._rules if issubclass(rule.output_type, Optionable) ) - # NB: We expect the parameter to be Iterable[Type[Target]], but we can't be confident in this - # because we pass whatever people put in their `register.py`s to this function; i.e., this is - # an impure function that reads from the outside world. So, we use the type hint `Any` and - # perform runtime type checking. + # NB: We expect the parameter to be Iterable[Type[Target]], but we can't be confident in + # this because we pass whatever people put in their `register.py`s to this function; + # I.e., this is an impure function that reads from the outside world. So, we use the type + # hint `Any` and perform runtime type checking. def register_target_types( self, target_types: Union[typing.Iterable[Type[Target]], Any] ) -> None: """Registers the given target types.""" if not isinstance(target_types, Iterable): raise TypeError( - f"The entrypoint `target_types` must return an iterable. Given {repr(target_types)}" + f"The entrypoint `target_types` must return an iterable. " + f"Given {repr(target_types)}" ) bad_elements = [ tgt_type diff --git a/src/python/pants/build_graph/build_configuration_test.py b/src/python/pants/build_graph/build_configuration_test.py index a5fe7130209..5332297a91b 100644 --- a/src/python/pants/build_graph/build_configuration_test.py +++ b/src/python/pants/build_graph/build_configuration_test.py @@ -1,66 +1,118 @@ # Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). +from typing import Type -import unittest +import pytest from pants.build_graph.build_configuration import BuildConfiguration from pants.build_graph.build_file_aliases import BuildFileAliases +from pants.engine.goal import GoalSubsystem +from pants.engine.target import Target from pants.engine.unions import UnionRule, union +from pants.option.optionable import Optionable +from pants.option.subsystem import Subsystem from pants.util.frozendict import FrozenDict from pants.util.ordered_set import FrozenOrderedSet -class BuildConfigurationTest(unittest.TestCase): - def setUp(self) -> None: - self.bc_builder = BuildConfiguration.Builder() +@pytest.fixture +def bc_builder() -> BuildConfiguration.Builder: + return BuildConfiguration.Builder() - def _register_aliases(self, **kwargs) -> None: - self.bc_builder.register_aliases(BuildFileAliases(**kwargs)) - def test_register_bad(self) -> None: - with self.assertRaises(TypeError): - self.bc_builder.register_aliases(42) +def _register_aliases(bc_builder, **kwargs) -> None: + bc_builder.register_aliases(BuildFileAliases(**kwargs)) - def test_register_exposed_object(self): - self._register_aliases(objects={"jane": 42}) - aliases = self.bc_builder.create().registered_aliases - assert FrozenDict() == aliases.context_aware_object_factories - assert FrozenDict(jane=42) == aliases.objects - def test_register_exposed_context_aware_object_factory(self): - def caof_function(parse_context): - return parse_context.rel_path +def test_register_bad(bc_builder: BuildConfiguration.Builder) -> None: + with pytest.raises(TypeError): + bc_builder.register_aliases(42) - class CaofClass: - def __init__(self, parse_context): - self._parse_context = parse_context - def __call__(self): - return self._parse_context.rel_path +def test_register_exposed_object(bc_builder: BuildConfiguration.Builder) -> None: + _register_aliases(bc_builder, objects={"jane": 42}) + aliases = bc_builder.create().registered_aliases + assert FrozenDict() == aliases.context_aware_object_factories + assert FrozenDict(jane=42) == aliases.objects + + +def test_register_exposed_context_aware_object_factory( + bc_builder: BuildConfiguration.Builder, +) -> None: + def caof_function(parse_context): + return parse_context.rel_path + + class CaofClass: + def __init__(self, parse_context): + self._parse_context = parse_context + + def __call__(self): + return self._parse_context.rel_path + + _register_aliases( + bc_builder, context_aware_object_factories={"func": caof_function, "cls": CaofClass} + ) + aliases = bc_builder.create().registered_aliases + assert FrozenDict() == aliases.objects + assert ( + FrozenDict({"func": caof_function, "cls": CaofClass}) + == aliases.context_aware_object_factories + ) - self._register_aliases( - context_aware_object_factories={"func": caof_function, "cls": CaofClass} - ) - aliases = self.bc_builder.create().registered_aliases - assert FrozenDict() == aliases.objects - assert ( - FrozenDict({"func": caof_function, "cls": CaofClass}) - == aliases.context_aware_object_factories - ) - def test_register_union_rules(self) -> None: - @union - class Base: - pass +def test_register_union_rules(bc_builder: BuildConfiguration.Builder) -> None: + @union + class Base: + pass - class A: - pass + class A: + pass - class B: - pass + class B: + pass - union_a = UnionRule(Base, A) - union_b = UnionRule(Base, B) - self.bc_builder.register_rules([union_a]) - self.bc_builder.register_rules([union_b]) - assert self.bc_builder.create().union_rules == FrozenOrderedSet([union_a, union_b]) + union_a = UnionRule(Base, A) + union_b = UnionRule(Base, B) + bc_builder.register_rules([union_a]) + bc_builder.register_rules([union_b]) + assert bc_builder.create().union_rules == FrozenOrderedSet([union_a, union_b]) + + +def test_validation(caplog, bc_builder: BuildConfiguration.Builder) -> None: + def mk_dummy_opt(_options_scope: str, goal: bool = False) -> Type[Optionable]: + class DummyOptionable(GoalSubsystem if goal else Subsystem): # type: ignore[misc] + options_scope = _options_scope + + return DummyOptionable + + def mk_dummy_tgt(_alias: str) -> Type[Target]: + class DummyTarget(Target): + alias = _alias + core_fields = tuple() # type: ignore[var-annotated] + + return DummyTarget + + bc_builder.register_optionables( + ( + mk_dummy_opt("foo"), + mk_dummy_opt("Bar-bar"), + mk_dummy_opt("baz"), + mk_dummy_opt("qux", goal=True), + mk_dummy_opt("global"), + ) + ) + bc_builder.register_target_types( + (mk_dummy_tgt("bar_bar"), mk_dummy_tgt("qux"), mk_dummy_tgt("global")) + ) + with pytest.raises(TypeError) as e: + bc_builder.create() + assert ( + "Naming collision: `Bar-bar`/`bar_bar` is registered as a subsystem and a " + "target type." in caplog.text + ) + assert "Naming collision: `qux` is registered as a goal and a target type." in caplog.text + assert ( + "Naming collision: `global` is registered as a reserved name, a subsystem " + "and a target type." in caplog.text + ) + assert "Found naming collisions" in str(e) diff --git a/src/python/pants/init/global_subsystems.py b/src/python/pants/init/global_subsystems.py deleted file mode 100644 index be5258fdd6c..00000000000 --- a/src/python/pants/init/global_subsystems.py +++ /dev/null @@ -1,13 +0,0 @@ -# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). -# Licensed under the Apache License, Version 2.0 (see LICENSE). - -from pants.goal.run_tracker import RunTracker -from pants.reporting.reporting import Reporting -from pants.vcs.changed import Changed - - -class GlobalSubsystems: - @classmethod - def get(cls): - """Subsystems used outside of any task.""" - return {Reporting, RunTracker, Changed} diff --git a/src/python/pants/init/options_initializer.py b/src/python/pants/init/options_initializer.py index c94c477093f..e56e46a2bd7 100644 --- a/src/python/pants/init/options_initializer.py +++ b/src/python/pants/init/options_initializer.py @@ -12,7 +12,6 @@ from pants.base.exceptions import BuildConfigurationError from pants.build_graph.build_configuration import BuildConfiguration from pants.init.extension_loader import load_backends_and_plugins -from pants.init.global_subsystems import GlobalSubsystems from pants.init.plugin_resolver import PluginResolver from pants.option.global_options import GlobalOptions from pants.option.options_bootstrapper import OptionsBootstrapper @@ -79,9 +78,10 @@ def _construct_options(options_bootstrapper, build_configuration): :returns: An Options object representing the full set of runtime options. """ - optionables = {GlobalOptions, *GlobalSubsystems.get(), *build_configuration.optionables} known_scope_infos = [ - si for optionable in optionables for si in optionable.known_scope_infos() + si + for optionable in build_configuration.all_optionables + for si in optionable.known_scope_infos() ] return options_bootstrapper.get_full_options(known_scope_infos) diff --git a/src/python/pants/option/scope.py b/src/python/pants/option/scope.py index 82b6c7f123a..8ce39213f22 100644 --- a/src/python/pants/option/scope.py +++ b/src/python/pants/option/scope.py @@ -10,6 +10,10 @@ GLOBAL_SCOPE_CONFIG_SECTION = "GLOBAL" +def normalize_scope(scope: str): + return scope.lower().replace("-", "_") + + @dataclass(frozen=True) class Scope: """An options scope.""" diff --git a/tests/python/pants_test/init/test_extension_loader.py b/tests/python/pants_test/init/test_extension_loader.py index 521ad9bb6ac..7bdfa3322cf 100644 --- a/tests/python/pants_test/init/test_extension_loader.py +++ b/tests/python/pants_test/init/test_extension_loader.py @@ -108,8 +108,6 @@ def setUp(self): def create_register( self, build_file_aliases=None, - register_goals=None, - global_subsystems=None, rules=None, target_types=None, module_name="register", @@ -131,8 +129,6 @@ def register_entrypoint(function_name, function): setattr(register_module, function_name, function) register_entrypoint("build_file_aliases", build_file_aliases) - register_entrypoint("global_subsystems", global_subsystems) - register_entrypoint("register_goals", register_goals) register_entrypoint("rules", rules) register_entrypoint("target_types", target_types)