Support running python tests in the pex chroot. (#5033)

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 #5019

.travis.yml

@@ -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"
 

3rdparty/python/BUILD

@@ -17,3 +17,9 @@ python_requirement_library(
                        repository='http://www.antlr3.org/download/Python/')
   ]
 )
+
+# NB: Needed only for tests: tests/python/pants_test/engine/legacy:graph.
+files(
+  name='requirements_files',
+  sources=rglobs('**/requirements.txt'),
+)

build-support/bin/ci.sh

@@ -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"

src/python/pants/backend/python/tasks2/gather_sources.py

@@ -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
@@ -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:

src/python/pants/backend/python/tasks2/pex_build_util.py

@@ -19,38 +19,50 @@
 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)) and tgt.has_sources()
 
 
 def has_resources(tgt):
-  return isinstance(tgt, Resources)
+  return isinstance(tgt, Files) and tgt.has_sources()
 
 
 def has_python_requirements(tgt):
   return isinstance(tgt, PythonRequirementLibrary)
 
 
-def dump_sources(builder, tgt, log):
+def _create_source_dumper(builder, tgt):
+  if type(tgt) == Files:
+    # Loose `Files` as opposed to `Resources` or `PythonTarget`s have no (implied) package structure
+    # and so we chroot them relative to the build root so that they can be accessed via the normal
+    # python filesystem APIs just as they would be accessed outside the chrooted environment.
+    # NB: This requires we mark the pex as not zip safe so these `Files` can still be accessed in
+    # the context of a built pex distribution.
+    chroot_path = lambda relpath: relpath
+    builder.info.zip_safe = False
+  else:
+    chroot_path = lambda relpath: os.path.relpath(relpath, tgt.target_base)
+
+  dump = builder.add_resource if has_resources(tgt) else builder.add_source
   buildroot = get_buildroot()
+  return lambda relpath: dump(os.path.join(buildroot, relpath), chroot_path(relpath))
+
+
+def dump_sources(builder, tgt, log):
+  dump_source = _create_source_dumper(builder, tgt)
   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):
-        builder.add_resource(src, relpath)
-      else:
-        builder.add_source(src, relpath)
+      dump_source(relpath)
     except OSError:
-      log.error('Failed to copy {} for target {}'.format(
-        os.path.join(tgt.target_base, relpath), tgt.address.spec))
+      log.error('Failed to copy {} for target {}'.format(relpath, tgt.address.spec))
       raise
 
   if (getattr(tgt, '_resource_target_specs', None) or

src/python/pants/backend/python/tasks2/pytest_run.py

@@ -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
@@ -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
@@ -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:
@@ -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()}
@@ -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)
@@ -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)

src/python/pants/backend/python/tasks2/python_execution_task_base.py

@@ -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
 
@@ -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

src/python/pants/java/executor.py

@@ -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
@@ -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):
@@ -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()
 
@@ -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:

tests/python/pants_test/backend/python/tasks2/BUILD

@@ -22,8 +22,10 @@ python_tests(
     'src/python/pants/build_graph',
     'src/python/pants/fs',
     'src/python/pants/python',
+    'src/python/pants/source',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:dirutil',
+    'src/python/pants/util:process_handler',
     'tests/python/pants_test/backend/python/tasks:python_task_test_base',
     'tests/python/pants_test/subsystem:subsystem_utils',
     'tests/python/pants_test/tasks:task_test_base',

tests/python/pants_test/backend/python/tasks2/test_gather_sources.py

@@ -13,8 +13,10 @@
 from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_library import PythonLibrary
 from pants.backend.python.tasks2.gather_sources import GatherSources
+from pants.build_graph.files import Files
 from pants.build_graph.resources import Resources
 from pants.python.python_repos import PythonRepos
+from pants.source.source_root import SourceRootConfig
 from pants_test.tasks.task_test_base import TaskTestBase
 
 
@@ -23,30 +25,48 @@ class GatherSourcesTest(TaskTestBase):
   def task_type(cls):
     return GatherSources
 
-  def test_gather_sources(self):
-    filemap = {
-      'src/python/foo.py': 'foo_py_content',
-      'src/python/bar.py': 'bar_py_content',
-      'src/python/baz.py': 'baz_py_content',
+  def setUp(self):
+    super(GatherSourcesTest, self).setUp()
+
+    self.filemap = {
+      'src/python/one/foo.py': 'foo_py_content',
+      'src/python/one/bar.py': 'bar_py_content',
+      'src/python/two/baz.py': 'baz_py_content',
       'resources/qux/quux.txt': 'quux_txt_content',
     }
+    # Pants does not do auto-detection of Resources target roots unless they are nested under some
+    # other source root so we erect a manual resources root here.
+    self.set_options_for_scope(SourceRootConfig.options_scope, source_roots={'resources': ()})
 
-    for rel_path, content in filemap.items():
+    for rel_path, content in self.filemap.items():
       self.create_file(rel_path, content)
 
-    sources1 = self.make_target(spec='//:sources1_tgt', target_type=PythonLibrary,
-                                sources=['src/python/foo.py', 'src/python/bar.py'])
-    sources2 = self.make_target(spec='//:sources2_tgt', target_type=PythonLibrary,
-                                sources=['src/python/baz.py'])
-    resources = self.make_target(spec='//:resources_tgt', target_type=Resources,
-                                 sources=['resources/qux/quux.txt'])
-    pex = self._gather_sources([sources1, sources2, resources])
-    pex_root = pex.cmdline()[1]
-
-    for rel_path, expected_content in filemap.items():
-      with open(os.path.join(pex_root, rel_path)) as infile:
-        content = infile.read()
-      self.assertEquals(expected_content, content)
+    self.sources1 = self.make_target(spec='src/python/one:sources1_tgt', target_type=PythonLibrary,
+                                     sources=['foo.py', 'bar.py'])
+    self.sources2 = self.make_target(spec='src/python/two:sources2_tgt', target_type=PythonLibrary,
+                                     sources=['baz.py'])
+    self.resources = self.make_target(spec='resources/qux:resources_tgt', target_type=Resources,
+                                      sources=['quux.txt'])
+    self.files = self.make_target(spec='resources/qux:files_tgt', target_type=Files,
+                                  sources=['quux.txt'])
+
+  def _assert_content(self, pex, relpath, prefix=None):
+    expected_content = self.filemap[os.path.join(prefix, relpath) if prefix else relpath]
+    with open(os.path.join(pex.path(), relpath)) as infile:
+      content = infile.read()
+    self.assertEquals(expected_content, content)
+
+  def test_gather_sources(self):
+    pex = self._gather_sources([self.sources1, self.sources2, self.resources])
+    self._assert_content(pex, 'one/foo.py', prefix='src/python')
+    self._assert_content(pex, 'one/bar.py', prefix='src/python')
+    self._assert_content(pex, 'two/baz.py', prefix='src/python')
+    self._assert_content(pex, 'qux/quux.txt', prefix='resources')
+
+  def test_gather_files(self):
+    pex = self._gather_sources([self.sources2, self.files])
+    self._assert_content(pex, 'two/baz.py', prefix='src/python')
+    self._assert_content(pex, 'resources/qux/quux.txt')
 
   def _gather_sources(self, target_roots):
     context = self.context(target_roots=target_roots, for_subsystems=[PythonSetup, PythonRepos])