Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tag filtering with --changed-dependees #15546

Merged
merged 1 commit into from
May 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/python/pants/engine/internals/mapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,3 +190,6 @@ def matches(self, target: Target) -> bool:
"""Check that the target matches the provided `--tag` and `--exclude-target-regexp`
options."""
return self._tag_filter(target) and not self._is_excluded_by_pattern(target.address)

def __bool__(self) -> bool:
return bool(self.tags or self.exclude_target_regexps)
36 changes: 31 additions & 5 deletions src/python/pants/vcs/changed.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
from pants.backend.project_info import dependees
from pants.backend.project_info.dependees import Dependees, DependeesRequest
from pants.base.build_environment import get_buildroot
from pants.engine.addresses import Address
from pants.engine.addresses import Address, Addresses
from pants.engine.collection import Collection
from pants.engine.internals.graph import Owners, OwnersRequest
from pants.engine.internals.mapper import SpecsFilter
from pants.engine.rules import Get, collect_rules, rule
from pants.engine.target import UnexpandedTargets
from pants.option.option_types import EnumOption, StrOption
from pants.option.option_value_container import OptionValueContainer
from pants.option.subsystem import Subsystem
Expand All @@ -40,9 +42,21 @@ class ChangedAddresses(Collection[Address]):


@rule
async def find_changed_owners(request: ChangedRequest) -> ChangedAddresses:
owners = await Get(Owners, OwnersRequest(request.sources, filter_by_global_options=True))
if request.dependees == DependeesOption.NONE:
async def find_changed_owners(
request: ChangedRequest, specs_filter: SpecsFilter
) -> ChangedAddresses:
no_dependees = request.dependees == DependeesOption.NONE
owners = await Get(
Owners,
OwnersRequest(
request.sources,
# If `--changed-dependees` is used, we cannot eagerly filter out root targets. We
# need to first find their dependees, and only then should we filter. See
# https://github.com/pantsbuild/pants/issues/15544
filter_by_global_options=no_dependees,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be using bool(specs_filter) instead, in order to ensure that regexes are applied as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regexes will be applied via the last part of this rule -- no_dependees only determines whether we eagerly filter with the roots, or defer the filtering until after dependees have been found

),
)
if no_dependees:
return ChangedAddresses(owners)

# See https://github.com/pantsbuild/pants/issues/15313. We filter out target generators because
Expand All @@ -63,7 +77,19 @@ async def find_changed_owners(request: ChangedRequest) -> ChangedAddresses:
include_roots=False,
),
)
return ChangedAddresses(FrozenOrderedSet(owners) | (dependees - owner_target_generators))
result = FrozenOrderedSet(owners) | (dependees - owner_target_generators)
if specs_filter:
# Finally, we must now filter out the result to only include what matches our tags, as the
# last step of https://github.com/pantsbuild/pants/issues/15544.
#
# Note that we use `UnexpandedTargets` rather than `Targets` or `FilteredTargets` so that
# we preserve target generators.
result_as_tgts = await Get(UnexpandedTargets, Addresses(result))
result = FrozenOrderedSet(
tgt.address for tgt in result_as_tgts if specs_filter.matches(tgt)
)

return ChangedAddresses(result)


@dataclass(frozen=True)
Expand Down
17 changes: 17 additions & 0 deletions src/python/pants/vcs/changed_integration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ def test_delete_generated_target(repo: str) -> None:
for dependees in DependeesOption:
assert_list_stdout(repo, ["//:lib"], dependees=dependees)

# Make sure that our fix for https://github.com/pantsbuild/pants/issues/15544 does not break
# this test when using `--tag`.
for dependees in (DependeesOption.NONE, DependeesOption.TRANSITIVE):
assert_list_stdout(repo, ["//:lib"], dependees=dependees, extra_args=["--tag=a"])

# If we also edit a sibling generated target, we should still (for now at least) include the
# target generator.
append_to_file(repo, "app.sh", "# foo")
Expand Down Expand Up @@ -208,3 +213,15 @@ def test_tag_filtering(repo: str) -> None:
dependees=DependeesOption.TRANSITIVE,
extra_args=["--tag=-b"],
)

# Regression test for https://github.com/pantsbuild/pants/issues/15544. Don't filter
# `--changed-since` roots until the very end if using `--changed-dependees`.
#
# We change `dep.sh`, which has the tag `b`. When we filter for only tag `a`, we should still
# find the dependees of `dep.sh`, like `app.sh`, and only then apply the filter.
reset_edits()
append_to_file(repo, "dep.sh", "# foo")
assert_list_stdout(repo, [], dependees=DependeesOption.NONE, extra_args=["--tag=a"])
assert_list_stdout(
repo, ["//app.sh:lib"], dependees=DependeesOption.TRANSITIVE, extra_args=["--tag=a"]
)