Fix use of generic target in runtime package dependencies

[RaniSputnik]

What

Fixes the following setup:

# BUILD
pex_binary(name='bin_a', entry_point='main_a.py')
pex_binary(name='bin_b', entry_point='main_b.py')
target(name='all_bins', dependencies=[':bin_a', ':bin_b'])
python_test(name="tests", runtime_package_dependencies=[":all_bins"])

Previously the PEX dependencies would not get packaged and materialized when the tests were run.

Why

Allows a target() alias to be used in runtime_package_dependencies to group packageable targets, rather than having to list each one individually. This would help simplify a setup I have where I have the misfortune of having to write a loop to generate a list of targets:

# BUILD
for model in models:
  docker_image(name=model.name)

target(name="models", dependencies=[model.name for model in models])

Using the generic target I can isolate this mess to the one relevant directory and keep the rest of the codebase clear.

Test plan

• New test added to cover the scenario

Open questions

• Is this the kind of thing we would add to the release notes? Or is it a bit niche?
• Is this the most reliable way to implement this? I was surprised to find that generic target isn't modeled as a TargetGenerator would that be a more robust fix? (sounds like it would be a much more invasive change)
• I think this issue affects the archive target too. Shall I investigate that in this PR? Or keep it small and investigate separately?

[benjyw]

• Is this the kind of thing we would add to the release notes? Or is it a bit niche?

I think this is useful in the release notes, yes. docs/notes/2.32.x.md in this case.

• Is this the most reliable way to implement this? I was surprised to find that generic target isn't modeled as a TargetGenerator would that be a more robust fix? (sounds like it would be a much more invasive change)

GenericTarget has no sources, so there is nothing for it to generate targets from. It is just a simple target. So your approach here is good.

I think this issue affects the archive target too. Shall I investigate that in this PR? Or keep it small and investigate separately?

I don't think one would naturally use archive() as a grouping target?

[benjyw]

This is great! Just one fix needed, plus the release notes

[RaniSputnik]

I don't think one would naturally use archive() as a grouping target?

Apologies for being unclear @benjyw, I meant that if one were to depend on a generic target in the packages field of archive, I don't think the transitive dependencies would be packaged. I haven't actually tested that so I might be wrong - I'll investigate if I get the chance.

GenericTarget has no sources, so there is nothing for it to generate targets from. It is just a simple target. So your approach here is good.

Right, that makes sense.

The thing that felt weird to me about the implementation is it felt like a behaviour that generic target should own rather than it being imposed on the caller. I'm not familiar enough with the codebase to know how that would work, and the dependency walking + predicate model also makes sense 🤷‍♂️ it just means that callers has to be mindful to implement the behaviour of generic target rather than it being implemented once and used transparently everywhere.

---

Thanks for the review! Should be all ready to go now 🚀

[benjyw]

Currently the callers have to know how they want to traverse the graph. But yeah, it's possible that "give me my direct dependencies" should always elide GenericTarget, and so we could push this logic up to there.

[benjyw]

I don't think this is a matter of correctness - it's more that we moved the goalposts (to a hopefully better place). But I wouldn't classify the previous behavior as a bug, more as an over-literal interpretation of direct dependencies...