Skip to content

Commit

Permalink
Replace the "base target" concept with "BUILD targets" (#11136)
Browse files Browse the repository at this point in the history
### 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]
  • Loading branch information
stuhood authored Nov 12, 2020
1 parent e179d9c commit c9972d2
Show file tree
Hide file tree
Showing 12 changed files with 77 additions and 81 deletions.
4 changes: 2 additions & 2 deletions src/python/pants/backend/project_info/filter_targets.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/goals/pytest_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
15 changes: 6 additions & 9 deletions src/python/pants/build_graph/address.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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)

Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/build_graph/address_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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"))
Expand Down
12 changes: 6 additions & 6 deletions src/python/pants/engine/internals/build_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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}"
)
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/engine/internals/build_files_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"))


Expand Down Expand Up @@ -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,
[
Expand Down
56 changes: 28 additions & 28 deletions src/python/pants/engine/internals/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand All @@ -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
Expand All @@ -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)

Expand All @@ -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)
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
12 changes: 6 additions & 6 deletions src/python/pants/engine/internals/graph_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'])")
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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'])")
Expand Down
Loading

0 comments on commit c9972d2

Please sign in to comment.