Skip to content

Commit

Permalink
Correctly describe the origin of invalid specs, e.g. --paths-from (#…
Browse files Browse the repository at this point in the history
…15730)

Before:

```
❯ ./pants paths --from=fake/f.py --to=src/python/pants/util/strutil.py
12:54:23.99 [ERROR] 1 Exception encountered:

  Exception: Unmatched glob from CLI arguments
```

After:

```
❯ ./pants paths --from=fake/f.py --to=src/python/pants/util/strutil.py
12:54:23.99 [ERROR] 1 Exception encountered:

  Exception: Unmatched glob from the option `--paths-from`: "fake/f.py"
```

It was a lie to hardcode "CLI arguments". 

Most of these rules should never error because we only error if the directory of the CLI specs do not exist, and we programmatically compute the specs based on already existing targets. But still, this gives us much better debugging if those assumptions are violated.

This also unblocks us from #14468. The `Specs` will be able to pass down their `description_of_origin` to `AddressInput` for much better errors with invalid `AddressLiteralSpec`s.

[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano authored Jun 3, 2022
1 parent d8ddde3 commit 4171322
Show file tree
Hide file tree
Showing 24 changed files with 239 additions and 117 deletions.
5 changes: 4 additions & 1 deletion src/python/pants/backend/go/goals/tailor.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,10 @@ async def find_putative_go_targets(
]
existing_targets = await Get(
UnexpandedTargets,
RawSpecs(ancestor_globs=tuple(AncestorGlobSpec(d) for d in main_package_dirs)),
RawSpecs(
ancestor_globs=tuple(AncestorGlobSpec(d) for d in main_package_dirs),
description_of_origin="the `go_binary` tailor rule",
),
)
owned_main_packages = await MultiGet(
Get(GoBinaryMainPackage, GoBinaryMainPackageRequest(t[GoBinaryMainPackageField]))
Expand Down
8 changes: 7 additions & 1 deletion src/python/pants/backend/go/target_type_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,13 @@ async def determine_main_pkg_for_go_binary(
)
return GoBinaryMainPackage(wrapped_specified_tgt.target.address)

candidate_targets = await Get(Targets, RawSpecs(dir_globs=(DirGlobSpec(addr.spec_path),)))
candidate_targets = await Get(
Targets,
RawSpecs(
dir_globs=(DirGlobSpec(addr.spec_path),),
description_of_origin="the `go_binary` dependency inference rule",
),
)
relevant_pkg_targets = [
tgt
for tgt in candidate_targets
Expand Down
6 changes: 5 additions & 1 deletion src/python/pants/backend/go/util_rules/go_mod.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ class OwningGoMod:
async def find_nearest_go_mod(request: OwningGoModRequest) -> OwningGoMod:
# We don't expect `go_mod` targets to be generated, so we can use UnexpandedTargets.
candidate_targets = await Get(
UnexpandedTargets, RawSpecs(ancestor_globs=(AncestorGlobSpec(request.address.spec_path),))
UnexpandedTargets,
RawSpecs(
ancestor_globs=(AncestorGlobSpec(request.address.spec_path),),
description_of_origin="the `OwningGoMod` rule",
),
)

# Sort by address.spec_path in descending order so the nearest go_mod target is sorted first.
Expand Down
8 changes: 6 additions & 2 deletions src/python/pants/backend/project_info/paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,18 @@ async def paths(
Targets,
Specs,
specs_parser.parse_specs(
[path_from], convert_dir_literal_to_address_literal=convert_dir_literals
[path_from],
description_of_origin="the option `--paths-from`",
convert_dir_literal_to_address_literal=convert_dir_literals,
),
),
Get(
Targets,
Specs,
specs_parser.parse_specs(
[path_to], convert_dir_literal_to_address_literal=convert_dir_literals
[path_to],
description_of_origin="the option `--paths-to`",
convert_dir_literal_to_address_literal=convert_dir_literals,
),
),
)
Expand Down
5 changes: 4 additions & 1 deletion src/python/pants/backend/project_info/peek_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,10 @@ def test_get_target_data(rule_runner: RuleRunner) -> None:
"foo/b.txt": "",
}
)
tds = rule_runner.request(TargetDatas, [RawSpecs(recursive_globs=(RecursiveGlobSpec("foo"),))])
tds = rule_runner.request(
TargetDatas,
[RawSpecs(recursive_globs=(RecursiveGlobSpec("foo"),), description_of_origin="tests")],
)
assert list(tds) == [
TargetData(
GenericTarget({"dependencies": [":baz"]}, Address("foo", target_name="bar")),
Expand Down
7 changes: 6 additions & 1 deletion src/python/pants/backend/python/goals/export_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,12 @@ def run(enable_resolves: bool) -> ExportResults:
env_inherit={"PATH", "PYENV_ROOT"},
)
targets = rule_runner.request(
Targets, [RawSpecs(recursive_globs=(RecursiveGlobSpec("src/foo"),))]
Targets,
[
RawSpecs(
recursive_globs=(RecursiveGlobSpec("src/foo"),), description_of_origin="tests"
)
],
)
all_results = rule_runner.request(ExportResults, [ExportVenvsRequest(targets)])

Expand Down
8 changes: 7 additions & 1 deletion src/python/pants/backend/python/goals/setup_py.py
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +930,13 @@ async def get_exporting_owner(owned_dependency: OwnedDependency) -> ExportedTarg
"""
target = owned_dependency.target
ancestor_addrs = AncestorGlobSpec(target.address.spec_path)
ancestor_tgts = await Get(Targets, RawSpecs(ancestor_globs=(ancestor_addrs,)))
ancestor_tgts = await Get(
Targets,
RawSpecs(
ancestor_globs=(ancestor_addrs,),
description_of_origin="the `python_distribution` `package` rules",
),
)
# Note that addresses sort by (spec_path, target_name), and all these targets are
# ancestors of the given target, i.e., their spec_paths are all prefixes. So sorting by
# address will effectively sort by closeness of ancestry to the given target.
Expand Down
5 changes: 4 additions & 1 deletion src/python/pants/backend/python/goals/tailor.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,10 @@ def add_req_targets(files: Iterable[str], alias: str, target_name: str) -> None:
entry_point_dirs = {os.path.dirname(entry_point) for entry_point in entry_points}
possible_existing_binary_targets = await Get(
UnexpandedTargets,
RawSpecs(ancestor_globs=tuple(AncestorGlobSpec(d) for d in entry_point_dirs)),
RawSpecs(
ancestor_globs=tuple(AncestorGlobSpec(d) for d in entry_point_dirs),
description_of_origin="the `pex_binary` tailor rule",
),
)
possible_existing_binary_entry_points = await MultiGet(
Get(ResolvedPexEntryPoint, ResolvePexEntryPointRequest(t[PexEntryPointField]))
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/backend/terraform/dependency_inference.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ async def infer_terraform_module_dependencies(
RawSpecs(
dir_globs=tuple(DirGlobSpec(path) for path in candidate_spec_paths),
unmatched_glob_behavior=GlobMatchErrorBehavior.ignore,
description_of_origin="the `terraform_module` dependency inference rule",
),
)
# TODO: Need to either implement the standard ambiguous dependency logic or ban >1 terraform_module
Expand Down
71 changes: 51 additions & 20 deletions src/python/pants/base/specs.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,15 +204,20 @@ def matches_target_residence_dir(self, residence_dir: str) -> bool:


def _create_path_globs(
globs: Iterable[str], unmatched_glob_behavior: GlobMatchErrorBehavior
globs: Iterable[str],
unmatched_glob_behavior: GlobMatchErrorBehavior,
*,
description_of_origin: str,
) -> PathGlobs:
return PathGlobs(
globs=globs,
glob_match_error_behavior=unmatched_glob_behavior,
# We validate that _every_ glob is valid.
conjunction=GlobExpansionConjunction.all_match,
description_of_origin=(
None if unmatched_glob_behavior == GlobMatchErrorBehavior.ignore else "CLI arguments"
None
if unmatched_glob_behavior == GlobMatchErrorBehavior.ignore
else description_of_origin
),
)

Expand All @@ -228,6 +233,8 @@ class RawSpecs:
either `Specs` or `RawSpecs` in rules, e.g. to find what targets exist in a directory.
"""

description_of_origin: str

address_literals: tuple[AddressLiteralSpec, ...] = ()
file_literals: tuple[FileLiteralSpec, ...] = ()
file_globs: tuple[FileGlobSpec, ...] = ()
Expand All @@ -245,6 +252,7 @@ def create(
cls,
specs: Iterable[Spec],
*,
description_of_origin: str,
convert_dir_literal_to_address_literal: bool,
unmatched_glob_behavior: GlobMatchErrorBehavior = GlobMatchErrorBehavior.error,
filter_by_global_options: bool = False,
Expand Down Expand Up @@ -283,13 +291,14 @@ def create(
else:
raise AssertionError(f"Unexpected type of Spec: {repr(spec)}")
return RawSpecs(
tuple(address_literals),
tuple(file_literals),
tuple(file_globs),
tuple(dir_literals),
tuple(dir_globs),
tuple(recursive_globs),
tuple(ancestor_globs),
address_literals=tuple(address_literals),
file_literals=tuple(file_literals),
file_globs=tuple(file_globs),
dir_literals=tuple(dir_literals),
dir_globs=tuple(dir_globs),
recursive_globs=tuple(recursive_globs),
ancestor_globs=tuple(ancestor_globs),
description_of_origin=description_of_origin,
unmatched_glob_behavior=unmatched_glob_behavior,
filter_by_global_options=filter_by_global_options,
from_change_detection=from_change_detection,
Expand Down Expand Up @@ -324,6 +333,7 @@ def to_specs_paths_path_globs(self) -> PathGlobs:
if self.from_change_detection
else self.unmatched_glob_behavior
),
description_of_origin=self.description_of_origin,
)


Expand All @@ -335,6 +345,8 @@ class RawSpecsWithoutFileOwners:
`Get(Addresses, RawSpecs)`, which will result in this rule being used.
"""

description_of_origin: str

address_literals: tuple[AddressLiteralSpec, ...] = ()
dir_literals: tuple[DirLiteralSpec, ...] = ()
dir_globs: tuple[DirGlobSpec, ...] = ()
Expand All @@ -347,11 +359,12 @@ class RawSpecsWithoutFileOwners:
@classmethod
def from_raw_specs(cls, specs: RawSpecs) -> RawSpecsWithoutFileOwners:
return RawSpecsWithoutFileOwners(
specs.address_literals,
specs.dir_literals,
specs.dir_globs,
specs.recursive_globs,
specs.ancestor_globs,
address_literals=specs.address_literals,
dir_literals=specs.dir_literals,
dir_globs=specs.dir_globs,
recursive_globs=specs.recursive_globs,
ancestor_globs=specs.ancestor_globs,
description_of_origin=specs.description_of_origin,
unmatched_glob_behavior=specs.unmatched_glob_behavior,
filter_by_global_options=specs.filter_by_global_options,
)
Expand Down Expand Up @@ -402,7 +415,11 @@ def to_build_file_path_globs_tuple(
validation_path_globs = (
PathGlobs(())
if self.unmatched_glob_behavior == GlobMatchErrorBehavior.ignore
else _create_path_globs((*validation_includes, *ignores), self.unmatched_glob_behavior)
else _create_path_globs(
(*validation_includes, *ignores),
self.unmatched_glob_behavior,
description_of_origin=self.description_of_origin,
)
)
return build_path_globs, validation_path_globs

Expand All @@ -415,6 +432,8 @@ class RawSpecsWithOnlyFileOwners:
`Get(Addresses, RawSpecs)`, which will result in this rule being used.
"""

description_of_origin: str

file_literals: tuple[FileLiteralSpec, ...] = ()
file_globs: tuple[FileGlobSpec, ...] = ()

Expand All @@ -425,8 +444,9 @@ class RawSpecsWithOnlyFileOwners:
@classmethod
def from_raw_specs(cls, specs: RawSpecs) -> RawSpecsWithOnlyFileOwners:
return RawSpecsWithOnlyFileOwners(
specs.file_literals,
specs.file_globs,
description_of_origin=specs.description_of_origin,
file_literals=specs.file_literals,
file_globs=specs.file_globs,
unmatched_glob_behavior=specs.unmatched_glob_behavior,
filter_by_global_options=specs.filter_by_global_options,
from_change_detection=specs.from_change_detection,
Expand All @@ -439,7 +459,11 @@ def path_globs_for_spec(self, spec: FileLiteralSpec | FileGlobSpec) -> PathGlobs
if self.from_change_detection
else self.unmatched_glob_behavior
)
return _create_path_globs((spec.to_glob(),), unmatched_glob_behavior)
return _create_path_globs(
(spec.to_glob(),),
unmatched_glob_behavior,
description_of_origin=self.description_of_origin,
)

def all_specs(self) -> Iterator[FileLiteralSpec | FileGlobSpec]:
yield from self.file_literals
Expand All @@ -459,12 +483,19 @@ class Specs:
directory, you can directly use `RawSpecs`.
"""

includes: RawSpecs = RawSpecs()
ignores: RawSpecs = RawSpecs()
includes: RawSpecs
ignores: RawSpecs

def __bool__(self) -> bool:
return bool(self.includes) or bool(self.ignores)

@classmethod
def empty(self) -> Specs:
return Specs(
RawSpecs(description_of_origin="<not used>"),
RawSpecs(description_of_origin="<not used>"),
)

def arguments_provided_description(self) -> str | None:
"""A description of what the user specified, e.g. 'target arguments'."""
specs_descriptions = []
Expand Down
3 changes: 3 additions & 0 deletions src/python/pants/base/specs_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ def parse_specs(
self,
specs: Iterable[str],
*,
description_of_origin: str,
convert_dir_literal_to_address_literal: bool,
unmatched_glob_behavior: GlobMatchErrorBehavior = GlobMatchErrorBehavior.error,
) -> Specs:
Expand All @@ -129,12 +130,14 @@ def parse_specs(

includes = RawSpecs.create(
include_specs,
description_of_origin=description_of_origin,
convert_dir_literal_to_address_literal=convert_dir_literal_to_address_literal,
unmatched_glob_behavior=unmatched_glob_behavior,
filter_by_global_options=True,
)
ignores = RawSpecs.create(
ignore_specs,
description_of_origin=description_of_origin,
convert_dir_literal_to_address_literal=convert_dir_literal_to_address_literal,
unmatched_glob_behavior=unmatched_glob_behavior,
# By setting the below to False, we will end up matching some targets
Expand Down
16 changes: 8 additions & 8 deletions src/python/pants/base/specs_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def test_dir_literal() -> None:
assert spec.matches_target_residence_dir("dir/subdir/nested") is False
assert spec.matches_target_residence_dir("another/subdir") is False
assert_build_file_globs(
RawSpecsWithoutFileOwners(dir_literals=(spec,)),
RawSpecsWithoutFileOwners(dir_literals=(spec,), description_of_origin="tests"),
expected_build_globs={"BUILD", "dir/BUILD", "dir/subdir/BUILD"},
expected_validation_globs={"dir/subdir/*"},
)
Expand All @@ -68,7 +68,7 @@ def test_dir_literal() -> None:
assert spec.matches_target_residence_dir("") is True
assert spec.matches_target_residence_dir("dir") is False
assert_build_file_globs(
RawSpecsWithoutFileOwners(dir_literals=(spec,)),
RawSpecsWithoutFileOwners(dir_literals=(spec,), description_of_origin="tests"),
expected_build_globs={"BUILD"},
expected_validation_globs={"*"},
)
Expand All @@ -83,7 +83,7 @@ def test_dir_glob() -> None:
assert spec.matches_target_residence_dir("dir/subdir/nested") is False
assert spec.matches_target_residence_dir("another/subdir") is False
assert_build_file_globs(
RawSpecsWithoutFileOwners(dir_globs=(spec,)),
RawSpecsWithoutFileOwners(dir_globs=(spec,), description_of_origin="tests"),
expected_build_globs={"BUILD", "dir/BUILD", "dir/subdir/BUILD"},
expected_validation_globs={"dir/subdir/*"},
)
Expand All @@ -93,7 +93,7 @@ def test_dir_glob() -> None:
assert spec.matches_target_residence_dir("") is True
assert spec.matches_target_residence_dir("dir") is False
assert_build_file_globs(
RawSpecsWithoutFileOwners(dir_globs=(spec,)),
RawSpecsWithoutFileOwners(dir_globs=(spec,), description_of_origin="tests"),
expected_build_globs={"BUILD"},
expected_validation_globs={"*"},
)
Expand All @@ -109,7 +109,7 @@ def test_recursive_glob() -> None:
assert spec.matches_target_residence_dir("dir/subdir/nested/again") is True
assert spec.matches_target_residence_dir("another/subdir") is False
assert_build_file_globs(
RawSpecsWithoutFileOwners(recursive_globs=(spec,)),
RawSpecsWithoutFileOwners(recursive_globs=(spec,), description_of_origin="tests"),
expected_build_globs={"BUILD", "dir/BUILD", "dir/subdir/BUILD", "dir/subdir/**/BUILD"},
expected_validation_globs={"dir/subdir/**"},
)
Expand All @@ -120,7 +120,7 @@ def test_recursive_glob() -> None:
assert spec.matches_target_residence_dir("dir") is True
assert spec.matches_target_residence_dir("another_dir") is True
assert_build_file_globs(
RawSpecsWithoutFileOwners(recursive_globs=(spec,)),
RawSpecsWithoutFileOwners(recursive_globs=(spec,), description_of_origin="tests"),
expected_build_globs={"BUILD", "**/BUILD"},
expected_validation_globs={"**"},
)
Expand All @@ -134,7 +134,7 @@ def test_ancestor_glob() -> None:
assert spec.matches_target_residence_dir("dir/subdir/nested") is False
assert spec.matches_target_residence_dir("another/subdir") is False
assert_build_file_globs(
RawSpecsWithoutFileOwners(ancestor_globs=(spec,)),
RawSpecsWithoutFileOwners(ancestor_globs=(spec,), description_of_origin="tests"),
expected_build_globs={"BUILD", "dir/BUILD", "dir/subdir/BUILD"},
expected_validation_globs={"dir/subdir/*"},
)
Expand All @@ -143,7 +143,7 @@ def test_ancestor_glob() -> None:
assert spec.matches_target_residence_dir("") is True
assert spec.matches_target_residence_dir("dir") is False
assert_build_file_globs(
RawSpecsWithoutFileOwners(ancestor_globs=(spec,)),
RawSpecsWithoutFileOwners(ancestor_globs=(spec,), description_of_origin="tests"),
expected_build_globs={"BUILD"},
expected_validation_globs={"*"},
)
4 changes: 3 additions & 1 deletion src/python/pants/bsp/util_rules/targets.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,9 @@ class _ParseOneBSPMappingRequest:
async def parse_one_bsp_mapping(request: _ParseOneBSPMappingRequest) -> BSPBuildTargetInternal:
specs_parser = SpecsParser()
specs = specs_parser.parse_specs(
request.definition.addresses, convert_dir_literal_to_address_literal=False
request.definition.addresses,
description_of_origin=f"the BSP mapping {request.name}",
convert_dir_literal_to_address_literal=False,
).includes
return BSPBuildTargetInternal(request.name, specs, request.definition)

Expand Down
Loading

0 comments on commit 4171322

Please sign in to comment.