From 1bc7a2838ebdc72edd30855792648a3e4f48e4b7 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Thu, 14 Jul 2022 08:29:15 -0700 Subject: [PATCH] Fix defaulting of parameters in explicitly specified deps on `parametrize`d targets for AsyncFieldMixin (Cherry-pick of #16176) (#16180) Although the tests for #14519 were reasonably good, they tested with `StringField`, rather than additionally with `AsyncFieldMixin`. The latter changes the definition of equality for the `Field` to include its address, meaning that comparing the `Field` instance itself will never match for `Field`s from different targets. That made #14519... not particularly useful in production. This change fixes `Field` value comparisons to use `field.value`, references the newly opened #16175 (which is out of scope for now, given that it will likely be impacted by `__defaults__` and would be much harder to cherry-pick), and adds an additional test. Fixes #14519. [ci skip-rust] [ci skip-build-wheels] --- .../pants/engine/internals/graph_test.py | 128 +++++++++--------- .../pants/engine/internals/parametrize.py | 11 +- 2 files changed, 71 insertions(+), 68 deletions(-) diff --git a/src/python/pants/engine/internals/graph_test.py b/src/python/pants/engine/internals/graph_test.py index 184d6391248..34bff9c8663 100644 --- a/src/python/pants/engine/internals/graph_test.py +++ b/src/python/pants/engine/internals/graph_test.py @@ -81,7 +81,7 @@ class SpecialCasedDeps2(SpecialCasedDependencies): alias = "special_cased_deps2" -class ResolveField(StringField): +class ResolveField(StringField, AsyncFieldMixin): alias = "resolve" @@ -831,9 +831,9 @@ def assert_generated( address: Address, build_content: str, files: list[str], - expected: set[Target], + expected_targets: set[Target] | None = None, *, - dependencies: dict[str, set[str]] | None = None, + expected_dependencies: dict[str, set[str]] | None = None, ) -> None: rule_runner.write_files( { @@ -845,11 +845,14 @@ def assert_generated( _TargetParametrizations, [_TargetParametrizationsRequest(address, description_of_origin="tests")], ) - assert expected == { - t for parametrization in parametrizations for t in parametrization.parametrization.values() - } + if expected_targets is not None: + assert expected_targets == { + t + for parametrization in parametrizations + for t in parametrization.parametrization.values() + } - if dependencies is not None: + if expected_dependencies is not None: # TODO: Adjust the `TransitiveTargets` API to expose the complete mapping. # see https://github.com/pantsbuild/pants/issues/11270 specs = SpecsParser(rule_runner.build_root).parse_specs( @@ -867,7 +870,7 @@ def assert_generated( ) assert { k.spec: {a.spec for a in v} for k, v in dependency_mapping.mapping.items() - } == dependencies + } == expected_dependencies def test_generate_multiple(generated_targets_rule_runner: RuleRunner) -> None: @@ -956,7 +959,7 @@ def test_parametrize(generated_targets_rule_runner: RuleRunner) -> None: residence_dir="demo", ), }, - dependencies={ + expected_dependencies={ "demo@tags=t1": {"demo/f1.ext@tags=t1"}, "demo@tags=t2": {"demo/f1.ext@tags=t2"}, "demo/f1.ext@tags=t1": set(), @@ -1027,7 +1030,7 @@ def test_parametrize_overrides(generated_targets_rule_runner: RuleRunner) -> Non residence_dir="demo", ), }, - dependencies={ + expected_dependencies={ "demo:demo": { "demo/f1.ext@resolve=a", "demo/f1.ext@resolve=b", @@ -1058,14 +1061,14 @@ def test_parametrize_atom(generated_targets_rule_runner: RuleRunner) -> None: residence_dir="demo", ), }, - dependencies={ + expected_dependencies={ "demo@resolve=a": set(), "demo@resolve=b": set(), }, ) -def test_parametrize_partial_atom(generated_targets_rule_runner: RuleRunner) -> None: +def test_parametrize_partial_atom_to_atom(generated_targets_rule_runner: RuleRunner) -> None: assert_generated( generated_targets_rule_runner, Address("demo", target_name="t2"), @@ -1085,27 +1088,7 @@ def test_parametrize_partial_atom(generated_targets_rule_runner: RuleRunner) -> """ ), ["f1.ext", "f2.ext"], - { - MockGeneratedTarget( - { - SingleSourceField.alias: "f2.ext", - ResolveField.alias: "a", - Dependencies.alias: [":t1"], - }, - Address("demo", target_name="t2", parameters={"resolve": "a"}), - residence_dir="demo", - ), - MockGeneratedTarget( - { - SingleSourceField.alias: "f2.ext", - ResolveField.alias: "b", - Dependencies.alias: [":t1"], - }, - Address("demo", target_name="t2", parameters={"resolve": "b"}), - residence_dir="demo", - ), - }, - dependencies={ + expected_dependencies={ "demo:t1@resolve=a": set(), "demo:t1@resolve=b": set(), "demo:t2@resolve=a": {"demo:t1@resolve=a"}, @@ -1114,7 +1097,9 @@ def test_parametrize_partial_atom(generated_targets_rule_runner: RuleRunner) -> ) -def test_parametrize_partial_generator(generated_targets_rule_runner: RuleRunner) -> None: +def test_parametrize_partial_generator_to_generator( + generated_targets_rule_runner: RuleRunner, +) -> None: assert_generated( generated_targets_rule_runner, Address("demo", target_name="t2"), @@ -1134,37 +1119,7 @@ def test_parametrize_partial_generator(generated_targets_rule_runner: RuleRunner """ ), ["f1.ext", "f2.ext"], - { - MockGeneratedTarget( - { - SingleSourceField.alias: "f2.ext", - ResolveField.alias: "a", - Dependencies.alias: [":t1"], - }, - Address( - "demo", - relative_file_path="f2.ext", - target_name="t2", - parameters={"resolve": "a"}, - ), - residence_dir="demo", - ), - MockGeneratedTarget( - { - SingleSourceField.alias: "f2.ext", - ResolveField.alias: "b", - Dependencies.alias: [":t1"], - }, - Address( - "demo", - relative_file_path="f2.ext", - target_name="t2", - parameters={"resolve": "b"}, - ), - residence_dir="demo", - ), - }, - dependencies={ + expected_dependencies={ "demo/f1.ext:t1@resolve=a": set(), "demo/f1.ext:t1@resolve=b": set(), "demo/f2.ext:t2@resolve=a": {"demo:t1@resolve=a"}, @@ -1185,6 +1140,49 @@ def test_parametrize_partial_generator(generated_targets_rule_runner: RuleRunner ) +def test_parametrize_partial_generator_to_generated( + generated_targets_rule_runner: RuleRunner, +) -> None: + assert_generated( + generated_targets_rule_runner, + Address("demo", target_name="t2"), + dedent( + """\ + generator( + name='t1', + resolve=parametrize('a', 'b'), + sources=['f1.ext'], + ) + generator( + name='t2', + resolve=parametrize('a', 'b'), + sources=['f2.ext'], + dependencies=['./f1.ext:t1'], + ) + """ + ), + ["f1.ext", "f2.ext"], + expected_dependencies={ + "demo/f1.ext:t1@resolve=a": set(), + "demo/f1.ext:t1@resolve=b": set(), + "demo/f2.ext:t2@resolve=a": {"demo/f1.ext:t1@resolve=a"}, + "demo/f2.ext:t2@resolve=b": {"demo/f1.ext:t1@resolve=b"}, + "demo:t1@resolve=a": { + "demo/f1.ext:t1@resolve=a", + }, + "demo:t1@resolve=b": { + "demo/f1.ext:t1@resolve=b", + }, + "demo:t2@resolve=a": { + "demo/f2.ext:t2@resolve=a", + }, + "demo:t2@resolve=b": { + "demo/f2.ext:t2@resolve=b", + }, + }, + ) + + # ----------------------------------------------------------------------------------------------- # Test `SourcesField`. Also see `engine/target_test.py`. # ----------------------------------------------------------------------------------------------- diff --git a/src/python/pants/engine/internals/parametrize.py b/src/python/pants/engine/internals/parametrize.py index 055b81eb39c..d7a6b22a00d 100644 --- a/src/python/pants/engine/internals/parametrize.py +++ b/src/python/pants/engine/internals/parametrize.py @@ -232,14 +232,19 @@ def get_subset(self, address: Address, consumer: Target) -> Target: return instance def remaining_fields_match(candidate: Target) -> bool: - """Returns true if all Fields absent from the candidate's Address match the consumer.""" + """Returns true if all Fields absent from the candidate's Address match the consumer. + + TODO: This does not account for the computed default values of Fields: + see https://github.com/pantsbuild/pants/issues/16175 + """ + unspecified_param_field_names = { key for key in candidate.address.parameters.keys() if key not in address.parameters } return all( - consumer.has_field(field_type) and consumer[field_type] == field_value - for field_type, field_value in candidate.field_values.items() + consumer.has_field(field_type) and consumer[field_type].value == field.value + for field_type, field in candidate.field_values.items() if field_type.alias in unspecified_param_field_names )