Fixup PythonChroot to ignore synthetic targets.

Since PythonChroot orchestrates all its codegen locally, any codegen
done upstream is not relevant.

Tests were added for both thrift and antlr codegen and each necessitated
some small refactors of the respective python builders.

Testing Done:
Used against https://github.com/wyattanderson/pantstest to verify
that issue is fixed.

CI went green here:
  https://travis-ci.org/pantsbuild/pants/builds/72418529

Bugs closed: 1858, 1862

Reviewed at https://rbcommons.com/s/twitter/r/2523/

src/python/pants/backend/python/antlr_builder.py

@@ -19,6 +19,11 @@ class PythonAntlrBuilder(CodeGenerator):
   """
     Antlr builder.
   """
+
+  def __init__(self, ivy_bootstrapper, *args, **kwargs):
+    super(PythonAntlrBuilder, self).__init__(*args, **kwargs)
+    self._ivy_bootstrapper = ivy_bootstrapper
+
   def run_antlrs(self, output_dir):
     # TODO(John Sirois): graduate to a JvmToolTask and/or merge with the java code gen AntlrGen
     # task.
@@ -33,7 +38,7 @@ def run_antlrs(self, output_dir):
       args.append(abs_path)
 
     try:
-      ivy = Bootstrapper.default_ivy()
+      ivy = self._ivy_bootstrapper.ivy()
       ivy.execute(args=args)  # TODO: Needs a workunit, when we have a context here.
     except (Bootstrapper.Error, Ivy.Error) as e:
       raise TaskError('ANTLR generation failed! {0}'.format(e))

src/python/pants/backend/python/python_chroot.py

@@ -60,6 +60,7 @@ def get_platforms(platform_list):
   def __init__(self,
                python_setup,
                python_repos,
+               ivy_bootstrapper,
                thrift_binary_factory,
                interpreter,
                builder,
@@ -68,6 +69,7 @@ def __init__(self,
                extra_requirements=None):
     self._python_setup = python_setup
     self._python_repos = python_repos
+    self._ivy_bootstrapper = ivy_bootstrapper
     self._thrift_binary_factory = thrift_binary_factory
 
     self._interpreter = interpreter
@@ -161,12 +163,20 @@ def _generate_thrift_requirement(self, library):
 
   def _generate_antlr_requirement(self, library):
     antlr_builder = functools.partial(PythonAntlrBuilder,
+                                      ivy_bootstrapper=self._ivy_bootstrapper,
                                       workdir=safe_mkdtemp(dir=self.path(), prefix='antlr.'))
     return self._generate_requirement(library, antlr_builder)
 
   def resolve(self, targets):
     children = defaultdict(OrderedSet)
+
     def add_dep(trg):
+      # Currently we handle all of our code generation, so we don't want to operate over any
+      # synthetic targets injected upstream.
+      # TODO(John Sirois): Revisit this when building a proper python product pipeline.
+      if trg.is_synthetic:
+        return
+
       for target_type, target_key in self._VALID_DEPENDENCIES.items():
         if isinstance(trg, target_type):
           children[target_key].add(trg)

src/python/pants/backend/python/tasks/BUILD

@@ -29,6 +29,7 @@ python_library(
     'src/python/pants/base:target',
     'src/python/pants/base:workunit',
     'src/python/pants/console:stty_utils',
+    'src/python/pants/ivy',
     'src/python/pants/option',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:dirutil',

src/python/pants/backend/python/tasks/python_task.py

@@ -20,6 +20,8 @@
 from pants.backend.python.python_setup import PythonRepos, PythonSetup
 from pants.base import hash_utils
 from pants.base.exceptions import TaskError
+from pants.ivy.bootstrapper import Bootstrapper
+from pants.ivy.ivy_subsystem import IvySubsystem
 from pants.thrift_util import ThriftBinary
 
 
@@ -29,7 +31,7 @@ class PythonTask(Task):
 
   @classmethod
   def global_subsystems(cls):
-    return super(PythonTask, cls).global_subsystems() + (PythonSetup, PythonRepos)
+    return super(PythonTask, cls).global_subsystems() + (IvySubsystem, PythonSetup, PythonRepos)
 
   @classmethod
   def task_subsystems(cls):
@@ -103,13 +105,18 @@ def select_interpreter(self, filters):
   def chroot_cache_dir(self):
     return PythonSetup.global_instance().chroot_cache_dir
 
+  @property
+  def ivy_bootstrapper(self):
+    return Bootstrapper(ivy_subsystem=IvySubsystem.global_instance())
+
   @property
   def thrift_binary_factory(self):
-    return ThriftBinary.Factory.scoped_instance(self)
+    return ThriftBinary.Factory.scoped_instance(self).create
 
   def create_chroot(self, interpreter, builder, targets, platforms, extra_requirements):
     return PythonChroot(python_setup=PythonSetup.global_instance(),
                         python_repos=PythonRepos.global_instance(),
+                        ivy_bootstrapper=self.ivy_bootstrapper,
                         thrift_binary_factory=self.thrift_binary_factory,
                         interpreter=interpreter,
                         builder=builder,

src/python/pants/backend/python/tasks/setup_py.py

@@ -429,6 +429,7 @@ def __init__(self, *args, **kwargs):
   def generated_targets(self):
     return {
       PythonAntlrLibrary: functools.partial(PythonAntlrBuilder,
+                                            ivy_bootstrapper=self.ivy_bootstrapper,
                                             workdir=os.path.join(self.workdir, 'antlr')),
       PythonThriftLibrary: functools.partial(PythonThriftBuilder,
                                              thrift_binary_factory=self.thrift_binary_factory,

src/python/pants/backend/python/thrift_builder.py

@@ -36,7 +36,7 @@ def install_requires(self):
 
   @memoized_property
   def _thrift_binary(self):
-    return self._thrift_binary_factory.create().path
+    return self._thrift_binary_factory().path
 
   def run_thrifts(self):
     """Generate Python thrift code.

src/python/pants/ivy/bootstrapper.py

@@ -59,9 +59,9 @@ def default_ivy(cls, bootstrap_workunit_factory=None):
     """
     return cls.instance().ivy(bootstrap_workunit_factory=bootstrap_workunit_factory)
 
-  def __init__(self, *args, **kwargs):
+  def __init__(self, ivy_subsystem=None):
     """Creates an ivy bootstrapper."""
-    self._ivy_subsystem = IvySubsystem.global_instance()
+    self._ivy_subsystem = ivy_subsystem or IvySubsystem.global_instance()
     self._version_or_ivyxml = self._ivy_subsystem.get_options().ivy_profile
     self._classpath = None
 

tests/python/pants_test/backend/python/BUILD

@@ -15,7 +15,20 @@ python_tests(
   sources=['test_python_chroot.py'],
   dependencies=[
     '3rdparty/python:pex',
+    'src/python/pants/backend/codegen/targets:python',
+    'src/python/pants/backend/python/targets:python',
+    'src/python/pants/backend/python:interpreter_cache',
     'src/python/pants/backend/python:python_chroot',
+    'src/python/pants/backend/python:python_requirement',
+    'src/python/pants/backend/python:python_setup',
+    'src/python/pants/base:build_environment',
+    'src/python/pants/base:source_root',
+    'src/python/pants/ivy',
+    'src/python/pants/util:contextutil',
+    'src/python/pants:binary_util',
+    'src/python/pants:thrift_util',
+    'tests/python/pants_test/base:context_utils',
+    'tests/python/pants_test:base_test',
   ]
 )
 

tests/python/pants_test/backend/python/test_python_chroot.py

@@ -5,15 +5,215 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
-import unittest
+import os
+import subprocess
+from contextlib import contextmanager
+from textwrap import dedent
 
+from pex.pex_builder import PEXBuilder
 from pex.platforms import Platform
 
+from pants.backend.codegen.targets.python_antlr_library import PythonAntlrLibrary
+from pants.backend.codegen.targets.python_thrift_library import PythonThriftLibrary
+from pants.backend.python.interpreter_cache import PythonInterpreterCache
 from pants.backend.python.python_chroot import PythonChroot
+from pants.backend.python.python_requirement import PythonRequirement
+from pants.backend.python.python_setup import PythonRepos, PythonSetup
+from pants.backend.python.targets.python_binary import PythonBinary
+from pants.backend.python.targets.python_library import PythonLibrary
+from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
+from pants.base.build_environment import get_pants_cachedir
+from pants.base.source_root import SourceRoot
+from pants.binary_util import BinaryUtil
+from pants.ivy.bootstrapper import Bootstrapper
+from pants.ivy.ivy_subsystem import IvySubsystem
+from pants.thrift_util import ThriftBinary
+from pants.util.contextutil import temporary_dir
+from pants_test.base.context_utils import create_option_values
+from pants_test.base_test import BaseTest
 
 
-class PythonChrootTest(unittest.TestCase):
-  def test_get_current_platform(self):
-    expected_platforms = [Platform.current(), 'linux-x86_64']
-    self.assertEqual(set(expected_platforms),
-                     set(PythonChroot.get_platforms(['current', 'linux-x86_64'])))
+def test_get_current_platform():
+  expected_platforms = [Platform.current(), 'linux-x86_64']
+  assert set(expected_platforms) == set(PythonChroot.get_platforms(['current', 'linux-x86_64']))
+
+
+class PythonChrootTest(BaseTest):
+  @contextmanager
+  def dumped_chroot(self, targets):
+    def options(**kwargs):
+      return create_option_values(kwargs)
+
+    with temporary_dir() as chroot:
+      # TODO(John Sirois): Find a way to get easy access to pants option defaults.
+      python_setup_workdir = os.path.join(self.real_build_root, '.pants.d', 'python-setup')
+
+      python_setup_options = options(
+          artifact_cache_dir=os.path.join(python_setup_workdir, 'artifacts'),
+          interpreter_cache_dir=os.path.join(python_setup_workdir, 'interpreters'),
+          interpreter_requirement='CPython>=2.7,<3',
+          resolver_cache_dir=os.path.join(python_setup_workdir, 'resolved_requirements'),
+          resolver_cache_ttl=None,
+          setuptools_version='5.4.1',
+          wheel_version='0.24.0')
+      python_setup = PythonSetup('test-scope', python_setup_options)
+      python_repos = PythonRepos('test-scope',
+                                 options(repos=[], indexes=['https://pypi.python.org/simple/']))
+
+      ivy_subsystem_options = options(ivy_profile='2.4.0',
+                                      ivy_settings=None,
+                                      cache_dir=os.path.expanduser('~/.ivy2/pants'),
+                                      http_proxy=None,
+                                      https_proxy=None,
+                                      pants_bootstrapdir=get_pants_cachedir())
+      ivy_subsystem = IvySubsystem('test-scope', ivy_subsystem_options)
+      ivy_bootstrapper = Bootstrapper(ivy_subsystem=ivy_subsystem)
+
+      def thrift_binary_factory():
+        binary_util = BinaryUtil(baseurls=['https://dl.bintray.com/pantsbuild/bin/build-support'],
+                                 timeout_secs=30,
+                                 bootstrapdir=get_pants_cachedir())
+        return ThriftBinary(binary_util=binary_util, relpath='bin/thrift', version='0.9.2')
+
+      interpreter_cache = PythonInterpreterCache(python_setup, python_repos)
+      interpreter_cache.setup()
+      interpreters = list(interpreter_cache.matches([python_setup.interpreter_requirement]))
+      self.assertGreater(len(interpreters), 0)
+      interpreter = interpreters[0]
+
+      pex_builder = PEXBuilder(path=chroot, interpreter=interpreter)
+
+      python_chroot = PythonChroot(python_setup=python_setup,
+                                   python_repos=python_repos,
+                                   ivy_bootstrapper=ivy_bootstrapper,
+                                   thrift_binary_factory=thrift_binary_factory,
+                                   interpreter=interpreter,
+                                   builder=pex_builder,
+                                   targets=targets,
+                                   platforms=['current'])
+      try:
+        python_chroot.dump()
+        yield pex_builder, python_chroot
+      finally:
+        python_chroot.delete()
+
+  def test_antlr(self):
+    SourceRoot.register('src/antlr', PythonThriftLibrary)
+    self.create_file(relpath='src/antlr/word/word.g', contents=dedent("""
+      grammar word;
+
+      options {
+        language=Python;
+        output=AST;
+      }
+
+      WORD: ('a'..'z'|'A'..'Z'|'!')+;
+
+      word_up: WORD (' ' WORD)*;
+    """))
+    antlr_target = self.make_target(spec='src/antlr/word',
+                                    target_type=PythonAntlrLibrary,
+                                    antlr_version='3.1.3',
+                                    sources=['word.g'],
+                                    module='word')
+
+    SourceRoot.register('src/python', PythonBinary)
+    antlr3 = self.make_target(spec='3rdparty/python:antlr3',
+                              target_type=PythonRequirementLibrary,
+                              requirements=[PythonRequirement('antlr_python_runtime==3.1.3')])
+    self.create_file(relpath='src/python/test/main.py', contents=dedent("""
+      import antlr3
+
+      from word import wordLexer, wordParser
+
+
+      def word_up():
+        input = 'Hello World!'
+        char_stream = antlr3.ANTLRStringStream(input)
+        lexer = wordLexer.wordLexer(char_stream)
+        tokens = antlr3.CommonTokenStream(lexer)
+        parser = wordParser.wordParser(tokens)
+
+        def print_node(node):
+          print(node.text)
+        visitor = antlr3.tree.TreeVisitor()
+        visitor.visit(parser.word_up().tree, pre_action=print_node)
+    """))
+    binary = self.make_target(spec='src/python/test',
+                              target_type=PythonBinary,
+                              source='main.py',
+                              dependencies=[antlr_target, antlr3])
+
+    with self.dumped_chroot([binary]) as (pex_builder, python_chroot):
+      pex_builder.set_entry_point('test.main:word_up')
+      pex_builder.freeze()
+      pex = python_chroot.pex()
+
+      process = pex.run(blocking=False, stdout=subprocess.PIPE)
+      stdout, _ = process.communicate()
+
+      self.assertEqual(0, process.returncode)
+      self.assertEqual(['Hello', ' ', 'World!'], stdout.splitlines())
+
+  @contextmanager
+  def do_test_thrift(self):
+    SourceRoot.register('src/thrift', PythonThriftLibrary)
+    self.create_file(relpath='src/thrift/test/const.thrift', contents=dedent("""
+      namespace py test
+
+      const list<string> VALID_IDENTIFIERS = ["Hello", "World!"]
+    """))
+    thrift_target = self.make_target(spec='src/thrift/test',
+                                     target_type=PythonThriftLibrary,
+                                     sources=['const.thrift'])
+
+    SourceRoot.register('src/python', PythonBinary)
+    self.create_file(relpath='src/python/test/main.py', contents=dedent("""
+      from test.constants import VALID_IDENTIFIERS
+
+
+      def say_hello():
+        print(' '.join(VALID_IDENTIFIERS))
+    """))
+    binary = self.make_target(spec='src/python/test',
+                              target_type=PythonBinary,
+                              source='main.py',
+                              dependencies=[thrift_target])
+
+    yield binary, thrift_target
+
+    with self.dumped_chroot([binary]) as (pex_builder, python_chroot):
+      pex_builder.set_entry_point('test.main:say_hello')
+      pex_builder.freeze()
+      pex = python_chroot.pex()
+
+      process = pex.run(blocking=False, stdout=subprocess.PIPE)
+      stdout, _ = process.communicate()
+
+      self.assertEqual(0, process.returncode)
+      self.assertEqual('Hello World!', stdout.strip())
+
+  def test_thrift(self):
+    with self.do_test_thrift():
+      pass  # Run the test on a standard isolated pure python target graph.
+
+  def test_thrift_issues_1858(self):
+    # Confirm a synthetic target for our python_thrift_library from some upstream task does not
+    # trample the PythonChroot/PythonThriftBuilder generated code.
+    # In https://github.com/pantsbuild/pants/issues/1858 the ApacheThriftGen task in the 'gen'
+    # phase upstream of the 'binary' goal was injecting a synthetic python_library target owning
+    # thrift generated code _and_ that code was a subset of all the code generated by thrift; ie:
+    # there was a synthetic python_library being added directly to the chroot missing some thrift
+    # codegened '.py' files, leading to import of those files (and errors) instead of the
+    # PythonChroot/PythonThriftBuilder generated files (packaged as deps in the PythonChroot).
+    with self.do_test_thrift() as (binary, thrift_target):
+      SourceRoot.register('.synthetic', PythonLibrary)
+      self.create_file(relpath='.synthetic/test/__init__.py')
+      self.create_file(relpath='.synthetic/test/constants.py', contents=dedent("""
+        VALID_IDENTIFIERS = ['generated', 'by', 'upstream', 'and', 'different!']
+      """))
+      synthetic_pythrift_codegen_target = self.make_target(spec='.synthetic/test:constants',
+                                                           target_type=PythonLibrary,
+                                                           sources=['__init__.py', 'constants.py'],
+                                                           derived_from=thrift_target)
+      binary.inject_dependency(synthetic_pythrift_codegen_target.address)