New-style BinaryTool Subsystems for isort and go distribution. (pants…

…build#5523)

Gets rid of the indirection via a Factory. It's easier to just
make the distribution itself the subsystem, since the base class
takes care of basically everything the factory previously did.

Also improved the fetching code to use a proper tmpdir instead
of a hardcoded name that might cause contention.

contrib/go/src/python/pants/contrib/go/subsystems/go_distribution.py

@@ -9,54 +9,26 @@
 from collections import OrderedDict, namedtuple
 
 from pants.base.workunit import WorkUnit, WorkUnitLabel
-from pants.binaries.binary_util import BinaryUtil
-from pants.fs.archive import TGZ
-from pants.subsystem.subsystem import Subsystem
-from pants.util.contextutil import temporary_dir
+from pants.binaries.binary_tool import NativeTool
 from pants.util.memo import memoized_property
 from pants.util.process_handler import subprocess
 
 
-class GoDistribution(object):
+class GoDistribution(NativeTool):
   """Represents a self-bootstrapping Go distribution."""
 
-  class Factory(Subsystem):
-    options_scope = 'go-distribution'
+  options_scope = 'go-distribution'
+  name = 'go'
+  default_version = '1.8.3'
+  archive_type = 'tgz'
 
-    @classmethod
-    def subsystem_dependencies(cls):
-      return (BinaryUtil.Factory,)
-
-    @classmethod
-    def register_options(cls, register):
-      register('--supportdir', advanced=True, default='bin/go',
-               help='Find the go distributions under this dir.  Used as part of the path to lookup '
-                    'the distribution with --binary-util-baseurls and --pants-bootstrapdir')
-      register('--version', advanced=True, default='1.8.3', fingerprint=True,
-               help='Go distribution version.  Used as part of the path to lookup the distribution '
-                    'with --binary-util-baseurls and --pants-bootstrapdir')
-
-    def create(self):
-      # NB: create is an instance method to allow the user to choose global or scoped.
-      # It's not unreasonable to imagine multiple go versions in play; for example: when
-      # transitioning from the 1.x series to the 2.x series.
-      binary_util = BinaryUtil.Factory.create()
-      options = self.get_options()
-      return GoDistribution(binary_util, options.supportdir, options.version)
-
-  def __init__(self, binary_util, relpath, version):
-    self._binary_util = binary_util
-    self._relpath = relpath
-    self._version = version
-
-  @property
-  def version(self):
-    """Returns the version of the Go distribution.
-
-    :returns: The Go distribution version number string.
-    :rtype: string
-    """
-    return self._version
+  @classmethod
+  def register_options(cls, register):
+    super(GoDistribution, cls).register_options(register)
+    register('--supportdir', advanced=True, default='bin/go',
+             removal_version='1.7.0.dev0', removal_hint='No longer supported.',
+             help='Find the go distributions under this dir.  Used as part of the path to lookup '
+                  'the distribution with --binary-util-baseurls and --pants-bootstrapdir')
 
   @memoized_property
   def goroot(self):
@@ -65,14 +37,14 @@ def goroot(self):
     :returns: The Go distribution $GOROOT.
     :rtype: string
     """
-    go_distribution = self._binary_util.select_binary(self._relpath, self.version, 'go.tar.gz')
-    distribution_workdir = os.path.dirname(go_distribution)
-    outdir = os.path.join(distribution_workdir, 'unpacked')
-    if not os.path.exists(outdir):
-      with temporary_dir(root_dir=distribution_workdir) as tmp_dist:
-        TGZ.extract(go_distribution, tmp_dist)
-        os.rename(tmp_dist, outdir)
-    return os.path.join(outdir, 'go')
+    #go_distribution = self.select()
+    # distribution_workdir = os.path.dirname(go_distribution)
+    # outdir = os.path.join(distribution_workdir, 'unpacked')
+    # if not os.path.exists(outdir):
+    #   with temporary_dir(root_dir=distribution_workdir) as tmp_dist:
+    #     TGZ.extract(go_distribution, tmp_dist)
+    #     os.rename(tmp_dist, outdir)
+    return os.path.join(self.select(), 'go')
 
   def go_env(self, gopath=None):
     """Return an env dict that represents a proper Go environment mapping for this distribution."""
@@ -89,14 +61,14 @@ class GoCommand(namedtuple('GoCommand', ['cmdline', 'env'])):
     """Encapsulates a go command that can be executed."""
 
     @classmethod
-    def _create(cls, goroot, cmd, go_env, args=None):
+    def create(cls, goroot, cmd, go_env, args=None):
       return cls([os.path.join(goroot, 'bin', 'go'), cmd] + (args or []), env=go_env)
 
     def spawn(self, env=None, **kwargs):
       """
       :param dict env: A custom environment to launch the Go command in.  If `None` the current
                        environment is used.
-      :param **kwargs: Keyword arguments to pass through to `subprocess.Popen`.
+      :param kwargs: Keyword arguments to pass through to `subprocess.Popen`.
       :returns: A handle to the spawned go command subprocess.
       :rtype: :class:`subprocess.Popen`
       """
@@ -109,7 +81,7 @@ def check_output(self, env=None, **kwargs):
 
       :param dict env: A custom environment to launch the Go command in.  If `None` the current
                        environment is used.
-      :param **kwargs: Keyword arguments to pass through to `subprocess.check_output`.
+      :param kwargs: Keyword arguments to pass through to `subprocess.check_output`.
       :return str: Output of Go command.
       :raises subprocess.CalledProcessError: Raises if Go command fails.
       """
@@ -132,7 +104,7 @@ def create_go_cmd(self, cmd, gopath=None, args=None):
     :returns: A go command that can be executed later.
     :rtype: :class:`GoDistribution.GoCommand`
     """
-    return self.GoCommand._create(self.goroot, cmd, go_env=self.go_env(gopath=gopath), args=args)
+    return self.GoCommand.create(self.goroot, cmd, go_env=self.go_env(gopath=gopath), args=args)
 
   def execute_go_cmd(self, cmd, gopath=None, args=None, env=None,
                      workunit_factory=None, workunit_name=None, workunit_labels=None, **kwargs):
@@ -149,11 +121,11 @@ def execute_go_cmd(self, cmd, gopath=None, args=None, env=None,
     :param workunit_factory: An optional callable that can produce a `WorkUnit` context
     :param string workunit_name: An optional name for the work unit; defaults to the `cmd`
     :param list workunit_labels: An optional sequence of labels for the work unit.
-    :param **kwargs: Keyword arguments to pass through to `subprocess.Popen`.
+    :param kwargs: Keyword arguments to pass through to `subprocess.Popen`.
     :returns: A tuple of the exit code and the go command that was run.
     :rtype: (int, :class:`GoDistribution.GoCommand`)
     """
-    go_cmd = self.GoCommand._create(self.goroot, cmd, go_env=self.go_env(gopath=gopath), args=args)
+    go_cmd = self.GoCommand.create(self.goroot, cmd, go_env=self.go_env(gopath=gopath), args=args)
     if workunit_factory is None:
       return go_cmd.spawn(**kwargs).wait()
     else:

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

@@ -27,7 +27,7 @@ class GoTask(Task):
 
   @classmethod
   def subsystem_dependencies(cls):
-    return super(GoTask, cls).subsystem_dependencies() + (GoDistribution.Factory,)
+    return super(GoTask, cls).subsystem_dependencies() + (GoDistribution.scoped(cls),)
 
   @staticmethod
   def is_binary(target):
@@ -51,7 +51,7 @@ def is_go(target):
 
   @memoized_property
   def go_dist(self):
-    return GoDistribution.Factory.global_instance().create()
+    return GoDistribution.scoped_instance(self)
 
   @memoized_property
   def import_oracle(self):

contrib/go/tests/python/pants_test/contrib/go/subsystems/test_go_distribution.py

@@ -18,8 +18,7 @@
 class GoDistributionTest(unittest.TestCase):
 
   def distribution(self):
-    factory = global_subsystem_instance(GoDistribution.Factory)
-    return factory.create()
+    return global_subsystem_instance(GoDistribution)
 
   def test_bootstrap(self):
     go_distribution = self.distribution()

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

@@ -22,8 +22,7 @@ def test_go_compile_simple(self):
               'contrib/go/examples/src/go/libA']
       pants_run = self.run_pants_with_workdir(args, workdir)
       self.assert_success(pants_run)
-      factory = global_subsystem_instance(GoDistribution.Factory)
-      go_dist = factory.create()
+      go_dist = global_subsystem_instance(GoDistribution)
       goos = go_dist.create_go_cmd('env', args=['GOOS']).check_output().strip()
       goarch = go_dist.create_go_cmd('env', args=['GOARCH']).check_output().strip()
       expected_files = set('contrib.go.examples.src.go.{libname}.{libname}/'

src/python/pants/backend/python/subsystems/isort.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 Script
+
+
+class Isort(Script):
+  options_scope = 'isort'
+  default_version = '4.2.5'
+  suffix = '.pex'
+
+  replaces_scope = 'fmt.isort'
+  replaces_name = 'version'

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

@@ -16,7 +16,6 @@ python_library(
     'src/python/pants/base:fingerprint_strategy',
     'src/python/pants/base:hash_utils',
     'src/python/pants/base:specs',
-    'src/python/pants/binaries:thrift_util',
     'src/python/pants/build_graph',
     'src/python/pants/invalidation',
     'src/python/pants/python',

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

@@ -8,17 +8,18 @@
 import logging
 import os
 
+from pants.backend.python.subsystems.isort import Isort
 from pants.backend.python.targets.python_binary import PythonBinary
 from pants.backend.python.targets.python_library import PythonLibrary
 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.binaries.binary_util import BinaryUtil
 from pants.task.fmt_task_mixin import FmtTaskMixin
 from pants.task.task import Task
 from pants.util.process_handler import subprocess
 
 
+# TODO: Should be named IsortRun, for consistency.
 class IsortPythonTask(FmtTaskMixin, Task):
   """Autoformats Python source files with isort.
 
@@ -40,7 +41,7 @@ class IsortPythonTask(FmtTaskMixin, Task):
 
   @classmethod
   def subsystem_dependencies(cls):
-    return super(IsortPythonTask, cls).subsystem_dependencies() + (BinaryUtil.Factory.scoped(cls), )
+    return super(IsortPythonTask, cls).subsystem_dependencies() + (Isort, )
 
   def __init__(self, *args, **kwargs):
     super(IsortPythonTask, self).__init__(*args, **kwargs)
@@ -50,6 +51,7 @@ def __init__(self, *args, **kwargs):
   def register_options(cls, register):
     super(IsortPythonTask, cls).register_options(register)
     register('--version', advanced=True, fingerprint=True, default='4.2.5',
+             removal_version='1.7.0.dev0', removal_hint='Use --version in scope isort',
              help='Version of isort.')
 
   def execute(self, test_output_file=None):
@@ -60,8 +62,7 @@ def execute(self, test_output_file=None):
       logging.debug(self.NOOP_MSG_HAS_TARGET_BUT_NO_SOURCE)
       return
 
-    isort_script = BinaryUtil.Factory.create().select_script('scripts/isort',
-                                                             self.options.version, 'isort.pex')
+    isort_script = Isort.global_instance().select(context=self.context)
     cmd = [isort_script] + self.get_passthru_args() + sources
     logging.debug(' '.join(cmd))
 

src/python/pants/binaries/binary_tool.py

@@ -20,13 +20,17 @@ class BinaryToolBase(Subsystem):
   # Subclasses must set these to appropriate values for the tool they define.
   # They must also set options_scope appropriately.
   platform_dependent = None
-  archive_type = None
+  archive_type = None  # See pants.fs.archive.archive for valid string values.
   default_version = None
 
   # Subclasses may set this to the tool name as understood by BinaryUtil.
   # If unset, it defaults to the value of options_scope.
   name = None
 
+  # Subclasses may set this to a suffix (e.g., '.pex') to add to the computed remote path.
+  # Note that setting archive_type will add an appropriate archive suffix after this suffix.
+  suffix = ''
+
   # 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
@@ -103,7 +107,7 @@ def _select_for_version(self, version):
     return self._binary_util.select(
       supportdir=self.get_support_dir(),
       version=version,
-      name=self._get_name(),
+      name='{}{}'.format(self._get_name(), self.suffix),
       platform_dependent=self.platform_dependent,
       archive_type=self.archive_type)
 

src/python/pants/binaries/binary_util.py

@@ -17,11 +17,11 @@
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TaskError
 from pants.base.hash_utils import hash_file
-from pants.fs.archive import archiver
+from pants.fs.archive import archiver as create_archiver
 from pants.net.http.fetcher import Fetcher
 from pants.subsystem.subsystem import Subsystem
-from pants.util.contextutil import temporary_file
-from pants.util.dirutil import chmod_plus_x, safe_delete, safe_open
+from pants.util.contextutil import temporary_dir, temporary_file
+from pants.util.dirutil import chmod_plus_x, safe_mkdir_for, safe_open
 from pants.util.osutil import get_os_id
 
 
@@ -143,8 +143,8 @@ def select(self, supportdir, version, name, platform_dependent, archive_type):
     """Fetches a file, unpacking it if necessary."""
     if archive_type is None:
       return self._select_file(supportdir, version, name, platform_dependent)
-    selected_archiver = archiver(archive_type)
-    return self._select_archive(supportdir, version, name, platform_dependent, selected_archiver)
+    archiver = create_archiver(archive_type)
+    return self._select_archive(supportdir, version, name, platform_dependent, archiver)
 
   def _select_file(self, supportdir, version, name, platform_dependent):
     """Generates a path to request a file and fetches the file located at that path.
@@ -176,9 +176,10 @@ def _select_archive(self, supportdir, version, name, platform_dependent, archive
     """
     full_name = '{}.{}'.format(name, archiver.extension)
     downloaded_file = self._select_file(supportdir, version, full_name, platform_dependent)
-    # use filename without extension as the directory name
+    # use filename without rightmost extension as the directory name.
     unpacked_dirname, _ = os.path.splitext(downloaded_file)
-    archiver.extract(downloaded_file, unpacked_dirname)
+    if not os.path.exists(unpacked_dirname):
+      archiver.extract(downloaded_file, unpacked_dirname)
     return unpacked_dirname
 
   def _binary_path_to_fetch(self, supportdir, version, name, platform_dependent):
@@ -237,29 +238,29 @@ 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)
     if not os.path.exists(bootstrapped_binary_path):
-      downloadpath = bootstrapped_binary_path + '~'
-      try:
+      safe_mkdir_for(bootstrapped_binary_path)
+      with temporary_dir(root_dir=os.path.dirname(bootstrapped_binary_path)) as tmpdir:
+        downloadpath = os.path.join(tmpdir, 'download')
         with self._select_binary_stream(name, binary_path) as stream:
           with safe_open(downloadpath, 'wb') as bootstrapped_binary:
             bootstrapped_binary.write(stream())
           os.rename(downloadpath, bootstrapped_binary_path)
           chmod_plus_x(bootstrapped_binary_path)
-      finally:
-        safe_delete(downloadpath)
 
     logger.debug('Selected {binary} binary bootstrapped to: {path}'
                  .format(binary=name, path=bootstrapped_binary_path))
     return bootstrapped_binary_path
 
-  def _compare_file_checksums(self, filepath, checksum=None, digest=None):
+  @staticmethod
+  def _compare_file_checksums(filepath, checksum=None, digest=None):
     digest = digest or hashlib.sha1()
 
     if os.path.isfile(filepath) and checksum:
       return hash_file(filepath, digest=digest) == checksum
 
     return os.path.isfile(filepath)
 
-  def is_bin_valid(self, basepath, binary_file_specs=[]):
+  def is_bin_valid(self, basepath, binary_file_specs=tuple()):
     """Check if this bin path is valid.
 
     :param string basepath: The absolute path where the binaries are stored under.

src/python/pants/fs/archive.py

@@ -148,6 +148,8 @@ def create(self, basedir, outdir, name, prefix=None):
 TYPE_NAMES_PRESERVE_SYMLINKS = TYPE_NAMES - TYPE_NAMES_NO_PRESERVE_SYMLINKS
 
 
+# TODO: Rename to `create_archiver`. Pretty much every caller of this method is going
+# to want to put the return value into a variable named `archiver`.
 def archiver(typename):
   """Returns Archivers in common configurations.