Skip to content

Commit

Permalink
Support running python tests in the pex chroot.
Browse files Browse the repository at this point in the history
With the new `--chroot` option, tests are run with `CWD` set to the
`py.test` pex chroot making access of un-declared file dependencies
~impossible and ensuring all files involved in `py.test` execution are
part of the test cache key.

CWD switching revealed an assumption error in the java
SubprocessExecutor and a relative path error in Travis CI
configuration, both now fixed.

Fixes pantsbuild#5019
  • Loading branch information
jsirois committed Nov 7, 2017
1 parent 8390373 commit a9d34c4
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 35 deletions.
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
env:
global:
- PANTS_CONFIG_FILES="pants.travis-ci.ini"
- PANTS_CONFIG_FILES="${TRAVIS_BUILD_DIR}/pants.travis-ci.ini"
- ANDROID_SDK_INSTALL_LOCATION="${HOME}/opt/android-sdk-install"
- ANDROID_HOME="$ANDROID_SDK_INSTALL_LOCATION/android-sdk-linux"

Expand Down
2 changes: 1 addition & 1 deletion build-support/bin/ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ if [[ "${skip_python:-false}" == "false" ]]; then
fi
start_travis_section "CoreTests" "Running core python tests${shard_desc}"
(
./pants.pex --tag='-integration' ${PANTS_ARGS[@]} test.pytest \
./pants.pex --tag='-integration' ${PANTS_ARGS[@]} test.pytest --chroot \
--test-pytest-test-shard=${python_unit_shard} \
tests/python::
) || die "Core python test failure"
Expand Down
5 changes: 3 additions & 2 deletions src/python/pants/backend/python/tasks2/gather_sources.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
from pex.pex import PEX
from pex.pex_builder import PEXBuilder

from pants.backend.python.tasks2.pex_build_util import dump_sources, has_python_sources
from pants.backend.python.tasks2.pex_build_util import (dump_sources, has_python_sources,
has_resources)
from pants.invalidation.cache_manager import VersionedTargetSet
from pants.task.task import Task
from pants.util.dirutil import safe_concurrent_creation
Expand Down Expand Up @@ -40,7 +41,7 @@ def prepare(cls, options, round_manager):
round_manager.require_data('python') # For codegen.

def execute(self):
targets = self.context.targets(predicate=has_python_sources)
targets = self.context.targets(predicate=lambda t: has_python_sources(t) or has_resources(t))
interpreter = self.context.products.get_data(PythonInterpreter)

with self.invalidated(targets) as invalidation_check:
Expand Down
17 changes: 11 additions & 6 deletions src/python/pants/backend/python/tasks2/pex_build_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,19 @@
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.resources import Resources
from pants.build_graph.files import Files
from pants.python.python_repos import PythonRepos


def has_python_sources(tgt):
# We'd like to take all PythonTarget subclasses, but currently PythonThriftLibrary and
# PythonAntlrLibrary extend PythonTarget, and until we fix that (which we can't do until
# we remove the old python pipeline entirely) we want to ignore those target types here.
return isinstance(tgt, (PythonLibrary, PythonTests, PythonBinary, Resources))
return isinstance(tgt, (PythonLibrary, PythonTests, PythonBinary))


def has_resources(tgt):
return isinstance(tgt, Resources)
return isinstance(tgt, Files)


def has_python_requirements(tgt):
Expand All @@ -41,10 +41,15 @@ def has_python_requirements(tgt):
def dump_sources(builder, tgt, log):
buildroot = get_buildroot()
log.debug(' Dumping sources: {}'.format(tgt))
for relpath in tgt.sources_relative_to_source_root():
for relpath in tgt.sources_relative_to_buildroot():
try:
src = os.path.join(buildroot, tgt.target_base, relpath)
if isinstance(tgt, Resources):
if type(tgt) == Files:
src = os.path.join(buildroot, relpath)
else:
relpath = os.path.relpath(relpath, tgt.target_base)
src = os.path.join(buildroot, tgt.target_base, relpath)

if has_resources(tgt):
builder.add_resource(src, relpath)
else:
builder.add_source(src, relpath)
Expand Down
55 changes: 42 additions & 13 deletions src/python/pants/backend/python/tasks2/pytest_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@
from pants.base.fingerprint_strategy import DefaultFingerprintStrategy
from pants.base.hash_utils import Sharder
from pants.base.workunit import WorkUnitLabel
from pants.build_graph.files import Files
from pants.build_graph.target import Target
from pants.invalidation.cache_manager import VersionedTargetSet
from pants.task.task import Task
from pants.task.testrunner_task_mixin import TestRunnerTaskMixin
from pants.util.contextutil import temporary_dir, temporary_file
from pants.util.contextutil import pushd, temporary_dir, temporary_file
from pants.util.dirutil import mergetree, safe_mkdir, safe_mkdir_for
from pants.util.memo import memoized_method, memoized_property
from pants.util.objects import datatype
Expand Down Expand Up @@ -142,6 +143,11 @@ def register_options(cls, register):
'will run in its own pytest invocation, which will be slower, but isolates '
'tests from process-wide state created by tests in other targets.')

register('--chroot', advanced=True, fingerprint=True, type=bool, default=False,
help='Run tests in a chroot. Any loose files tests depend on via `{}` dependencies '
'will be copied to the chroot.'
.format(Files.alias()))

# NB: We always produce junit xml privately, and if this option is specified, we then copy
# it to the user-specified directory, post any interaction with the cache to retrieve the
# privately generated and cached xml files. As such, this option is not part of the
Expand Down Expand Up @@ -286,8 +292,7 @@ def _maybe_emit_coverage_data(self, workdirs, targets, pex):
yield []
return

pex_src_root = os.path.relpath(
self.context.products.get_data(GatherSources.PYTHON_SOURCES).path(), get_buildroot())
pex_src_root = os.path.relpath(self._source_chroot_path, get_buildroot())

source_mappings = {}
for target in targets:
Expand Down Expand Up @@ -500,8 +505,7 @@ def _do_run_tests_with_args(self, pex, args):
return PytestResult.exception()

def _map_relsrc_to_targets(self, targets):
pex_src_root = os.path.relpath(
self.context.products.get_data(GatherSources.PYTHON_SOURCES).path(), get_buildroot())
pex_src_root = os.path.relpath(self._source_chroot_path, get_buildroot())
# First map chrooted sources back to their targets.
relsrc_to_target = {os.path.join(pex_src_root, src): target for target in targets
for src in target.sources_relative_to_source_root()}
Expand Down Expand Up @@ -717,13 +721,16 @@ def _run_pytest(self, workdirs, targets):
if not targets:
return PytestResult.rc(0)

buildroot = get_buildroot()
source_chroot = os.path.relpath(
self.context.products.get_data(GatherSources.PYTHON_SOURCES).path(), buildroot)
if self._run_in_chroot:
path_func = lambda rel_src: rel_src
else:
source_chroot = os.path.relpath(self._source_chroot_path, get_buildroot())
path_func = lambda rel_src: os.path.join(source_chroot, rel_src)

sources_map = {} # Path from chroot -> Path from buildroot.
for t in targets:
for p in t.sources_relative_to_source_root():
sources_map[os.path.join(source_chroot, p)] = os.path.join(t.target_base, p)
sources_map[path_func(p)] = os.path.join(t.target_base, p)

if not sources_map:
return PytestResult.rc(0)
Expand Down Expand Up @@ -780,15 +787,37 @@ def parse_error_handler(parse_error):

return result.with_failed_targets(failed_targets)

@memoized_property
def _source_chroot_path(self):
return self.context.products.get_data(GatherSources.PYTHON_SOURCES).path()

def _pex_run(self, pex, workunit_name, args, env):
with self.context.new_workunit(name=workunit_name,
cmd=pex.cmdline(args),
labels=[WorkUnitLabel.TOOL, WorkUnitLabel.TEST]) as workunit:
process = self._spawn(pex, workunit, args, setsid=False, env=env)
return process.wait()

@property
def _run_in_chroot(self):
return self.get_options().chroot

@contextmanager
def _maybe_run_in_chroot(self):
if self._run_in_chroot:
with pushd(self._source_chroot_path):
yield
else:
yield

def _spawn(self, pex, workunit, args, setsid=False, env=None):
env = env or {}
process = pex.run(args, blocking=False, setsid=setsid, env=env,
stdout=workunit.output('stdout'), stderr=workunit.output('stderr'))
return SubprocessProcessHandler(process)
with self._maybe_run_in_chroot():
env = env or {}
process = pex.run(args,
with_chroot=False, # We handle chrooting ourselves.
blocking=False,
setsid=setsid,
env=env,
stdout=workunit.output('stdout'),
stderr=workunit.output('stderr'))
return SubprocessProcessHandler(process)
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from pants.backend.python.tasks2.resolve_requirements import ResolveRequirements
from pants.backend.python.tasks2.resolve_requirements_task_base import ResolveRequirementsTaskBase
from pants.build_graph.address import Address
from pants.build_graph.resources import Resources
from pants.build_graph.files import Files
from pants.invalidation.cache_manager import VersionedTargetSet
from pants.util.dirutil import safe_concurrent_creation

Expand Down Expand Up @@ -101,7 +101,7 @@ def extra_requirements(self):
def create_pex(self, pex_info=None):
"""Returns a wrapped pex that "merges" the other pexes via PEX_PATH."""
relevant_targets = self.context.targets(
lambda tgt: isinstance(tgt, (PythonRequirementLibrary, PythonTarget, Resources)))
lambda tgt: isinstance(tgt, (PythonRequirementLibrary, PythonTarget, Files)))
with self.invalidated(relevant_targets) as invalidation_check:

# If there are no relevant targets, we still go through the motions of resolving
Expand Down
16 changes: 6 additions & 10 deletions src/python/pants/java/executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def execute(self, classpath, main, jvm_options=None, args=None, stdout=None, std
Raises Executor.Error if there was a problem launching java itself.
"""
runner = self.runner(classpath=classpath, main=main, jvm_options=jvm_options, args=args,
cwd=cwd)
cwd=cwd)
return runner.run(stdout=stdout, stderr=stderr)

@abstractmethod
Expand Down Expand Up @@ -205,12 +205,8 @@ def __init__(self, distribution):
self._buildroot = get_buildroot()
self._process = None

def _create_command(self, classpath, main, jvm_options, args, cwd=None):
cwd = cwd or self._buildroot
return super(SubprocessExecutor, self)._create_command(classpath, main, jvm_options,
args, cwd=cwd)

def _runner(self, classpath, main, jvm_options, args, cwd=None):
cwd = cwd or os.getcwd()
command = self._create_command(classpath, main, jvm_options, args, cwd=cwd)

class Runner(self.Runner):
Expand All @@ -223,10 +219,10 @@ def command(_):
return list(command)

def spawn(_, stdout=None, stderr=None):
return self._spawn(command, stdout=stdout, stderr=stderr, cwd=cwd)
return self._spawn(command, cwd, stdout=stdout, stderr=stderr)

def run(_, stdout=None, stderr=None):
return self._spawn(command, stdout=stdout, stderr=stderr, cwd=cwd).wait()
return self._spawn(command, cwd, stdout=stdout, stderr=stderr).wait()

return Runner()

Expand All @@ -239,12 +235,12 @@ def spawn(self, classpath, main, jvm_options=None, args=None, cwd=None, **subpro
:raises: :class:`Executor.Error` if there is a problem spawning the subprocess.
"""
cwd = cwd or os.getcwd()
cmd = self._create_command(*self._scrub_args(classpath, main, jvm_options, args, cwd=cwd))
return self._spawn(cmd, cwd, **subprocess_args)

def _spawn(self, cmd, cwd=None, **subprocess_args):
def _spawn(self, cmd, cwd, **subprocess_args):
with self._maybe_scrubbed_env():
cwd = cwd or self._buildroot
logger.debug('Executing: {cmd} args={args} at cwd={cwd}'
.format(cmd=' '.join(cmd), args=subprocess_args, cwd=cwd))
try:
Expand Down
1 change: 1 addition & 0 deletions tests/python/pants_test/logging/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

python_tests(
sources=globs('*.py', exclude=['test_workunit_label.py']),
dependencies=[
'3rdparty/python:six',
'src/python/pants/logging',
Expand Down

0 comments on commit a9d34c4

Please sign in to comment.