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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@

from pex.interpreter import PythonInterpreter

from pants.backend.python.python_requirement import PythonRequirement
from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
from pants.backend.python.tasks.pex_build_util import is_local_python_dist
from pants.backend.python.tasks.setup_py import SetupPyRunner
from pants.base.build_environment import get_buildroot
from pants.base.exceptions import TargetDefinitionException, TaskError
from pants.base.fingerprint_strategy import DefaultFingerprintStrategy
from pants.build_graph.address import Address
from pants.task.task import Task
from pants.util.dirutil import safe_mkdir

Expand All @@ -24,11 +27,13 @@ class BuildLocalPythonDistributions(Task):
"""Create python distributions (.whl) from python_dist targets."""

options_scope = 'python-create-distributions'
PYTHON_DISTS = 'user_defined_python_dists'

@classmethod
def product_types(cls):
return [cls.PYTHON_DISTS]
# Note that we don't actually place the products in the product map. We stitch
# them into the build graph instead. This is just to force the round engine
# to run this task when dists need to be built.
return [PythonRequirementLibrary]

@classmethod
def prepare(cls, options, round_manager):
Expand All @@ -40,24 +45,29 @@ def cache_target_dirs(self):

def execute(self):
dist_targets = self.context.targets(is_local_python_dist)
built_dists = set()
build_graph = self.context.build_graph

if dist_targets:
with self.invalidated(dist_targets,
fingerprint_strategy=DefaultFingerprintStrategy(),
invalidate_dependents=True) as invalidation_check:
for vt in invalidation_check.invalid_vts:
if vt.target.dependencies:
raise TargetDefinitionException(
vt.target, 'The `dependencies` field is disallowed on `python_dist` targets. '
'List any 3rd party requirements in the install_requirements argument '
'of your setup function.'
)
self._create_dist(vt.target, vt.results_dir)

for vt in invalidation_check.all_vts:
if vt.valid:
built_dists.add(self._get_whl_from_dir(os.path.join(vt.results_dir, 'dist')))
else:
if vt.target.dependencies:
raise TargetDefinitionException(
vt.target, 'The `dependencies` field is disallowed on `python_dist` targets. List any 3rd '
'party requirements in the install_requirements argument of your setup function.'
)
built_dists.add(self._create_dist(vt.target, vt.results_dir))

self.context.products.register_data(self.PYTHON_DISTS, built_dists)
dist = self._get_whl_from_dir(os.path.join(vt.results_dir, 'dist'))
req_lib_addr = Address.parse('{}__req_lib'.format(vt.target.address.spec))
self._inject_synthetic_dist_requirements(dist, req_lib_addr)
# Make any target that depends on the dist depend on the synthetic req_lib,
# for downstream consumption.
for dependent in build_graph.dependents_of(vt.target.address):
build_graph.inject_dependency(dependent, req_lib_addr)

def _create_dist(self, dist_tgt, dist_target_dir):
"""Create a .whl file for the specified python_distribution target."""
Expand All @@ -76,9 +86,24 @@ def _create_dist(self, dist_tgt, dist_target_dir):
# Build a whl using SetupPyRunner and return its absolute path.
setup_runner = SetupPyRunner(dist_target_dir, 'bdist_wheel', interpreter=interpreter)
setup_runner.run()
return self._get_whl_from_dir(os.path.join(dist_target_dir, 'dist'))

def _get_whl_from_dir(self, install_dir):
def _inject_synthetic_dist_requirements(self, dist, req_lib_addr):
"""Inject a synthetic requirements library that references a local wheel.

:param dist: Path of the locally built wheel to reference.
:param req_lib_addr: :class: `Address` to give to the synthetic target.
:return: a :class: `PythonRequirementLibrary` referencing the locally-built wheel.
"""
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.

req = PythonRequirement(req_name, repository=whl_dir)
self.context.build_graph.inject_synthetic_target(req_lib_addr, PythonRequirementLibrary,
requirements=[req])

@staticmethod
def _get_whl_from_dir(install_dir):
"""Return the absolute path of the whl in a setup.py install directory."""
dists = glob.glob(os.path.join(install_dir, '*.whl'))
if len(dists) == 0:
Expand Down
44 changes: 0 additions & 44 deletions src/python/pants/backend/python/tasks/pex_build_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
from pex.resolver import resolve
from twitter.common.collections import OrderedSet

from pants.backend.python.python_requirement import PythonRequirement
from pants.backend.python.subsystems.python_setup import PythonSetup
from pants.backend.python.targets.python_binary import PythonBinary
from pants.backend.python.targets.python_distribution import PythonDistribution
Expand All @@ -20,7 +19,6 @@
from pants.backend.python.targets.python_tests import PythonTests
from pants.base.build_environment import get_buildroot
from pants.base.exceptions import TaskError
from pants.build_graph.address import Address
from pants.build_graph.files import Files
from pants.python.python_repos import PythonRepos

Expand Down Expand Up @@ -161,45 +159,3 @@ def _resolve_multi(interpreter, requirements, platforms, find_links):
allow_prereleases=python_setup.resolver_allow_prereleases)

return distributions


def inject_synthetic_dist_requirements(build_graph, local_built_dists, synthetic_address,
binary_tgt=None):
"""Inject a synthetic requirements library from a local wheel.

:param build_graph: The build graph needed for injecting synthetic targets.
:param local_built_dists: A list of paths to locally built wheels to package into
requirements libraries.
:param synthetic_address: A generative address for addressing synthetic targets.
:param binary_tgt: An optional parameter to be passed only when called by the
`python_binary_create` task. This is needed to ensure that only python_dist targets in a binary
target's closure are included in the binary for the case where a user specifies mulitple binary
targets in a single invocation of `./pants binary`.
:return: a :class: `PythonRequirementLibrary` containing a requirements that maps to a
locally-built wheel.
"""
def should_create_req(bin_tgt, loc):
if not bin_tgt:
return True
# Ensure that a target is in a binary target's closure. See docstring for more detail.
return any([tgt.id in loc for tgt in bin_tgt.closure()])

def python_requirement_from_wheel(path):
base = os.path.basename(path)
whl_dir = os.path.dirname(path)
whl_metadata = base.split('-')
req_name = '=='.join([whl_metadata[0], whl_metadata[1]])
return PythonRequirement(req_name, repository=whl_dir)

local_whl_reqs = [
python_requirement_from_wheel(whl_location)
for whl_location in local_built_dists
if should_create_req(binary_tgt, whl_location)
]

if not local_whl_reqs:
return []

addr = Address.parse(synthetic_address)
build_graph.inject_synthetic_target(addr, PythonRequirementLibrary, requirements=local_whl_reqs)
return [build_graph.get_target(addr)]
17 changes: 3 additions & 14 deletions src/python/pants/backend/python/tasks/python_binary_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,10 @@
from pex.pex_info import PexInfo

from pants.backend.python.targets.python_binary import PythonBinary
from pants.backend.python.tasks.build_local_python_distributions import \
BuildLocalPythonDistributions
from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
from pants.backend.python.tasks.pex_build_util import (dump_requirement_libs, dump_sources,
has_python_requirements, has_python_sources,
has_resources,
inject_synthetic_dist_requirements)
has_resources)
from pants.base.build_environment import get_buildroot
from pants.base.exceptions import TaskError
from pants.build_graph.target_scopes import Scopes
Expand Down Expand Up @@ -46,7 +44,7 @@ def cache_target_dirs(self):
def prepare(cls, options, round_manager):
round_manager.require_data(PythonInterpreter)
round_manager.require_data('python') # For codegen.
round_manager.require_data(BuildLocalPythonDistributions.PYTHON_DISTS)
round_manager.optional_product(PythonRequirementLibrary) # For local dists.

@staticmethod
def is_binary(target):
Expand Down Expand Up @@ -130,15 +128,6 @@ def _create_binary(self, binary_tgt, results_dir):
# Dump everything into the builder's chroot.
for tgt in source_tgts:
dump_sources(builder, tgt, self.context.log)

# Handle locally-built python distribution dependencies.
built_dists = self.context.products.get_data(BuildLocalPythonDistributions.PYTHON_DISTS)
if built_dists:
req_tgts = inject_synthetic_dist_requirements(self.context.build_graph,
built_dists,
':'.join(2 * [binary_tgt.invalidation_hash()]),
binary_tgt) + req_tgts

dump_requirement_libs(builder, interpreter, req_tgts, self.context.log, binary_tgt.platforms)

# Build the .pex file.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@
from pex.pex_builder import PEXBuilder

from pants.backend.python.python_requirement import PythonRequirement
from pants.backend.python.tasks.build_local_python_distributions import \
BuildLocalPythonDistributions
from pants.backend.python.tasks.pex_build_util import (dump_requirement_libs, dump_requirements,
inject_synthetic_dist_requirements)
from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
from pants.backend.python.tasks.pex_build_util import dump_requirement_libs, dump_requirements
from pants.base.hash_utils import hash_all
from pants.invalidation.cache_manager import VersionedTargetSet
from pants.task.task import Task
Expand All @@ -33,7 +31,7 @@ class ResolveRequirementsTaskBase(Task):
@classmethod
def prepare(cls, options, round_manager):
round_manager.require_data(PythonInterpreter)
round_manager.require_data(BuildLocalPythonDistributions.PYTHON_DISTS)
round_manager.optional_product(PythonRequirementLibrary) # For local dists.

def resolve_requirements(self, req_libs, local_dist_targets=None):
"""Requirements resolution for PEX files.
Expand Down Expand Up @@ -61,12 +59,6 @@ def resolve_requirements(self, req_libs, local_dist_targets=None):
# to cover the empty case.
if not os.path.isdir(path):
with safe_concurrent_creation(path) as safe_path:
# Handle locally-built python distribution dependencies.
built_dists = self.context.products.get_data(BuildLocalPythonDistributions.PYTHON_DISTS)
if built_dists:
req_libs = inject_synthetic_dist_requirements(
self.context.build_graph, built_dists, ':'.join(2 * [target_set_id])
) + req_libs
builder = PEXBuilder(path=safe_path, interpreter=interpreter, copy=True)
dump_requirement_libs(builder, interpreter, req_libs, self.context.log)
builder.freeze()
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/build_graph/build_graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ def dependencies_of(self, address):
return self._target_dependencies_by_address[address]

def dependents_of(self, address):
"""Returns the Targets which depend on the target at `address`.
"""Returns the addresses of the targets that depend on the target at `address`.

This method asserts that the address given is actually in the BuildGraph.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,13 @@ def setUp(self):
sources=sources)

def test_python_create_distributions(self):
context = self.context(target_roots=[self.python_dist_tgt], for_task_types=[BuildLocalPythonDistributions])
context = self.context(target_roots=[self.python_dist_tgt],
for_task_types=[BuildLocalPythonDistributions])
self.assertEquals([self.python_dist_tgt], context.build_graph.targets())
python_create_distributions_task = self.create_task(context)
python_create_distributions_task.execute()
built_dists = context.products.get_data(BuildLocalPythonDistributions.PYTHON_DISTS)
self.assertGreater(len(built_dists), 0)
self.assertTrue(any(['my_dist-0.0.0-py2-none-any.whl' in dist for dist in list(built_dists)]))
synthetic_tgts = set(context.build_graph.targets()) - {self.python_dist_tgt}
self.assertEquals(1, len(synthetic_tgts))
synthetic_target = next(iter(synthetic_tgts))
self.assertEquals(['my_dist==0.0.0'],
[str(x.requirement) for x in synthetic_target.requirements.value])