Split out rsc distribution selection into JvmCompile (#7290)

### Problem

See #7272 (comment). We are selecting the jvm distribution in a pretty manual way in `rsc_compile.py` that might benefit from being extracted into the `JvmCompile` base class.

### Solution

- Move `_get_jvm_distribution()` from `RscCompile` into `JvmCompile`.
- Use that logic to simplify `ZincCompile#_get_zinc_arguments()`.

### Result

Distribution selection, and in particular preparing the distribution to be usable in a hermetic context, is extracted into a common method in the jvm compile base class.

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

@@ -9,7 +9,7 @@
 from builtins import object, open, str
 from multiprocessing import cpu_count
 
-from future.utils import string_types
+from future.utils import string_types, text_type
 from twitter.common.collections import OrderedSet
 
 from pants.backend.jvm.subsystems.dependency_context import DependencyContext
@@ -35,6 +35,7 @@
 from pants.base.worker_pool import WorkerPool
 from pants.base.workunit import WorkUnitLabel
 from pants.engine.fs import PathGlobs, PathGlobsAndRoot
+from pants.java.distribution.distribution import DistributionLocator
 from pants.option.compiler_option_sets_mixin import CompilerOptionSetsMixin
 from pants.reporting.reporting_utils import items_to_report_element
 from pants.util.contextutil import Timer
@@ -833,6 +834,62 @@ def _plugin_targets(self, compiler):
     plugin_tgts = self.context.targets(predicate=lambda t: isinstance(t, plugin_cls))
     return {t.plugin: t.closure() for t in plugin_tgts}
 
+  @staticmethod
+  def _local_jvm_distribution(settings=None):
+    settings_args = [settings] if settings else []
+    try:
+      local_distribution = JvmPlatform.preferred_jvm_distribution(settings_args, strict=True)
+    except DistributionLocator.Error:
+      local_distribution = JvmPlatform.preferred_jvm_distribution(settings_args, strict=False)
+    return local_distribution
+
+  class _HermeticDistribution(object):
+    def __init__(self, home_path, distribution):
+      self._underlying = distribution
+      self._home = home_path
+
+    def find_libs(self, names):
+      underlying_libs = self._underlying.find_libs(names)
+      return [self._rehome(l) for l in underlying_libs]
+
+    def find_libs_path_globs(self, names):
+      libs_abs = self._underlying.find_libs(names)
+      libs_unrooted = [self._unroot_lib_path(l) for l in libs_abs]
+      path_globs = PathGlobsAndRoot(
+        PathGlobs(tuple(libs_unrooted)),
+        text_type(self._underlying.home))
+      return (libs_unrooted, path_globs)
+
+    @property
+    def java(self):
+      return os.path.join(self._home, 'bin', 'java')
+
+    @property
+    def home(self):
+      return self._home
+
+    @property
+    def underlying_home(self):
+      return self._underlying.home
+
+    def _unroot_lib_path(self, path):
+      return path[len(self._underlying.home)+1:]
+
+    def _rehome(self, l):
+      return os.path.join(self._home, self._unroot_lib_path(l))
+
+  def _get_jvm_distribution(self):
+    # TODO We may want to use different jvm distributions depending on what
+    # java version the target expects to be compiled against.
+    # See: https://github.com/pantsbuild/pants/issues/6416 for covering using
+    #      different jdks in remote builds.
+    local_distribution = self._local_jvm_distribution()
+    return self.execution_strategy_enum.resolve_for_enum_variant({
+      self.SUBPROCESS: lambda: local_distribution,
+      self.NAILGUN: lambda: local_distribution,
+      self.HERMETIC: lambda: self._HermeticDistribution('.jdk', local_distribution),
+    })()
+
   def _default_work_for_vts(self, vts, ctx, input_classpath_product_key, counter, all_compile_contexts, output_classpath_product):
     progress_message = ctx.target.address.spec
 

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

@@ -13,7 +13,6 @@
 from twitter.common.collections import OrderedSet
 
 from pants.backend.jvm.subsystems.dependency_context import DependencyContext  # noqa
-from pants.backend.jvm.subsystems.jvm_platform import JvmPlatform
 from pants.backend.jvm.subsystems.shader import Shader
 from pants.backend.jvm.targets.junit_tests import JUnitTests
 from pants.backend.jvm.targets.jvm_target import JvmTarget
@@ -327,19 +326,19 @@ def work_for_vts_rsc(vts, ctx):
 
           sources_snapshot = ctx.target.sources_snapshot(scheduler=self.context._scheduler)
 
+          distribution = self._get_jvm_distribution()
+
           def hermetic_digest_classpath():
-            hermetic_dist = self._hermetic_jvm_distribution()
-            jdk_libs_rel, jdk_libs_digest = self._jdk_libs_paths_and_digest(hermetic_dist)
+            jdk_libs_rel, jdk_libs_digest = self._jdk_libs_paths_and_digest(distribution)
             merged_sources_and_jdk_digest = self.context._scheduler.merge_directories(
               (jdk_libs_digest, sources_snapshot.directory_digest))
             classpath_rel_jdk = rsc_classpath_rel + jdk_libs_rel
-            return (merged_sources_and_jdk_digest, classpath_rel_jdk, hermetic_dist)
+            return (merged_sources_and_jdk_digest, classpath_rel_jdk)
           def nonhermetic_digest_classpath():
-            nonhermetic_dist = self._nonhermetic_jvm_distribution()
-            classpath_abs_jdk = rsc_classpath_rel + self._jdk_libs_abs(nonhermetic_dist)
-            return ((EMPTY_DIRECTORY_DIGEST), classpath_abs_jdk, nonhermetic_dist)
+            classpath_abs_jdk = rsc_classpath_rel + self._jdk_libs_abs(distribution)
+            return ((EMPTY_DIRECTORY_DIGEST), classpath_abs_jdk)
 
-          (input_digest, classpath_entry_paths, distribution) = self.execution_strategy_enum.resolve_for_enum_variant({
+          (input_digest, classpath_entry_paths) = self.execution_strategy_enum.resolve_for_enum_variant({
             self.HERMETIC: hermetic_digest_classpath,
             self.SUBPROCESS: nonhermetic_digest_classpath,
             self.NAILGUN: nonhermetic_digest_classpath,
@@ -608,54 +607,6 @@ def _jdk_libs_paths_and_digest(self, hermetic_dist):
   def _jdk_libs_abs(self, nonhermetic_dist):
     return nonhermetic_dist.find_libs(self._JDK_LIB_NAMES)
 
-  class _HermeticDistribution(object):
-    def __init__(self, home_path, distribution):
-      self._underlying = distribution
-      self._home = home_path
-
-    def find_libs(self, names):
-      underlying_libs = self._underlying.find_libs(names)
-      return [self._rehome(l) for l in underlying_libs]
-
-    def find_libs_path_globs(self, names):
-      libs_abs = self._underlying.find_libs(names)
-      libs_unrooted = [self._unroot_lib_path(l) for l in libs_abs]
-      path_globs = PathGlobsAndRoot(
-        PathGlobs(tuple(libs_unrooted)),
-        text_type(self._underlying.home))
-      return (libs_unrooted, path_globs)
-
-    @property
-    def java(self):
-      return os.path.join(self._home, 'bin', 'java')
-
-    @property
-    def home(self):
-      return self._home
-
-    @property
-    def underlying_home(self):
-      return self._underlying.home
-
-    def _unroot_lib_path(self, path):
-      return path[len(self._underlying.home)+1:]
-
-    def _rehome(self, l):
-      return os.path.join(self._home, self._unroot_lib_path(l))
-
-  @memoized_method
-  def _hermetic_jvm_distribution(self):
-    # TODO We may want to use different jvm distributions depending on what
-    # java version the target expects to be compiled against.
-    # See: https://github.com/pantsbuild/pants/issues/6416 for covering using
-    #      different jdks in remote builds.
-    local_distribution = JvmPlatform.preferred_jvm_distribution([], strict=True)
-    return self._HermeticDistribution('.jdk', local_distribution)
-
-  @memoized_method
-  def _nonhermetic_jvm_distribution(self):
-    return JvmPlatform.preferred_jvm_distribution([], strict=True)
-
   def _on_invalid_compile_dependency(self, dep, compile_target):
     """Decide whether to continue searching for invalid targets to use in the execution graph.
 

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py

@@ -32,7 +32,6 @@
 from pants.base.workunit import WorkUnitLabel
 from pants.engine.fs import DirectoryToMaterialize
 from pants.engine.isolated_process import ExecuteProcessRequest
-from pants.java.distribution.distribution import DistributionLocator
 from pants.util.contextutil import open_zip
 from pants.util.dirutil import fast_relpath, safe_open
 from pants.util.memo import memoized_method, memoized_property
@@ -93,8 +92,12 @@ def validate(idx):
     while arg_index < len(args):
       arg_index += validate(arg_index)
 
+  def _get_zinc_arguments(self, settings):
+    distribution = self._get_jvm_distribution()
+    return self._format_zinc_arguments(settings, distribution)
+
   @staticmethod
-  def _get_zinc_arguments(settings):
+  def _format_zinc_arguments(settings, distribution):
     """Extracts and formats the zinc arguments given in the jvm platform settings.
 
     This is responsible for the symbol substitution which replaces $JAVA_HOME with the path to an
@@ -110,10 +113,6 @@ def _get_zinc_arguments(settings):
     if settings.args:
       settings_args = settings.args
       if any('$JAVA_HOME' in a for a in settings.args):
-        try:
-          distribution = JvmPlatform.preferred_jvm_distribution([settings], strict=True)
-        except DistributionLocator.Error:
-          distribution = JvmPlatform.preferred_jvm_distribution([settings], strict=False)
         logger.debug('Substituting "$JAVA_HOME" with "{}" in jvm-platform args.'
                      .format(distribution.home))
         settings_args = (a.replace('$JAVA_HOME', distribution.home) for a in settings.args)

tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_java_compile_settings_partitioning.py

@@ -11,6 +11,7 @@
 
 from pants.backend.jvm.subsystems.jvm_platform import JvmPlatformSettings
 from pants.backend.jvm.targets.java_library import JavaLibrary
+from pants.backend.jvm.tasks.jvm_compile.jvm_compile import JvmCompile
 from pants.backend.jvm.tasks.jvm_compile.zinc.zinc_compile import ZincCompile
 from pants.base.revision import Revision
 from pants.java.distribution.distribution import DistributionLocator
@@ -166,9 +167,13 @@ def test_compile_setting_inequivalence(self):
     self.assertNotEqual(JvmPlatformSettings('1.4', '1.6', ['-Xfoo:bar']),
                         JvmPlatformSettings('1.6', '1.6', ['-Xfoo:bar']))
 
+  def _get_zinc_arguments(self, settings):
+    distribution = JvmCompile._local_jvm_distribution(settings=settings)
+    return ZincCompile._format_zinc_arguments(settings, distribution)
+
   def test_java_home_extraction(self):
     init_subsystem(DistributionLocator)
-    _, source, _, target, foo, bar, composite, single = tuple(ZincCompile._get_zinc_arguments(
+    _, source, _, target, foo, bar, composite, single = tuple(self._get_zinc_arguments(
       JvmPlatformSettings('1.7', '1.7', [
         'foo', 'bar', 'foo:$JAVA_HOME/bar:$JAVA_HOME/foobar', '$JAVA_HOME',
       ])
@@ -183,7 +188,8 @@ def test_java_home_extraction(self):
     self.assertEqual('foo:{0}/bar:{0}/foobar'.format(single), composite)
 
   def test_java_home_extraction_empty(self):
-    result = tuple(ZincCompile._get_zinc_arguments(
+    init_subsystem(DistributionLocator)
+    result = tuple(self._get_zinc_arguments(
       JvmPlatformSettings('1.7', '1.7', [])
     ))
     self.assertEqual(4, len(result),
@@ -233,15 +239,15 @@ def fake_distribution_locator(*versions):
     # Completely missing a usable distribution.
     with fake_distribution_locator(far_future_version):
       with self.assertRaises(DistributionLocator.Error):
-        ZincCompile._get_zinc_arguments(JvmPlatformSettings(
+        self._get_zinc_arguments(JvmPlatformSettings(
           source_level=farer_future_version,
           target_level=farer_future_version,
           args=['$JAVA_HOME/foo'],
         ))
 
     # Missing a strict distribution.
     with fake_distribution_locator(farer_future_version) as paths:
-      results = ZincCompile._get_zinc_arguments(JvmPlatformSettings(
+      results = self._get_zinc_arguments(JvmPlatformSettings(
         source_level=far_future_version,
         target_level=far_future_version,
         args=['$JAVA_HOME/foo', '$JAVA_HOME'],
@@ -252,7 +258,7 @@ def fake_distribution_locator(*versions):
     # Make sure we pick up the strictest possible distribution.
     with fake_distribution_locator(farer_future_version, far_future_version) as paths:
       farer_path, far_path = paths
-      results = ZincCompile._get_zinc_arguments(JvmPlatformSettings(
+      results = self._get_zinc_arguments(JvmPlatformSettings(
         source_level=far_future_version,
         target_level=far_future_version,
         args=['$JAVA_HOME/foo', '$JAVA_HOME'],
@@ -263,7 +269,7 @@ def fake_distribution_locator(*versions):
     # Make sure we pick the higher distribution when the lower one doesn't work.
     with fake_distribution_locator(farer_future_version, far_future_version) as paths:
       farer_path, far_path = paths
-      results = ZincCompile._get_zinc_arguments(JvmPlatformSettings(
+      results = self._get_zinc_arguments(JvmPlatformSettings(
         source_level=farer_future_version,
         target_level=farer_future_version,
         args=['$JAVA_HOME/foo', '$JAVA_HOME'],