Skip to content

Commit

Permalink
Fix that running on a BUILD file expands to all of its targets (pants…
Browse files Browse the repository at this point in the history
…build#16546)

## Problem

@thejcannon is moving formatting of BUILD files from `update-build-files` to `fmt`. 

But there's a weird edge: `./pants fmt BUILD` often formats much more than `BUILD` because it runs on all the defined targets. Josh and I both believe this is not intuitive - a user would expect only `BUILD` to be changed.

Before:

```
❯ ./pants --cli-build-files-expand-to-targets list BUILD
//BUILD_ROOT:files
//cargo:scripts
//conftest.py:test_utils
//pants:scripts
//pants.toml:files
```

After:

```
❯ ./pants --no-cli-build-files-expand-to-targets list BUILD
15:12:08.84 [WARN] No targets were matched in goal `list`.
```

While this is technically a user API change, we believe the old behavior was buggy and not intentional. It also was not documented. So we fix this as a bug fix, without using a deprecation.

### `--changed-since`

It's important that `--changed-since` does expand BUILD files to their targets, as changing a BUILD file might result in any of the targets changing too. This preserves that behavior, regardless of `--cli-build-files-expand-to-targets `.
  • Loading branch information
Eric-Arellano authored Aug 17, 2022
1 parent 1551fe8 commit 35866ff
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 4 deletions.
6 changes: 5 additions & 1 deletion src/python/pants/engine/internals/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,7 @@ class OwnersRequest:
sources: tuple[str, ...]
owners_not_found_behavior: OwnersNotFoundBehavior = OwnersNotFoundBehavior.ignore
filter_by_global_options: bool = False
match_if_owning_build_file_included_in_sources: bool = False


class Owners(Collection[Address]):
Expand Down Expand Up @@ -799,7 +800,10 @@ def create_live_and_deleted_gets(
matching_files.update(
matches_filespec(secondary_owner_field.filespec, paths=sources_set)
)
if not matching_files and bfa.rel_path not in sources_set:
if not matching_files and not (
owners_request.match_if_owning_build_file_included_in_sources
and bfa.rel_path in sources_set
):
continue

unmatched_sources -= matching_files
Expand Down
23 changes: 21 additions & 2 deletions src/python/pants/engine/internals/graph_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -708,9 +708,21 @@ def owners_rule_runner() -> RuleRunner:


def assert_owners(
rule_runner: RuleRunner, requested: Iterable[str], *, expected: Set[Address]
rule_runner: RuleRunner,
requested: Iterable[str],
*,
expected: Set[Address],
match_if_owning_build_file_included_in_sources: bool = False,
) -> None:
result = rule_runner.request(Owners, [OwnersRequest(tuple(requested))])
result = rule_runner.request(
Owners,
[
OwnersRequest(
tuple(requested),
match_if_owning_build_file_included_in_sources=match_if_owning_build_file_included_in_sources,
)
],
)
assert set(result) == expected


Expand Down Expand Up @@ -823,6 +835,13 @@ def test_owners_build_file(owners_rule_runner: RuleRunner) -> None:
assert_owners(
owners_rule_runner,
["demo/BUILD"],
match_if_owning_build_file_included_in_sources=False,
expected=set(),
)
assert_owners(
owners_rule_runner,
["demo/BUILD"],
match_if_owning_build_file_included_in_sources=True,
expected={
Address("demo", target_name="f1"),
Address("demo", target_name="f2_first"),
Expand Down
7 changes: 6 additions & 1 deletion src/python/pants/engine/internals/specs_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,12 @@ async def addresses_from_raw_specs_with_only_file_owners(
all_files = tuple(itertools.chain.from_iterable(paths.files for paths in paths_per_include))
owners = await Get(
Owners,
OwnersRequest(all_files, filter_by_global_options=specs.filter_by_global_options),
OwnersRequest(
all_files,
filter_by_global_options=specs.filter_by_global_options,
# Specifying a BUILD file should not expand to all the targets it defines.
match_if_owning_build_file_included_in_sources=False,
),
)
return Addresses(sorted(owners))

Expand Down
2 changes: 2 additions & 0 deletions src/python/pants/vcs/changed.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ async def find_changed_owners(
# 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,
# Changing a BUILD file might impact the targets it defines.
match_if_owning_build_file_included_in_sources=True,
),
)
if no_dependees:
Expand Down

0 comments on commit 35866ff

Please sign in to comment.