diff --git a/src/python/pants/backend/python/target_types_test.py b/src/python/pants/backend/python/target_types_test.py index f53e4cf15e5..eb5473deda7 100644 --- a/src/python/pants/backend/python/target_types_test.py +++ b/src/python/pants/backend/python/target_types_test.py @@ -25,6 +25,7 @@ PythonRequirementsField, PythonRequirementTarget, PythonSourcesGeneratorTarget, + PythonSourceTarget, ResolvedPexEntryPoint, ResolvePexEntryPointRequest, ResolvePythonDistributionEntryPointsRequest, @@ -41,6 +42,7 @@ from pants.core.goals.generate_lockfiles import UnrecognizedResolveNamesError from pants.engine.addresses import Address from pants.engine.internals.graph import _TargetParametrizations, _TargetParametrizationsRequest +from pants.engine.internals.parametrize import Parametrize from pants.engine.internals.scheduler import ExecutionError from pants.engine.target import ( InferredDependencies, @@ -615,3 +617,92 @@ def gen_pex_binary_tgt(entry_point: str, tags: list[str] | None = None) -> PexBi gen_pex_binary_tgt("subdir.f.py", tags=["overridden"]), gen_pex_binary_tgt("subdir.f:main"), } + + +def assert_inferred( + rule_runner: RuleRunner, address: Address, expected: list[Address], comment="" +) -> None: + tgt = rule_runner.get_target(address) + inferred = rule_runner.request( + InferredDependencies, + [ + InferPexBinaryEntryPointDependency( + PexBinaryEntryPointDependencyInferenceFieldSet.create(tgt) + ) + ], + ) + assert InferredDependencies(expected) == inferred, comment + + +def test_20806() -> None: + rule_runner = RuleRunner( + rules=[ + *target_types_rules.rules(), + *import_rules(), + *python_sources.rules(), + QueryRule(InferredDependencies, [InferPythonDistributionDependencies]), + ], + target_types=[ + PythonDistribution, + PythonRequirementTarget, + PythonSourceTarget, + PexBinary, + ], + objects={"parametrize": Parametrize}, + ) + + rule_runner.write_files( + { + "BUILD": dedent( + """\ + python_source(name="src", source="example.py", tags=parametrize(a=["a"], b=["b"])) + + pex_binary(name="ambiguous", entry_point="./example.py") + + pex_binary(name="with-target", entry_point="./example.py:src@tags=a") + + pex_binary(name="with-deps", entry_point="./example.py", dependencies=[":src@tags=a"]) + + pex_binary(name="with-deps-and-ignore", entry_point="./example.py", dependencies=[":src@tags=a", "!:src@tags=b"]) + + pex_binary(name="with-ignore-only", entry_point="./example.py", dependencies=[ "!:src@tags=b"]) + """ + ), + "example.py": "print('worked!')", + } + ) + + assert_inferred( + rule_runner, + Address(spec_path="", target_name="ambiguous"), + [], + comment="The ambiguous should not be disambiguated", + ) + + # assert_inferred( + # rule_runner, + # Address(spec_path="", target_name="with-target"), + # [Address(spec_path="", target_name="src", parameters={"tags": "a"})], + # comment="Using an explicit target is not standard (the entry is actually a Python entrypoint, not a Pants target)", + # ) + + assert_inferred( + rule_runner, + Address(spec_path="", target_name="with-deps"), + [Address(spec_path="", target_name="src", parameters={"tags": "a"})], + comment="Explicitly providing the dep should resolve the ambiguity", + ) + + assert_inferred( + rule_runner, + Address(spec_path="", target_name="with-ignore-only"), + [Address(spec_path="", target_name="src", parameters={"tags": "a"})], + comment="Ignoring the other ambiguous cases should disambiguate", + ) + + assert_inferred( + rule_runner, + Address(spec_path="", target_name="with-deps-and-ignore"), + [Address(spec_path="", target_name="src", parameters={"tags": "a"})], + comment="Ignoring the other ambiguous cases should disambiguate, even if one of the ambiguous ones is provided as a dep", + ) diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index 1f64b88be6d..512430fc680 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -2626,7 +2626,7 @@ class ExplicitlyProvidedDependencies: @memoized_method def any_are_covered_by_includes(self, addresses: Iterable[Address]) -> bool: - """Return True if every address is in the explicitly provided includes. + """Return True if any address is in the explicitly provided includes. Note that if the input addresses are generated targets, they will still be marked as covered if their original target generator is in the explicitly provided includes. @@ -2636,6 +2636,20 @@ def any_are_covered_by_includes(self, addresses: Iterable[Address]) -> bool: for addr in addresses ) + @memoized_method + def covered_by_includes(self, addresses: frozenset[Address]) -> frozenset[Address]: + """Return only the addresses which are in the explicitly provided includes.""" + out = set() + for addr in addresses: + if addr in self.includes: + out.add(addr) + maybe_tgt_generator = addr.maybe_convert_to_target_generator() + if maybe_tgt_generator in self.includes: + # TODO: do we include the maybe_tgt_generator or the addr? + out.add(maybe_tgt_generator) + + return frozenset(out) + @memoized_method def remaining_after_disambiguation( self, addresses: Iterable[Address], owners_must_be_ancestors: bool @@ -2682,21 +2696,43 @@ def maybe_warn_of_ambiguous_dependency_inference( `!!`. If `owners_must_be_ancestors` is True, any addresses which are not ancestors of the target in question will also be ignored. """ - if not ambiguous_addresses or self.any_are_covered_by_includes(ambiguous_addresses): - return - remaining = self.remaining_after_disambiguation( + if not ambiguous_addresses: + return None + remaining_after_ignores = self.remaining_after_disambiguation( ambiguous_addresses, owners_must_be_ancestors=owners_must_be_ancestors ) + if len(remaining_after_ignores) == 1: + return + + remaining_after_includes = self.covered_by_includes(remaining_after_ignores) + if len(remaining_after_includes) == 1: + return + elif len(remaining_after_includes) > 1: + # if multiple includes still own this target, we warn only about them, and not all targets + remaining = remaining_after_includes + suggestion = ( + f"\n\nMultiple dependencies specified in the `dependencies` field of {original_address} " + f"own this {import_reference}. Try removing all of them except for one, " + "or ignore the ones you do not want by prefixing " + f"with `!` or `!!` so that one or no targets are left." + ) + else: + # if none of the includes helped, we warn about all owners + remaining = remaining_after_ignores + suggestion = ( + f"\n\nPlease explicitly include the dependency you want in the `dependencies` " + f"field of {original_address}, or ignore the ones you do not want by prefixing " + f"with `!` or `!!` so that one or no targets are left." + ) + if len(remaining) <= 1: return logger.warning( f"{context}, but Pants cannot safely infer a dependency because more than one target " f"owns this {import_reference}, so it is ambiguous which to use: " f"{sorted(addr.spec for addr in remaining)}." - f"\n\nPlease explicitly include the dependency you want in the `dependencies` " - f"field of {original_address}, or ignore the ones you do not want by prefixing " - f"with `!` or `!!` so that one or no targets are left." - f"\n\nAlternatively, you can remove the ambiguity by deleting/changing some of the " + + suggestion + + f"\n\nAlternatively, you can remove the ambiguity by deleting/changing some of the " f"targets so that only 1 target owns this {import_reference}. Refer to " f"{doc_url('docs/using-pants/troubleshooting-common-issues#import-errors-and-missing-dependencies')}." ) @@ -2710,12 +2746,17 @@ def disambiguated( `!!`. If `owners_must_be_ancestors` is True, any addresses which are not ancestors of the target in question will also be ignored. """ - if not ambiguous_addresses or self.any_are_covered_by_includes(ambiguous_addresses): + if not ambiguous_addresses: return None remaining_after_ignores = self.remaining_after_disambiguation( ambiguous_addresses, owners_must_be_ancestors=owners_must_be_ancestors ) - return list(remaining_after_ignores)[0] if len(remaining_after_ignores) == 1 else None + if len(remaining_after_ignores) == 1: + return list(remaining_after_ignores)[0] + only_explicitly_included = self.covered_by_includes(remaining_after_ignores) + if len(only_explicitly_included) == 1: + return list(only_explicitly_included)[0] + return None FS = TypeVar("FS", bound="FieldSet") diff --git a/src/python/pants/engine/target_test.py b/src/python/pants/engine/target_test.py index 74cb89d2edd..c0a3a7c747f 100644 --- a/src/python/pants/engine/target_test.py +++ b/src/python/pants/engine/target_test.py @@ -1309,16 +1309,15 @@ def get_disambiguated( assert get_disambiguated(all_addr, ignores=[addr_a.maybe_convert_to_target_generator()]) is None assert get_disambiguated(all_addr, ignores=all_addr) is None assert get_disambiguated([]) is None - # If any includes would disambiguate the ambiguous target, we don't consider disambiguating - # via excludes as the user has already explicitly disambiguated the module. - assert get_disambiguated(all_addr, ignores=[addr_a, addr_b], includes=[addr_a]) is None + # Verify that ignores take precedence over includes + assert get_disambiguated(all_addr, ignores=[addr_a, addr_b], includes=[addr_a]) is addr_c assert ( get_disambiguated( ambiguous=all_addr, ignores=[addr_a, addr_b], includes=[addr_a.maybe_convert_to_target_generator()], ) - is None + is addr_c ) # You can also disambiguate via `owners_must_be_ancestors`. @@ -1332,6 +1331,24 @@ def get_disambiguated( == addr_a ) + # Verify that includes can resolve ambiguities + assert get_disambiguated(ambiguous=all_addr, includes=[addr_a]) is addr_a + + # Verify that includes which do not resolve the ambiguity do not resolve the ambiguity + assert get_disambiguated(ambiguous=all_addr, includes=[addr_a, addr_b]) is None + + # Verify that ignores works for parametrized targets + addr_param_a = Address(spec_path="", target_name="src", parameters={"tag": "a"}) + addr_param_b = Address(spec_path="", target_name="src", parameters={"tag": "b"}) + assert ( + get_disambiguated( + ambiguous=[addr_param_a, addr_param_b], + includes=[addr_param_a], + ignores=[addr_param_b], + ) + == addr_param_a + ) + def test_explicitly_provided_dependencies_maybe_warn_of_ambiguous_dependency_inference( caplog,