whittle down the combined jvm tool support to a single method

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

@@ -13,8 +13,7 @@
 from pants.base.exceptions import TaskError
 from pants.java.distribution.distribution import DistributionLocator
 from pants.option.custom_types import target_option
-from pants.util.memo import memoized_property
-from pants.util.meta import classproperty
+from pants.util.memo import memoized_method
 from pants.util.objects import Exactly, datatype
 
 
@@ -158,7 +157,11 @@ class JvmToolDeclaration(datatype([
       ('classpath', tuple),
       ('custom_rules', tuple),
   ])):
-    """A mostly-typed specification for a JVM tool. Can be passed around across python modules."""
+    """A mostly-typed specification for a JVM tool.
+
+    This allows a specification for a JVM tool to be passed around verbatim across JvmToolMixin
+    subclasses.
+    """
 
     def __new__(cls, tool_name, main=None, classpath=None, custom_rules=None):
       return super(JvmToolMixin.JvmToolDeclaration, cls).__new__(
@@ -185,36 +188,25 @@ def register_jvm_tool_decl(cls, register, decl, **kwargs):
 
   class CombinedJvmToolsError(JvmToolDeclError): pass
 
-  @classproperty
-  def combined_jvm_tool_names(cls):
-    """Return a list of tool names which can be invoked as a single jar.
+  @memoized_method
+  def ensure_combined_jvm_tool_classpath(self, tool_name, combined_jvm_tool_names):
+    """Get a single classpath for all tools returned by `combined_jvm_tool_names`.
 
-    This allows tools to be invoked one at a time, but with the combined classpath of all of them
-    using ensure_combined_jvm_tool_classpath(). This allows creating nailgun instances for the
-    tools which have the same fingerprint, and allows a task to invoke multiple different JVM tools
-    from the same nailgun instances. See #7089.
-    """
-    raise NotImplementedError('combined_jvm_tool_names is not implemented, see '
-                              'https://github.com/pantsbuild/pants/pull/7092')
+    Also check to ensure `tool_name` is a member of `combined_jvm_tool_names`.
 
-  @memoized_property
-  def _combined_jvm_tool_classpath(self):
-    cp = []
-    for tool_name in self.combined_jvm_tool_names:
-      cp.extend(self.tool_classpath(tool_name))
-    return cp
-
-  def ensure_combined_jvm_tool_classpath(self, tool_name):
-    """Get a single classpath for all tools returned by `combined_jvm_tool_names()`.
-
-    Also check to ensure the tool was registered for consumption as a combined JVM tool.
+    This allows tools to be invoked one at a time, but with the combined classpath of all of
+    them. This allows creating nailgun instances for the tools which have the same fingerprint, and
+    allows a task to invoke multiple different JVM tools from the same nailgun instances. See #7089.
     """
-    if tool_name not in self.combined_jvm_tool_names:
+    if tool_name not in combined_jvm_tool_names:
       raise self.CombinedJvmToolsError(
-        "tool with name '{}' must be added to {}.combined_jvm_tool_names "
-        "(known keys are: {})"
-        .format(tool_name, type(self).__name__, self.combined_jvm_tool_names))
-    return self._combined_jvm_tool_classpath
+        "tool with name '{}' must be added to `combined_jvm_tool_names` "
+        "(which was: {})"
+        .format(tool_name, combined_jvm_tool_names))
+    cp = []
+    for component_tool_name in combined_jvm_tool_names:
+      cp.extend(self.tool_classpath(component_tool_name))
+    return cp
 
   @classmethod
   def prepare_tools(cls, round_manager):

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py

@@ -16,7 +16,7 @@
 from pants.backend.jvm.subsystems.jvm_platform import JvmPlatform
 from pants.backend.jvm.subsystems.jvm_tool_mixin import JvmToolMixin
 from pants.backend.jvm.subsystems.shader import Shader
-from pants.backend.jvm.subsystems.zinc import ZINC_COMPILER_DECL, Zinc
+from pants.backend.jvm.subsystems.zinc import ZINC_COMPILER_DECL
 from pants.backend.jvm.targets.jar_library import JarLibrary
 from pants.backend.jvm.targets.java_library import JavaLibrary
 from pants.backend.jvm.targets.jvm_target import JvmTarget
@@ -37,7 +37,6 @@
 from pants.util.contextutil import Timer
 from pants.util.dirutil import (fast_relpath, fast_relpath_optional, maybe_read_file,
                                 safe_file_dump, safe_mkdir)
-from pants.util.meta import classproperty
 
 
 #
@@ -191,18 +190,24 @@ def register_options(cls, register):
     for decl in [rsc_decl, metacp_decl, metai_decl, ZINC_COMPILER_DECL]:
       cls.register_jvm_tool_decl(register, decl)
 
-  @classproperty
-  def combined_jvm_tool_names(cls):
+  def get_nailgunnable_combined_classpath(self, tool_name):
     """Register all of the component tools of the rsc compile task as a "combined" jvm tool.
 
     This allows us to invoke their combined classpath in a single nailgun instance. We still invoke
     their classpaths separately when not using nailgun, however.
     """
-    return ['rsc', 'metai', 'metacp', 'zinc']
+    return self.ensure_combined_jvm_tool_classpath(
+      tool_name,
+      # TODO: allow @memoized_method to convert lists into tuples so they can be hashed!
+      ('rsc', 'metai', 'metacp', ZINC_COMPILER_DECL.tool_name))
 
   # Overrides the normal zinc compiler classpath, which only contains zinc.
   def get_zinc_compiler_classpath(self):
-    return self.ensure_combined_jvm_tool_classpath(Zinc.ZINC_COMPILER_TOOL_NAME)
+    return self.do_for_execution_strategy_variant({
+      self.HERMETIC: lambda: super(RscCompile, self).get_zinc_compiler_classpath(),
+      self.SUBPROCESS: lambda: super(RscCompile, self).get_zinc_compiler_classpath(),
+      self.NAILGUN: lambda: self.get_nailgunnable_combined_classpath(ZINC_COMPILER_DECL.tool_name),
+    })
 
   def register_extra_products_from_contexts(self, targets, compile_contexts):
     super(RscCompile, self).register_extra_products_from_contexts(targets, compile_contexts)
@@ -857,18 +862,17 @@ def _runtool_nonhermetic(self, parent_workunit, classpath, main, tool_name, args
 
   def _runtool(self, main, tool_name, args, distribution,
                tgt=None, input_files=tuple(), input_digest=None, output_dir=None):
-    def workunit_factory(*args, **kwargs):
-      return self.context.new_workunit(tool_name, *args, **kwargs)
-    return self.do_for_execution_strategy_variant(workunit_factory, {
-      self.HERMETIC: lambda _wu: self._runtool_hermetic(
-        main, tool_name, args, distribution,
-        tgt=tgt, input_files=input_files, input_digest=input_digest, output_dir=output_dir),
-      self.SUBPROCESS: lambda wu: self._runtool_nonhermetic(
-        wu, self.tool_classpath(tool_name), main, tool_name, args, distribution),
-      self.NAILGUN: lambda wu: self._runtool_nonhermetic(
-        wu, self.ensure_combined_jvm_tool_classpath(tool_name),
-        main, tool_name, args, distribution),
-    })
+    with self.context.new_workunit(tool_name) as wu:
+      return self.do_for_execution_strategy_variant({
+        self.HERMETIC: lambda: self._runtool_hermetic(
+          main, tool_name, args, distribution,
+          tgt=tgt, input_files=input_files, input_digest=input_digest, output_dir=output_dir),
+        self.SUBPROCESS: lambda: self._runtool_nonhermetic(
+          wu, self.tool_classpath(tool_name), main, tool_name, args, distribution),
+        self.NAILGUN: lambda: self._runtool_nonhermetic(
+          wu, self.get_nailgunnable_combined_classpath(tool_name),
+          main, tool_name, args, distribution),
+      })
 
   def _run_metai_tool(self,
                       distribution,

src/python/pants/backend/jvm/tasks/nailgun_task.py

@@ -28,19 +28,19 @@ class InvalidExecutionStrategyMapping(Exception): pass
 
   _all_execution_strategies = frozenset([NAILGUN, SUBPROCESS, HERMETIC])
 
-  def do_for_execution_strategy_variant(self, workunit_factory, mapping):
+  def do_for_execution_strategy_variant(self, mapping):
     """Invoke the method in `mapping` with the key corresponding to the execution strategy.
 
-    `mapping` is a dict mapping execution strategy -> method accepting a workunit argument.
+    `mapping` is a dict mapping execution strategy -> zero-argument lambda.
     """
     variants = frozenset(mapping.keys())
     if variants != self._all_execution_strategies:
       raise self.InvalidExecutionStrategyMapping(
-        'Must specify a mapping with exactly the keys {}'.format(self._all_execution_strategies))
+        'Must specify a mapping with exactly the keys {} (was: {})'
+        .format(self._all_execution_strategies, variants))
     method_for_variant = mapping[self.execution_strategy]
-    with workunit_factory() as workunit:
-      # The methods need not return a value, but we pass it along if they do.
-      return method_for_variant(workunit)
+    # The methods need not return a value, but we pass it along if they do.
+    return method_for_variant()
 
   @classmethod
   def register_options(cls, register):