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

Link requirements targets to their source. #6405

Merged
merged 1 commit into from Aug 28, 2018

Conversation

Projects
None yet
4 participants
@jsirois
Copy link
Member

jsirois commented Aug 28, 2018

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

Link requirements targets to their source.
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 stuhood added this to the 1.9.x milestone Aug 28, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

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

@stuhood stuhood modified the milestones: 1.9.x, 1.8.x Aug 28, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 28, 2018

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.

@jsirois jsirois requested a review from benjyw Aug 28, 2018

@stuhood stuhood requested a review from illicitonion Aug 28, 2018

@stuhood
Copy link
Member

stuhood left a comment

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!

@@ -58,9 +58,16 @@ def __call__(self, requirements_relpath='requirements.txt'):
raise ValueError('Only 1 --find-links url is supported per requirements file')
repository = value

requirements_file_target_name = requirements_relpath
self._parse_context.create_object('files',

This comment has been minimized.

@stuhood

stuhood Aug 28, 2018

Member

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)?

This comment has been minimized.

@jsirois

jsirois Aug 29, 2018

Member

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.

This comment has been minimized.

@stuhood

stuhood Aug 29, 2018

Member

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

This comment has been minimized.

@jsirois

jsirois Aug 29, 2018

Member

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

benjyw approved these changes Aug 28, 2018

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Aug 28, 2018

Thanks for the quick turnaround!

@jsirois jsirois merged commit 1e4fa18 into pantsbuild:master Aug 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jsirois jsirois deleted the jsirois:issues/6404 branch Aug 28, 2018

@stuhood stuhood modified the milestones: 1.8.x, 1.9.x Aug 28, 2018

stuhood added a commit that referenced this pull request Aug 28, 2018

Link requirements targets to their source. (#6405)
Previously 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

This comment has been minimized.

Copy link
Member

stuhood commented Aug 28, 2018

I've picked this to 1.8.x.

stuhood added a commit that referenced this pull request Aug 28, 2018

Link requirements targets to their source. (#6405)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment