A convenient mechanism for fetching binary tools via subsystems (#5443)

An easy way to generate subsystems representing binary tools.

The subsystem registers an option to set the tool's version,
and provides a convenient mechanism for retrieving the tool
lazily as needed.

Provides a convenient migration path from existing tool
version options to this standardized one.

This change switches just one usage to the new mechanism.
The others will be switched in followup changes.

src/python/pants/backend/codegen/protobuf/java/BUILD

@@ -4,6 +4,7 @@
 python_library(
   dependencies = [
     '3rdparty/python/twitter/commons:twitter.common.collections',
+    'src/python/pants/backend/codegen/protobuf/subsystem',
     'src/python/pants/backend/jvm/targets:java',
     'src/python/pants/backend/jvm/targets:jvm',
     'src/python/pants/backend/jvm/tasks:jar_import_products',
@@ -17,7 +18,6 @@ python_library(
     'src/python/pants/fs',
     'src/python/pants/goal:task_registrar',
     'src/python/pants/task',
-    'src/python/pants/util:memo',
     'src/python/pants/util:process_handler',
   ],
 )

src/python/pants/backend/codegen/protobuf/java/protobuf_gen.py

@@ -12,25 +12,24 @@
 from twitter.common.collections import OrderedSet
 
 from pants.backend.codegen.protobuf.java.java_protobuf_library import JavaProtobufLibrary
+from pants.backend.codegen.protobuf.subsystem.protoc import Protoc
 from pants.backend.jvm.targets.jar_library import JarLibrary
 from pants.backend.jvm.targets.java_library import JavaLibrary
 from pants.backend.jvm.tasks.jar_import_products import JarImportProducts
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TaskError
 from pants.base.workunit import WorkUnitLabel
-from pants.binaries.binary_util import BinaryUtil
 from pants.build_graph.address import Address
 from pants.fs.archive import ZIP
 from pants.task.simple_codegen_task import SimpleCodegenTask
-from pants.util.memo import memoized_property
 from pants.util.process_handler import subprocess
 
 
 class ProtobufGen(SimpleCodegenTask):
 
   @classmethod
   def subsystem_dependencies(cls):
-    return super(ProtobufGen, cls).subsystem_dependencies() + (BinaryUtil.Factory,)
+    return super(ProtobufGen, cls).subsystem_dependencies() + (Protoc.scoped(cls),)
 
   @classmethod
   def register_options(cls, register):
@@ -42,6 +41,7 @@ def register_options(cls, register):
     # proper invalidation of protobuf products in the face of plugin modification that affects
     # plugin outputs.
     register('--version', advanced=True, fingerprint=True,
+             removal_version='1.7.0.dev0', removal_hint='Use --protoc-version instead.',
              help='Version of protoc.  Used to create the default --javadeps and as part of '
                   'the path to lookup the tool with --pants-support-baseurls and '
                   '--pants-bootstrapdir.  When changing this parameter you may also need to '
@@ -56,6 +56,7 @@ def register_options(cls, register):
                   'Intended to help protoc find its plugins.',
              default=None)
     register('--supportdir', advanced=True,
+             removal_version='1.7.0.dev0', removal_hint='Will no longer be configurable.',
              help='Path to use for the protoc binary.  Used as part of the path to lookup the'
                   'tool under --pants-bootstrapdir.',
              default='bin/protobuf')
@@ -82,12 +83,9 @@ def __init__(self, *args, **kwargs):
     self.plugins = self.get_options().protoc_plugins or []
     self._extra_paths = self.get_options().extra_path or []
 
-  @memoized_property
+  @property
   def protobuf_binary(self):
-    binary_util = BinaryUtil.Factory.create()
-    return binary_util.select_binary(self.get_options().supportdir,
-                                     self.get_options().version,
-                                     'protoc')
+    return Protoc.scoped_instance(self).select(context=self.context)
 
   @property
   def javadeps(self):

src/python/pants/backend/codegen/protobuf/subsystem/BUILD

@@ -0,0 +1,8 @@
+# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+python_library(
+  dependencies = [
+    'src/python/pants/binaries:binary_util'
+  ],
+)

src/python/pants/backend/codegen/protobuf/subsystem/protoc.py

@@ -0,0 +1,17 @@
+# 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
+
+
+class Protoc(NativeTool):
+  options_scope = 'protoc'
+  support_dir = 'bin/protobuf'
+  default_version = '2.4.1'
+
+  deprecated_option_scope = 'gen.protoc'
+  deprecated_option_name = 'version'

src/python/pants/backend/jvm/subsystems/jvm_tool_mixin.py

@@ -16,6 +16,8 @@ class JvmToolMixin(object):
   """A mixin for registering and accessing JVM-based tools.
 
   Must be mixed in to something that can register and use options, e.g., a Task or a Subsystem.
+
+  :API: public
   """
   class InvalidToolClasspath(TaskError):
     """Indicates an invalid jvm tool classpath."""
@@ -81,7 +83,7 @@ def register_jvm_tool(cls,
                         removal_hint=None):
     """Registers a jvm tool under `key` for lazy classpath resolution.
 
-    Classpaths can be retrieved in `execute` scope via `tool_classpath`.
+    Classpaths can be retrieved in `execute` scope via `tool_classpath_from_products`.
 
     NB: If the tool's `main` class name is supplied the tool classpath will be shaded.
 
@@ -175,7 +177,8 @@ def tool_jar_from_products(self, products, key, scope):
                                       'jar, instead found {count}:\n\t{classpath}'.format(**params))
     return classpath[0]
 
-  def tool_classpath_from_products(self, products, key, scope):
+  @staticmethod
+  def tool_classpath_from_products(products, key, scope):
     """Get a classpath for the tool previously registered under key in the given scope.
 
     :param products: The products of the current pants run.

src/python/pants/binaries/BUILD

@@ -3,16 +3,18 @@
 
 python_library(
   name='binary_util',
-  sources=['binary_util.py'],
+  sources=['binary_tool.py', 'binary_tool_mixin.py', 'binary_util.py'],
   dependencies=[
     '3rdparty/python/twitter/commons:twitter.common.collections',
     'src/python/pants/base:build_environment',
+    'src/python/pants/base:deprecated',
     'src/python/pants/base:exceptions',
     'src/python/pants/net',
     'src/python/pants/option',
     'src/python/pants/subsystem',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:dirutil',
+    'src/python/pants/util:memo',
     'src/python/pants/util:osutil',
   ],
 )

src/python/pants/binaries/binary_tool.py

@@ -0,0 +1,92 @@
+# 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_util import BinaryUtil
+from pants.subsystem.subsystem import Subsystem
+from pants.util.memo import memoized_method
+
+
+class BinaryToolBase(Subsystem):
+  """Base class for subsytems that configure binary tools.
+
+  Typically, a specific subclass is created via create_binary_tool_subsystem_cls() below.
+  That subclass can be further subclassed, manually, e.g., to add any extra options.
+
+  :API: public
+  """
+  # Subclasses must set these to appropriate values for the tool they define.
+  # They must also set options_scope to the tool name as understood by BinaryUtil.
+  support_dir = None
+  platform_dependent = None
+  default_version = None
+
+  # Subclasses may set these to effect migration from an old --version option to this one.
+  # TODO(benjy): Remove these after migration to the mixin is complete.
+  replaces_scope = None
+  replaces_name = None
+
+  # Subclasses may set this to provide extra register() kwargs for the --version option.
+  extra_version_option_kwargs = None
+
+  @classmethod
+  def register_options(cls, register):
+    super(BinaryToolBase, cls).register_options(register)
+
+    version_registration_kwargs = {
+      'type': str,
+      'default': cls.default_version,
+    }
+    if cls.extra_version_option_kwargs:
+      version_registration_kwargs.update(cls.extra_version_option_kwargs)
+    version_registration_kwargs['help'] = (
+      version_registration_kwargs.get('help') or
+      'Version of the {} {} to use'.format(cls.options_scope,
+                                           'binary' if cls.platform_dependent else 'script')
+    )
+    # The default for fingerprint in register() is False, but we want to default to True.
+    if 'fingerprint' not in version_registration_kwargs:
+      version_registration_kwargs['fingerprint'] = True
+    register('--version', **version_registration_kwargs)
+
+  def select(self, context=None):
+    """Returns the path to 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
+    """
+    version = self.get_options().version
+    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 not old_opts.is_default(self.replaces_name):
+        version = old_opts.get(self.replaces_name)
+    return self._select_for_version(version)
+
+  @memoized_method
+  def _select_for_version(self, version):
+    return BinaryUtil.Factory.create().select(
+      self.support_dir, version, self.options_scope, self.platform_dependent)
+
+
+class NativeTool(BinaryToolBase):
+  """A base class for native-code tools.
+
+  :API: public
+  """
+  platform_dependent = True
+
+
+class Script(BinaryToolBase):
+  """A base class for platform-independent scripts.
+
+  :API: public
+  """
+  platform_dependent = False

src/python/pants/binaries/binary_util.py

@@ -129,6 +129,45 @@ def __init__(self, baseurls, timeout_secs, bootstrapdir, path_by_id=None):
     if path_by_id:
       self._path_by_id.update((tuple(k), tuple(v)) for k, v in path_by_id.items())
 
+  def select(self, supportdir, version, name, platform_dependent):
+    if platform_dependent:
+      return self._select_binary(supportdir, version, name)
+    else:
+      return self._select_script(supportdir, version, name)
+
+  def select_binary(self, supportdir, version, name):
+    return self._select_binary(supportdir, version, name)
+
+  def select_script(self, supportdir, version, name):
+    return self._select_script(supportdir, version, name)
+
+  # TODO: Deprecate passing in an explicit supportdir? Seems like we should be able to
+  # organize our binary hosting so that it's not needed.
+  def _select_binary(self, supportdir, version, name):
+    """Selects a binary matching the current os and architecture.
+
+    :param string supportdir: The path the `name` binaries are stored under.
+    :param string version: The version number of the binary to select.
+    :param string name: The name of the binary to fetch.
+    :raises: :class:`pants.binary_util.BinaryUtil.BinaryNotFound` if no binary of the given version
+      and name could be found for the current platform.
+    """
+    # TODO(John Sirois): finish doc of the path structure expected under base_path.
+    binary_path = self._select_binary_base_path(supportdir, version, name)
+    return self._fetch_binary(name=name, binary_path=binary_path)
+
+  def _select_script(self, supportdir, version, name):
+    """Selects a platform-independent script.
+
+    :param string supportdir: The path the `name` scripts are stored under.
+    :param string version: The version number of the script to select.
+    :param string name: The name of the script to fetch.
+    :raises: :class:`pants.binary_util.BinaryUtil.BinaryNotFound` if no script of the given version
+      and name could be found.
+    """
+    binary_path = os.path.join(supportdir, version, name)
+    return self._fetch_binary(name=name, binary_path=binary_path)
+
   @contextmanager
   def _select_binary_stream(self, name, binary_path, fetcher=None):
     """Select a binary matching the current os and architecture.
@@ -167,31 +206,6 @@ def _select_binary_stream(self, name, binary_path, fetcher=None):
     if not downloaded_successfully:
       raise self.BinaryNotFound(binary_path, accumulated_errors)
 
-  def select_binary(self, supportdir, version, name):
-    """Selects a binary matching the current os and architecture.
-
-    :param string supportdir: The path the `name` binaries are stored under.
-    :param string version: The version number of the binary to select.
-    :param string name: The name of the binary to fetch.
-    :raises: :class:`pants.binary_util.BinaryUtil.BinaryNotFound` if no binary of the given version
-      and name could be found for the current platform.
-    """
-    # TODO(John Sirois): finish doc of the path structure expected under base_path.
-    binary_path = self._select_binary_base_path(supportdir, version, name)
-    return self._fetch_binary(name=name, binary_path=binary_path)
-
-  def select_script(self, supportdir, version, name):
-    """Selects a platform-independent script.
-
-    :param string supportdir: The path the `name` scripts are stored under.
-    :param string version: The version number of the script to select.
-    :param string name: The name of the script to fetch.
-    :raises: :class:`pants.binary_util.BinaryUtil.BinaryNotFound` if no script of the given version
-      and name could be found.
-    """
-    binary_path = os.path.join(supportdir, version, name)
-    return self._fetch_binary(name=name, binary_path=binary_path)
-
   def _fetch_binary(self, name, binary_path):
     bootstrap_dir = os.path.realpath(os.path.expanduser(self._pants_bootstrapdir))
     bootstrapped_binary_path = os.path.join(bootstrap_dir, binary_path)