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

Correctly describe the origin of invalid specs, e.g. --paths-from #15730

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
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}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tdyas is this an accurate description?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, looks accurate

convert_dir_literal_to_address_literal=False,
).includes
return BSPBuildTargetInternal(request.name, specs, request.definition)

Expand Down
Loading