A new Thrift binary tool subsystem.

contrib/go/src/python/pants/contrib/go/tasks/BUILD

@@ -12,7 +12,6 @@ python_library(
     'src/python/pants/base:exceptions',
     'src/python/pants/base:generator',
     'src/python/pants/base:workunit',
-    'src/python/pants/binaries:thrift_util',
     'src/python/pants/build_graph',
     'src/python/pants/option',
     'src/python/pants/process',

contrib/go/src/python/pants/contrib/go/tasks/go_thrift_gen.py

@@ -8,11 +8,11 @@
 import os
 import re
 
+from pants.backend.codegen.thrift.lib.thrift import Thrift
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TaskError
 from pants.base.revision import Revision
 from pants.base.workunit import WorkUnitLabel
-from pants.binaries.thrift_binary import ThriftBinary
 from pants.option.custom_types import target_option
 from pants.task.simple_codegen_task import SimpleCodegenTask
 from pants.util.dirutil import safe_mkdir
@@ -44,15 +44,26 @@ def register_options(cls, register):
 
   @classmethod
   def subsystem_dependencies(cls):
-    return super(GoThriftGen, cls).subsystem_dependencies() + (ThriftBinary.Factory.scoped(cls),)
+    return super(GoThriftGen, cls).subsystem_dependencies() + (Thrift.scoped(cls),)
 
-  @memoized_property
+  @property
   def _thrift_binary(self):
-    return ThriftBinary.Factory.scoped_instance(self).create()
+    return self._thrift.select(context=self.context)
+
+  @property
+  def _thrift_version(self):
+    return self._thrift.version(context=self.context)
+
+  @memoized_property
+  def _thrift(self):
+    return Thrift.scoped_instance(self)
 
   @memoized_property
   def _deps(self):
     thrift_import_target = self.get_options().thrift_import_target
+    if thrift_import_target is None:
+      raise TaskError('Option thrift_import_target in scope {} must be set.'.format(
+        self.options_scope))
     thrift_imports = self.context.resolve(thrift_import_target)
     return thrift_imports
 
@@ -93,18 +104,18 @@ def _validate_supports_more_than_one_source(self):
     # https://issues.apache.org/jira/browse/THRIFT-3776; first available in 0.10.0
     if self.get_options().multiple_files_per_target_override:
       return
-    actual_revision = Revision.semver(self._thrift_binary.version)
     required_version = '0.10.0'
-    if Revision.semver(required_version) <= actual_revision:
-      return
-    raise TaskError('A single .thrift source file is supported per go_thrift_library with thrift '
-                    'version `{}`: upgrade to at least `{}` to support multiple files.'.format(
-                      self._thrift_binary.version, required_version))
+    if  Revision.semver(self._thrift_version) < Revision.semver(required_version):
+      raise TaskError('A single .thrift source file is supported per go_thrift_library with thrift '
+                      'version `{}`: upgrade to at least `{}` to support multiple files.'.format(
+                      self._thrift_version, required_version))
 
   @memoized_property
   def _thrift_cmd(self):
-    cmd = [self._thrift_binary.path]
+    cmd = [self._thrift_binary]
     thrift_import = 'thrift_import={}'.format(self.get_options().thrift_import)
+    if thrift_import is None:
+      raise TaskError('Option thrift_import in scope {} must be set.'.format(self.options_scope))
     gen_options = self.get_options().gen_options
     if gen_options:
       gen_options += ',' + thrift_import
@@ -140,7 +151,7 @@ def _generate_thrift(self, target, target_workdir):
                                  stdout=workunit.output('stdout'),
                                  stderr=workunit.output('stderr'))
         if result != 0:
-          raise TaskError('{} ... exited non-zero ({})'.format(self._thrift_binary.path, result))
+          raise TaskError('{} ... exited non-zero ({})'.format(self._thrift_binary, result))
 
     gen_dir = os.path.join(target_workdir, 'gen-go')
     src_dir = os.path.join(target_workdir, 'src')

contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_thrift_gen.py

@@ -5,8 +5,8 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+from pants.backend.codegen.thrift.lib.thrift import Thrift
 from pants.base.exceptions import TaskError
-from pants.binaries.thrift_binary import ThriftBinary
 from pants_test.tasks.task_test_base import TaskTestBase
 
 from pants.contrib.go.tasks.go_thrift_gen import GoThriftGen
@@ -19,7 +19,7 @@ def task_type(cls):
     return GoThriftGen
 
   def _validate_for(self, version):
-    options = {ThriftBinary.Factory.options_scope: {'version': version}}
+    options = {Thrift.options_scope: {'version': version}}
     self.create_task(self.context(options=options))._validate_supports_more_than_one_source()
 
   def test_validate_source_too_low(self):

src/python/pants/backend/codegen/thrift/java/apache_thrift_java_gen.py

@@ -10,7 +10,6 @@
 from pants.backend.codegen.thrift.lib.apache_thrift_gen_base import ApacheThriftGenBase
 from pants.backend.jvm.targets.java_library import JavaLibrary
 from pants.base.exceptions import TargetDefinitionException
-from pants.binaries.thrift_binary import ThriftBinary
 
 
 # TODO: Currently the injected runtime deps are specified by the --deps option defined in the
@@ -29,8 +28,7 @@ class ApacheThriftJavaGen(ApacheThriftGenBase):
 
   @classmethod
   def subsystem_dependencies(cls):
-    return (super(ApacheThriftJavaGen, cls).subsystem_dependencies() +
-            (ThriftDefaults, ThriftBinary.Factory.scoped(cls)))
+    return super(ApacheThriftJavaGen, cls).subsystem_dependencies() + (ThriftDefaults,)
 
   @classmethod
   def implementation_version(cls):

src/python/pants/backend/codegen/thrift/lib/apache_thrift_gen_base.py

@@ -11,10 +11,10 @@
 
 from twitter.common.collections import OrderedSet
 
+from pants.backend.codegen.thrift.lib.thrift import Thrift
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TaskError
 from pants.base.workunit import WorkUnitLabel
-from pants.binaries.thrift_binary import ThriftBinary
 from pants.option.custom_types import target_option
 from pants.task.simple_codegen_task import SimpleCodegenTask
 from pants.util.memo import memoized_property
@@ -52,8 +52,7 @@ def register_options(cls, register):
 
   @classmethod
   def subsystem_dependencies(cls):
-    return (super(ApacheThriftGenBase, cls).subsystem_dependencies() +
-            (ThriftBinary.Factory.scoped(cls),))
+    return super(ApacheThriftGenBase, cls).subsystem_dependencies() + (Thrift.scoped(cls),)
 
   def synthetic_target_extra_dependencies(self, target, target_workdir):
     for source in target.sources_relative_to_buildroot():
@@ -95,8 +94,7 @@ def execute_codegen(self, target, target_workdir):
 
   @memoized_property
   def _thrift_binary(self):
-    thrift_binary = ThriftBinary.Factory.scoped_instance(self).create()
-    return thrift_binary.path
+    return Thrift.scoped_instance(self).select(context=self.context)
 
   @memoized_property
   def _deps(self):

src/python/pants/backend/codegen/thrift/lib/thrift.py

@@ -0,0 +1,23 @@
+# coding=utf-8
+# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
+                        unicode_literals, with_statement)
+
+from pants.binaries.binary_tool import NativeTool
+from pants.binaries.thrift_binary import ThriftBinary
+
+
+class Thrift(NativeTool):
+  @classmethod
+  def subsystem_dependencies(cls):
+    # Ensure registration of the ThriftBinary.Factory, so that the thrift-binary
+    # scope exists, for backwards compatibility during deprecation.
+    return super(Thrift, cls).subsystem_dependencies() + (ThriftBinary.Factory,)
+
+  options_scope = 'thrift'
+  default_version = '0.9.2'
+
+  replaces_scope = 'thrift-binary'
+  replaces_name = 'version'

src/python/pants/binaries/BUILD

@@ -25,6 +25,7 @@ python_library(
   sources=['thrift_binary.py'],
   dependencies=[
     ':binary_util',
+    'src/python/pants/base:deprecated',
     'src/python/pants/subsystem',
     'src/python/pants/util:memo',
   ],

src/python/pants/binaries/binary_tool.py

@@ -59,6 +59,7 @@ def register_options(cls, register):
       version_registration_kwargs['fingerprint'] = True
     register('--version', **version_registration_kwargs)
 
+  @memoized_method
   def select(self, context=None):
     """Returns the path to the specified binary tool.
 
@@ -69,13 +70,25 @@ def select(self, context=None):
 
     :API: public
     """
-    version = self.get_options().version
+    return self._select_for_version(self.version(context))
+
+  @memoized_method
+  def version(self, context=None):
+    """Returns the version of the specified binary tool.
+
+    If replaces_scope and replaces_name are defined, then the caller must pass in
+    a context, otherwise no context should be passed.
+
+    # TODO: Once we're migrated, get rid of the context arg.
+
+    :API: public
+    """
     if self.replaces_scope and self.replaces_name:
       # If the old option is provided explicitly, let it take precedence.
       old_opts = context.options.for_scope(self.replaces_scope)
       if old_opts.get(self.replaces_name) and not old_opts.is_default(self.replaces_name):
-        version = old_opts.get(self.replaces_name)
-    return self._select_for_version(version)
+        return old_opts.get(self.replaces_name)
+    return self.get_options().version
 
   @memoized_property
   def _binary_util(self):

src/python/pants/binaries/thrift_binary.py

@@ -5,6 +5,7 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+from pants.base.deprecated import deprecated
 from pants.binaries.binary_util import BinaryUtil
 from pants.subsystem.subsystem import Subsystem
 from pants.util.memo import memoized_property
@@ -29,12 +30,18 @@ def subsystem_dependencies(cls):
     @classmethod
     def register_options(cls, register):
       register('--supportdir', advanced=True, default='bin/thrift',
+               removal_version='1.7.0.dev0',
+               removal_hint='Use pants.backend.codegen.thrift.lib.thrift instead.',
                help='Find thrift binaries under this dir.   Used as part of the path to lookup the'
                     'tool with --binary-util-baseurls and --pants-bootstrapdir')
       register('--version', advanced=True, default='0.9.2', fingerprint=True,
+               removal_version='1.7.0.dev0',
+               removal_hint='Use pants.backend.codegen.thrift.lib.thrift instead.',
                help='Thrift compiler version.   Used as part of the path to lookup the'
                     'tool with --binary-util-baseurls and --pants-bootstrapdir')
 
+    @deprecated(removal_version='1.7.0.dev0',
+                hint_message='Use pants.backend.codegen.thrift.lib.thrift instead.')
     def create(self):
       """
       :API: public

tests/python/pants_test/backend/codegen/thrift/python/BUILD

@@ -5,6 +5,7 @@ python_tests(
   dependencies=[
     '3rdparty/python:pex',
     '3rdparty/python:six',
+    'src/python/pants/backend/codegen/thrift/lib',
     'src/python/pants/backend/codegen/thrift/python',
     'src/python/pants/backend/python:interpreter_cache',
     'src/python/pants/backend/python/subsystems',

tests/python/pants_test/backend/codegen/thrift/python/test_apache_thrift_py_gen.py

@@ -11,13 +11,13 @@
 import six
 from pex.resolver import resolve
 
+from pants.backend.codegen.thrift.lib.thrift import Thrift
 from pants.backend.codegen.thrift.python.apache_thrift_py_gen import ApacheThriftPyGen
 from pants.backend.codegen.thrift.python.python_thrift_library import PythonThriftLibrary
 from pants.backend.python.interpreter_cache import PythonInterpreterCache
 from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_library import PythonLibrary
 from pants.base.build_environment import get_buildroot
-from pants.binaries.thrift_binary import ThriftBinary
 from pants.python.python_repos import PythonRepos
 from pants.util.process_handler import subprocess
 from pants_test.subsystem.subsystem_util import global_subsystem_instance
@@ -30,10 +30,10 @@ class ApacheThriftPyGenTest(TaskTestBase):
   def task_type(cls):
     return ApacheThriftPyGen
 
-  def get_thrift_version(self, apache_thrift_gen):
-    thrift_binary_factory = global_subsystem_instance(ThriftBinary.Factory)
-    thrift_binary = thrift_binary_factory.scoped_instance(apache_thrift_gen).create()
-    return thrift_binary.version
+  @staticmethod
+  def get_thrift_version(apache_thrift_gen):
+    thrift = global_subsystem_instance(Thrift).scoped_instance(apache_thrift_gen)
+    return thrift.get_options().version
 
   def generate_single_thrift_target(self, python_thrift_library):
     context = self.context(target_roots=[python_thrift_library])