Link requirements targets to their source.

[jsirois]

Previsouly the python_requirements macro would not link the
PythonRequirementLibrary targets it generated from a requirements
file to the requirement file itself, leading to invalidation bugs. We
now do this by adding a dependency on the requirements file to each
PythonRequirementLibrary generated.

Fixes #6404

[stuhood]

Thanks!

An alternative implementation might be to add a default_sources_glob and a sources argument.

[jsirois]

An alternative implementation might be to add a default_sources_glob and a sources argument.

Adding a source - singular - optional argument to PythonRequirementLibrary might make sense to take the place of the dependency python_requirements injects. The tradeoff here is a wonky public BUILD dictionary source field that users should never use (see 2. below) vs a synthetic target (the Files target dependency) user's might not know what to make of when executing goals like list, dependencies or filemap.

The default_sources_glob makes less sense. You don't describe what that would be used for, but I can only see two cases and both have problems:

1. The PythonRequirementLibrary is declared in a BUILD directly where the BUILD has a sibling requirements.txt. Now you get a bogus source dependency on requirments.txt for that PythonRequirementLibrary.
2. If handed a source, whether via default globs or an explicit source argument, the PythonRequirementLibrary expands the source to requirements like the python_requirements macro currently does. This would lead to fat PythonRequirementLibrary targets that own all the requirements in a requirements.txt file which is almost always not what you want.

[stuhood]

The PythonRequirementLibrary is declared in a BUILD directly where the BUILD has a sibling requirements.txt. Now you get a bogus source dependency on requirments.txt for that PythonRequirementLibrary.

Got it. I was forgetting the relationship between the macro and the target type.

Thanks!

[stuhood]

Not sure how likely collisions are, but it would be good to make this name a bit more synthetic, and include the owning target's name as well... '__{}_files'.format(requirements_file_target_name)?

[jsirois]

Saw this comment late. Right now the mainline case is a target named requirements.txt (or foo.bob if their requirements file is foo.bob). Having that filename match a requirement name or hand-written target name seemed pretty unlikely to me. I can circle back with a follow-up PR if you think my unlikely guess underestimates things.

[stuhood]

Should be fine, unless we can imagine a case with multiple target owners of the same requirements files.

[jsirois]

That's not possible even prior to this change since the requirements would each be exported as targets named by their requirement key leading to a dup target for each of those.

[benjyw]

Thanks for the quick turnaround!

[stuhood]

I've picked this to 1.8.x.