From c9972d26bf34467637fde9b13c33d6cf5f4acf77 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Wed, 11 Nov 2020 17:14:04 -0800 Subject: [PATCH] Replace the "base target" concept with "BUILD targets" (#11136) ### Problem In #11126, there was rough consensus that the "base" target concept would be more easily explained as a "`BUILD` target" (a target declared in a `BUILD` file). ### Solution Rename base target to `BUILD` target in APIs, variables, and docstrings. [ci skip-rust] [ci skip-build-wheels] --- .../backend/project_info/filter_targets.py | 4 +- .../dependency_inference/module_mapper.py | 3 +- .../backend/python/goals/pytest_runner.py | 2 +- .../pants/backend/python/target_types.py | 2 +- src/python/pants/build_graph/address.py | 15 ++--- src/python/pants/build_graph/address_test.py | 6 +- .../pants/engine/internals/build_files.py | 12 ++-- .../engine/internals/build_files_test.py | 4 +- src/python/pants/engine/internals/graph.py | 56 +++++++++---------- .../pants/engine/internals/graph_test.py | 12 ++-- src/python/pants/engine/target.py | 40 ++++++------- src/python/pants/engine/target_test.py | 2 +- 12 files changed, 77 insertions(+), 81 deletions(-) diff --git a/src/python/pants/backend/project_info/filter_targets.py b/src/python/pants/backend/project_info/filter_targets.py index 08a4b8c97da..b07147ed0a6 100644 --- a/src/python/pants/backend/project_info/filter_targets.py +++ b/src/python/pants/backend/project_info/filter_targets.py @@ -110,8 +110,8 @@ def filter_granularity(granularity: TargetGranularity) -> TargetFilter: granularity, { TargetGranularity.all_targets: lambda _: True, - TargetGranularity.file_targets: lambda tgt: not tgt.address.is_base_target, - TargetGranularity.build_targets: lambda tgt: tgt.address.is_base_target, + TargetGranularity.file_targets: lambda tgt: tgt.address.is_file_target, + TargetGranularity.build_targets: lambda tgt: not tgt.address.is_file_target, }, ) diff --git a/src/python/pants/backend/python/dependency_inference/module_mapper.py b/src/python/pants/backend/python/dependency_inference/module_mapper.py index 297876b7e1e..990664a0cc8 100644 --- a/src/python/pants/backend/python/dependency_inference/module_mapper.py +++ b/src/python/pants/backend/python/dependency_inference/module_mapper.py @@ -41,8 +41,7 @@ class FirstPartyModuleToAddressMapping: """A mapping of module names to owning addresses. All mapped addresses will be file addresses, aka generated subtargets. That is, each target - will own no more than one single source file. Its metadata will be copied from the original - base target. + will own no more than one single source file. If there are >1 original owning targets that refer to the same module—such as `//:a` and `//:b` both owning module `foo`—then we will not add any of the targets to the mapping because there is ambiguity. (We make an exception if diff --git a/src/python/pants/backend/python/goals/pytest_runner.py b/src/python/pants/backend/python/goals/pytest_runner.py index 3a9b447dff1..4c57ddd5ae3 100644 --- a/src/python/pants/backend/python/goals/pytest_runner.py +++ b/src/python/pants/backend/python/goals/pytest_runner.py @@ -69,7 +69,7 @@ class PythonTestFieldSet(TestFieldSet): def is_conftest_or_type_stub(self) -> bool: """We skip both `conftest.py` and `.pyi` stubs, even though though they often belong to a `python_tests` target, because neither contain any tests to run on.""" - if self.address.is_base_target: + if not self.address.is_file_target: return False file_name = PurePath(self.address.filename) return file_name.name == "conftest.py" or file_name.suffix == ".pyi" diff --git a/src/python/pants/backend/python/target_types.py b/src/python/pants/backend/python/target_types.py index e8fcedb35ae..7ff7800e97c 100644 --- a/src/python/pants/backend/python/target_types.py +++ b/src/python/pants/backend/python/target_types.py @@ -244,7 +244,7 @@ class PexPlatformsField(StringOrStringSequenceField): def compute_value( cls, raw_value: Optional[Iterable[str]], *, address: Address ) -> Optional[Tuple[str, ...]]: - if isinstance(raw_value, str) and address.is_base_target: + if isinstance(raw_value, str) and not address.is_file_target: warn_or_error( deprecated_entity_description=f"Using a bare string for the `{cls.alias}` field", removal_version="2.2.0.dev0", diff --git a/src/python/pants/build_graph/address.py b/src/python/pants/build_graph/address.py index 7353493fd0c..ad635210513 100644 --- a/src/python/pants/build_graph/address.py +++ b/src/python/pants/build_graph/address.py @@ -265,8 +265,8 @@ def __init__( ) @property - def is_base_target(self) -> bool: - return self._relative_file_path is None + def is_file_target(self) -> bool: + return self._relative_file_path is not None @property def is_default_target(self) -> bool: @@ -279,7 +279,7 @@ def is_default_target(self) -> bool: @property def filename(self) -> str: if self._relative_file_path is None: - raise ValueError("Only a file Address (`not self.is_base_target`) has a filename.") + raise ValueError("Only a file Address (`self.is_file_target`) has a filename.") return os.path.join(self.spec_path, self._relative_file_path) @property @@ -330,15 +330,12 @@ def path_safe_spec(self) -> str: target_portion = f"{parent_prefix}{target_name}" return f"{self.spec_path.replace(os.path.sep, '.')}{file_portion}{target_portion}" - def maybe_convert_to_base_target(self) -> "Address": - """If this address is a generated subtarget, convert it back into its original base target. + def maybe_convert_to_build_target(self) -> "Address": + """If this address is for a file target, convert it back into its BUILD target. Otherwise, return itself unmodified. - - TODO: This is not correct: we don't know the owning BUILD file of the base target without - resolving. But it's possible that this method can be removed. """ - if self.is_base_target: + if not self.is_file_target: return self return self.__class__(self.spec_path, relative_file_path=None, target_name=self.target_name) diff --git a/src/python/pants/build_graph/address_test.py b/src/python/pants/build_graph/address_test.py index df991cda1dd..abdae779118 100644 --- a/src/python/pants/build_graph/address_test.py +++ b/src/python/pants/build_graph/address_test.py @@ -280,9 +280,9 @@ def assert_spec(address: Address, *, expected: str, expected_path_spec: str) -> ) -def test_address_maybe_convert_to_base_target() -> None: +def test_address_maybe_convert_to_build_target() -> None: def assert_converts_to_base_target(generated_addr: Address, *, expected: Address) -> None: - assert generated_addr.maybe_convert_to_base_target() == expected + assert generated_addr.maybe_convert_to_build_target() == expected assert_converts_to_base_target( Address("a/b", relative_file_path="c.txt", target_name="c"), @@ -300,7 +300,7 @@ def assert_converts_to_base_target(generated_addr: Address, *, expected: Address ) def assert_base_target_noops(addr: Address) -> None: - assert addr.maybe_convert_to_base_target() is addr + assert addr.maybe_convert_to_build_target() is addr assert_base_target_noops(Address("a/b", target_name="c")) assert_base_target_noops(Address("a/b")) diff --git a/src/python/pants/engine/internals/build_files.py b/src/python/pants/engine/internals/build_files.py index 0905906e04f..01f14352efd 100644 --- a/src/python/pants/engine/internals/build_files.py +++ b/src/python/pants/engine/internals/build_files.py @@ -115,7 +115,7 @@ async def parse_address_family( @rule async def find_build_file(address: Address) -> BuildFileAddress: address_family = await Get(AddressFamily, AddressFamilyDir(address.spec_path)) - owning_address = address.maybe_convert_to_base_target() + owning_address = address.maybe_convert_to_build_target() if address_family.get_target_adaptor(owning_address) is None: raise ResolveError.did_you_mean( bad_name=owning_address.target_name, @@ -128,14 +128,14 @@ async def find_build_file(address: Address) -> BuildFileAddress: if build_file_address.address == owning_address ) return ( - bfa if address.is_base_target else BuildFileAddress(rel_path=bfa.rel_path, address=address) + BuildFileAddress(rel_path=bfa.rel_path, address=address) if address.is_file_target else bfa ) @rule async def find_target_adaptor(address: Address) -> TargetAdaptor: """Hydrate a TargetAdaptor so that it may be converted into the Target API.""" - if not address.is_base_target: + if address.is_file_target: raise ValueError( f"Subtargets are not resident in BUILD files, and so do not have TargetAdaptors: {address}" ) @@ -170,13 +170,13 @@ async def addresses_from_address_specs( for spec in address_specs.literals ) literal_target_adaptors = await MultiGet( - Get(TargetAdaptor, Address, addr.maybe_convert_to_base_target()) + Get(TargetAdaptor, Address, addr.maybe_convert_to_build_target()) for addr in literal_addresses ) # We convert to targets for the side effect of validating that any file addresses actually - # belong to the specified base targets. + # belong to the specified BUILD targets. await Get( - UnexpandedTargets, Addresses(addr for addr in literal_addresses if not addr.is_base_target) + UnexpandedTargets, Addresses(addr for addr in literal_addresses if addr.is_file_target) ) for literal_spec, addr, target_adaptor in zip( address_specs.literals, literal_addresses, literal_target_adaptors diff --git a/src/python/pants/engine/internals/build_files_test.py b/src/python/pants/engine/internals/build_files_test.py index 6c887b4451e..8ff2164c16b 100644 --- a/src/python/pants/engine/internals/build_files_test.py +++ b/src/python/pants/engine/internals/build_files_test.py @@ -201,7 +201,7 @@ def assert_bfa_resolved(address: Address) -> None: assert bfa == expected_bfa assert_bfa_resolved(Address("helloworld")) - # File addresses should use their base target to find the BUILD file. + # File addresses should use their BUILD target to find the BUILD file. assert_bfa_resolved(Address("helloworld", relative_file_path="f.txt")) @@ -257,7 +257,7 @@ def test_address_specs_filter_by_tag(address_specs_rule_runner: RuleRunner) -> N } # The same filtering should work when given literal addresses, including file addresses. - # For file addresses, we look up the `tags` field of the original base target. + # For file addresses, we look up the `tags` field of the original BUILD target. literals_result = resolve_address_specs( address_specs_rule_runner, [ diff --git a/src/python/pants/engine/internals/graph.py b/src/python/pants/engine/internals/graph.py index 803f197f159..fa97322be09 100644 --- a/src/python/pants/engine/internals/graph.py +++ b/src/python/pants/engine/internals/graph.py @@ -90,18 +90,18 @@ async def resolve_unexpanded_targets(addresses: Addresses) -> UnexpandedTargets: @rule async def generate_subtargets(address: Address, global_options: GlobalOptions) -> Subtargets: - if not address.is_base_target: + if address.is_file_target: raise ValueError(f"Cannot generate file Targets for a file Address: {address}") - wrapped_base_target = await Get(WrappedTarget, Address, address) - base_target = wrapped_base_target.target + wrapped_build_target = await Get(WrappedTarget, Address, address) + build_target = wrapped_build_target.target - if not base_target.has_field(Dependencies) or not base_target.has_field(Sources): + if not build_target.has_field(Dependencies) or not build_target.has_field(Sources): # If a target type does not support dependencies, we do not split it, as that would prevent - # the base target from depending on its splits. - return Subtargets(base_target, ()) + # the BUILD target from depending on its splits. + return Subtargets(build_target, ()) # Create subtargets for matched sources. - sources_field = base_target[Sources] + sources_field = build_target[Sources] sources_field_path_globs = sources_field.path_globs( global_options.options.files_not_found_behavior ) @@ -118,7 +118,7 @@ async def generate_subtargets(address: Address, global_options: GlobalOptions) - for subtarget_file in paths.files ) - return Subtargets(base_target, tuple(wt.target for wt in wrapped_subtargets)) + return Subtargets(build_target, tuple(wt.target for wt in wrapped_subtargets)) @rule @@ -127,10 +127,10 @@ async def resolve_target( registered_target_types: RegisteredTargetTypes, union_membership: UnionMembership, ) -> WrappedTarget: - if not address.is_base_target: - base_target = await Get(WrappedTarget, Address, address.maybe_convert_to_base_target()) + if address.is_file_target: + build_target = await Get(WrappedTarget, Address, address.maybe_convert_to_build_target()) subtarget = generate_subtarget( - base_target.target, full_file_name=address.filename, union_membership=union_membership + build_target.target, full_file_name=address.filename, union_membership=union_membership ) return WrappedTarget(subtarget) @@ -146,24 +146,24 @@ async def resolve_target( @rule async def resolve_targets(targets: UnexpandedTargets) -> Targets: - # Split out and expand any base targets. + # Split out and expand any BUILD targets. other_targets = [] - base_targets = [] + build_targets = [] for target in targets: - if target.address.is_base_target: - base_targets.append(target) + if not target.address.is_file_target: + build_targets.append(target) else: other_targets.append(target) - base_targets_subtargets = await MultiGet( - Get(Subtargets, Address, bt.address) for bt in base_targets + build_targets_subtargets = await MultiGet( + Get(Subtargets, Address, bt.address) for bt in build_targets ) - # Zip the subtargets back to the base targets and replace them. - # NB: If a target had no subtargets, we use the base. + # Zip the subtargets back to the BUILD targets and replace them. + # NB: If a target had no subtargets, we use the original. expanded_targets = OrderedSet(other_targets) expanded_targets.update( target - for subtargets in base_targets_subtargets + for subtargets in build_targets_subtargets for target in (subtargets.subtargets if subtargets.subtargets else (subtargets.base,)) ) return Targets(expanded_targets) @@ -200,14 +200,14 @@ def _detect_cycles( def maybe_report_cycle(address: Address) -> None: # NB: File-level dependencies are cycle tolerant. - if not address.is_base_target or address not in path_stack: + if address.is_file_target or address not in path_stack: return # The path of the cycle is shorter than the entire path to the cycle: if the suffix of # the path representing the cycle contains a file dep, it is ignored. in_cycle = False for path_address in path_stack: - if in_cycle and not path_address.is_base_target: + if in_cycle and path_address.is_file_target: # There is a file address inside the cycle: do not report it. return elif in_cycle: @@ -791,16 +791,16 @@ async def resolve_dependencies( for inference_request_type in relevant_inference_request_types ) - # If this is a base target, or no dependency inference implementation can infer dependencies on - # a file address's sibling files, then we inject dependencies on all the base target's + # If this is a BUILD target, or no dependency inference implementation can infer dependencies on + # a file address's sibling files, then we inject dependencies on all the BUILD target's # generated subtargets. subtarget_addresses: Tuple[Address, ...] = () no_sibling_file_deps_inferrable = not inferred or all( inferred_deps.sibling_dependencies_inferrable is False for inferred_deps in inferred ) - if request.field.address.is_base_target or no_sibling_file_deps_inferrable: + if not request.field.address.is_file_target or no_sibling_file_deps_inferrable: subtargets = await Get( - Subtargets, Address, request.field.address.maybe_convert_to_base_target() + Subtargets, Address, request.field.address.maybe_convert_to_build_target() ) subtarget_addresses = tuple( t.address for t in subtargets.subtargets if t.address != request.field.address @@ -875,9 +875,9 @@ async def resolve_dependencies_lite( if isinstance(request.field, inject_request_type.inject_for) ) - # Inject dependencies on all the base target's generated subtargets. + # Inject dependencies on all the BUILD target's generated file targets. subtargets = await Get( - Subtargets, Address, request.field.address.maybe_convert_to_base_target() + Subtargets, Address, request.field.address.maybe_convert_to_build_target() ) subtarget_addresses = tuple( t.address for t in subtargets.subtargets if t.address != request.field.address diff --git a/src/python/pants/engine/internals/graph_test.py b/src/python/pants/engine/internals/graph_test.py index eedfd48e1f6..c0ab3a0c08e 100644 --- a/src/python/pants/engine/internals/graph_test.py +++ b/src/python/pants/engine/internals/graph_test.py @@ -627,9 +627,9 @@ def test_resolve_addresses_from_specs() -> None: rule_runner.add_to_build_file("address_spec", "target(sources=['f.txt'])") no_interaction_specs = ["fs_spec/f.txt", "address_spec:address_spec"] - # If a generated subtarget's original base target is included via an address spec, - # we will still include the generated subtarget for consistency. When we expand Targets - # into their base targets this redundancy is removed, but during Address expansion we + # If a file target's original BUILD target is included via an address spec, + # we will still include the file target for consistency. When we expand Targets + # into their BUILD targets this redundancy is removed, but during Address expansion we # get literal matches. rule_runner.create_files("multiple_files", ["f1.txt", "f2.txt"]) rule_runner.add_to_build_file("multiple_files", "target(sources=['*.txt'])") @@ -1310,7 +1310,7 @@ def test_dependency_inference(dependencies_rule_runner: RuleRunner) -> None: """We test that dependency inference works generally and that we merge it correctly with explicitly provided dependencies. - For consistency, dep inference does not merge generated subtargets with base targets: if both + For consistency, dep inference does not merge generated subtargets with BUILD targets: if both are inferred, expansion to Targets will remove the redundancy while converting to subtargets. """ dependencies_rule_runner.create_files( @@ -1419,8 +1419,8 @@ def test_dependency_inference(dependencies_rule_runner: RuleRunner) -> None: def test_depends_on_subtargets(dependencies_rule_runner: RuleRunner) -> None: - """If the address is a base target, or none of the dependency inference rules can infer - dependencies on sibling files, then we should depend on all the base target's subtargets.""" + """If the address is a BUILD target, or none of the dependency inference rules can infer + dependencies on sibling files, then we should depend on all the BUILD target's files.""" dependencies_rule_runner.create_file("src/smalltalk/f1.st") dependencies_rule_runner.create_file("src/smalltalk/f2.st") dependencies_rule_runner.add_to_build_file("src/smalltalk", "smalltalk(sources=['*.st'])") diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index a504c044592..eaaf22bf159 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -64,7 +64,7 @@ class Field(ABC): # NB: We still expect `PrimitiveField` and `AsyncField` to define their own constructors. This # is only implemented so that we have a common deprecation mechanism. def __init__(self, raw_value: Optional[Any], *, address: Address) -> None: - if self.deprecated_removal_version and address.is_base_target and raw_value is not None: + if self.deprecated_removal_version and not address.is_file_target and raw_value is not None: if not self.deprecated_removal_hint: raise ValueError( f"You specified `deprecated_removal_version` for {self.__class__}, but not " @@ -290,7 +290,7 @@ def __init__( # rarely directly instantiate Targets and should instead use the engine to request them. union_membership: Optional[UnionMembership] = None, ) -> None: - if self.deprecated_removal_version and address.is_base_target: + if self.deprecated_removal_version and not address.is_file_target: if not self.deprecated_removal_hint: raise ValueError( f"You specified `deprecated_removal_version` for {self.__class__}, but not " @@ -544,9 +544,9 @@ def register_plugin_field(cls, field: Type[Field]) -> UnionRule: @dataclass(frozen=True) class Subtargets: - # The base target from which the subtargets were extracted. + # The BUILD target from which the subtargets were extracted. base: Target - # The subtargets, one per file that was owned by the base target. + # The subtargets, one per file that was owned by the BUILD target. subtargets: Tuple[Target, ...] @@ -672,21 +672,21 @@ def types(self) -> Tuple[Type[Target], ...]: # ----------------------------------------------------------------------------------------------- -def generate_subtarget_address(base_target_address: Address, *, full_file_name: str) -> Address: - """Return the address for a new target based on the original target, but with a more precise +def generate_subtarget_address(build_target_address: Address, *, full_file_name: str) -> Address: + """Return the address for a new target based on the BUILD target, but with a more precise `sources` field. The address's target name will be the relativized file, such as `:app.json`, or `:subdir/f.txt`. See generate_subtarget(). """ - if not base_target_address.is_base_target: - raise ValueError(f"Cannot generate file targets for a file Address: {base_target_address}") - original_spec_path = base_target_address.spec_path + if build_target_address.is_file_target: + raise ValueError(f"Cannot generate file targets for a file Address: {build_target_address}") + original_spec_path = build_target_address.spec_path relative_file_path = PurePath(full_file_name).relative_to(original_spec_path).as_posix() return Address( spec_path=original_spec_path, - target_name=base_target_address.target_name, + target_name=build_target_address.target_name, relative_file_path=relative_file_path, ) @@ -695,7 +695,7 @@ def generate_subtarget_address(base_target_address: Address, *, full_file_name: def generate_subtarget( - base_target: _Tgt, + build_target: _Tgt, *, full_file_name: str, # NB: `union_membership` is only optional to facilitate tests. In production, we should @@ -703,30 +703,30 @@ def generate_subtarget( # rarely directly instantiate Targets and should instead use the engine to request them. union_membership: Optional[UnionMembership] = None, ) -> _Tgt: - """Generate a new target with the exact same metadata as the original, except for the `sources` - field only referring to the single file `full_file_name` and with a new address. + """Generate a new target with the exact same metadata as the BUILD target, except for the + `sources` field only referring to the single file `full_file_name` and with a new address. This is used for greater precision when using dependency inference and file arguments. When we are able to deduce specifically which files are being used, we can use only the files we care about, rather than the entire `sources` field. """ - if not base_target.has_field(Dependencies) or not base_target.has_field(Sources): + if not build_target.has_field(Dependencies) or not build_target.has_field(Sources): raise ValueError( - f"Target {base_target.address.spec} of type {type(base_target).__qualname__} does " + f"Target {build_target.address.spec} of type {type(build_target).__qualname__} does " "not have both a `dependencies` and `sources` field, and thus cannot generate a " f"subtarget for the file {full_file_name}." ) relativized_file_name = ( - PurePath(full_file_name).relative_to(base_target.address.spec_path).as_posix() + PurePath(full_file_name).relative_to(build_target.address.spec_path).as_posix() ) generated_target_fields = {} - for field in base_target.field_values.values(): + for field in build_target.field_values.values(): if isinstance(field, Sources): if not bool(matches_filespec(field.filespec, paths=[full_file_name])): raise ValueError( - f"Target {base_target.address.spec}'s `sources` field does not match a file " + f"Target {build_target.address.spec}'s `sources` field does not match a file " f"{full_file_name}." ) value = (relativized_file_name,) @@ -738,10 +738,10 @@ def generate_subtarget( ) generated_target_fields[field.alias] = value - target_cls = type(base_target) + target_cls = type(build_target) return target_cls( generated_target_fields, - address=generate_subtarget_address(base_target.address, full_file_name=full_file_name), + address=generate_subtarget_address(build_target.address, full_file_name=full_file_name), union_membership=union_membership, ) diff --git a/src/python/pants/engine/target_test.py b/src/python/pants/engine/target_test.py index 3f584901415..5be78f7e482 100644 --- a/src/python/pants/engine/target_test.py +++ b/src/python/pants/engine/target_test.py @@ -469,7 +469,7 @@ class MockTarget(Target): == expected_subdir_address ) - # The full_file_name must match the filespec of the base target's Sources field. + # The full_file_name must match the filespec of the BUILD target's Sources field. with pytest.raises(ValueError) as exc: generate_subtarget(single_source_tgt, full_file_name="src/fortran/fake_file.f95") assert "does not match a file src/fortran/fake_file.f95" in str(exc.value)