Skip to content
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

Simplify python local dist handling code. #5480

Merged
merged 2 commits into from
Feb 20, 2018

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Feb 17, 2018

Previously, BuildLocalPythonDistributions generated local dists and put them
in products. Downstream tasks then synthesized PythonRequirementLibrary
targets to point to the local dists, if they needed to resolve them. This was clunky,
and required special knowledge of local dist issues to reside in multiple downstream
tasks.

This change has BuildLocalPythonDistributions itself inject the synthetic
PythonRequirementLibrary targets - one per dist - and stitches them into the
build graph. Downstream tasks no longer need to know or care about local dists
(apart from declaring them as required products), they just see PythonRequirementLibrary
targets, whether synthetic or organic, and handle them uniformly.

Note that previously PythonBinaryCreate had to do some footwork to ensure that only the
dists depended on by a specific PythonBinary were used when creating that binary.
This now happens naturally, since we generate one requirement library per dist,
and the dependencies are wired up in the right way.

base = os.path.basename(dist)
whl_dir = os.path.dirname(dist)
whl_metadata = base.split('-')
req_name = '=='.join([whl_metadata[0], whl_metadata[1]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in other discussions today in slack and in comments on #5479, this is broken since the requirement will be stable across source file edits (until version itself is edited in setup.py). That's a pre-existing problem though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this will be fixed by creating unique versions, rather than directly special-casing in this or other code.

Copy link
Contributor

@CMLivingston CMLivingston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for making the improvement.

@benjyw benjyw merged commit 7788d47 into pantsbuild:master Feb 20, 2018
@benjyw benjyw deleted the simplify_python_local_dists2 branch February 20, 2018 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants