[zinc-compile] fully adopt enum based switches for hermetic/not; test…

… coverage (#7268)

@cosmicexplorer wrote this as part of #7227. This patch is pulling out just the Zinc changes, with a few differences. I also added a new test for hermetic failures and some additional assertions to ensure that the right message is being communicated on failures, while doing that I discovered that hermetic/non-hermetic appear to produce error messages on different streams.

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

@@ -386,71 +386,90 @@ def relative_to_exec_root(path):
     with open(ctx.zinc_args_file, 'w') as fp:
       for arg in zinc_args:
         # NB: in Python 2, options are stored sometimes as bytes and sometimes as unicode in the OptionValueContainer.
-        # This is due to how Python 2 natively stores attributes as a map of `str` (aka `bytes`) to their value. So, 
+        # This is due to how Python 2 natively stores attributes as a map of `str` (aka `bytes`) to their value. So,
         # the setattr() and getattr() functions sometimes use bytes.
         if PY2:
           arg = ensure_text(arg)
         fp.write(arg)
         fp.write('\n')
 
-    if self.execution_strategy == self.HERMETIC:
-      zinc_relpath = fast_relpath(self._zinc.zinc, get_buildroot())
-
-      snapshots = [
-        self._zinc.snapshot(self.context._scheduler),
-        ctx.target.sources_snapshot(self.context._scheduler),
-      ]
-
-      relevant_classpath_entries = dependency_classpath + [compiler_bridge_classpath_entry]
-      directory_digests = tuple(
-        entry.directory_digest for entry in relevant_classpath_entries if entry.directory_digest
-      )
-      if len(directory_digests) != len(relevant_classpath_entries):
-        for dep in relevant_classpath_entries:
-          if dep.directory_digest is None:
-            logger.warning(
-              "ClasspathEntry {} didn't have a Digest, so won't be present for hermetic "
-              "execution".format(dep)
-            )
-
-      snapshots.extend(
-        classpath_entry.directory_digest for classpath_entry in scalac_classpath_entries
-      )
-
-      merged_input_digest = self.context._scheduler.merge_directories(
-        tuple(s.directory_digest for s in (snapshots)) + directory_digests
-      )
-
-      # TODO: Extract something common from Executor._create_command to make the command line
-      # TODO: Lean on distribution for the bin/java appending here
-      argv = tuple(['.jdk/bin/java'] + jvm_options + ['-cp', zinc_relpath, Zinc.ZINC_COMPILE_MAIN] + zinc_args)
-      req = ExecuteProcessRequest(
-        argv=argv,
-        input_files=merged_input_digest,
-        output_directories=(classes_dir,),
-        description="zinc compile for {}".format(ctx.target.address.spec),
-        # TODO: These should always be unicodes
-        # Since this is always hermetic, we need to use `underlying_dist`
-        jdk_home=text_type(self._zinc.underlying_dist.home),
-      )
-      res = self.context.execute_process_synchronously_or_raise(req, self.name(), [WorkUnitLabel.COMPILER])
-
-      # TODO: Materialize as a batch in do_compile or somewhere
-      self.context._scheduler.materialize_directories((
-        DirectoryToMaterialize(get_buildroot(), res.output_directory_digest),
-      ))
-
-      # TODO: This should probably return a ClasspathEntry rather than a Digest
-      return res.output_directory_digest
-    else:
-      if self.runjava(classpath=self.get_zinc_compiler_classpath(),
-                      main=Zinc.ZINC_COMPILE_MAIN,
-                      jvm_options=jvm_options,
-                      args=zinc_args,
-                      workunit_name=self.name(),
-                      workunit_labels=[WorkUnitLabel.COMPILER],
-                      dist=self._zinc.dist):
-        raise TaskError('Zinc compile failed.')
+    return self.execution_strategy_enum.resolve_for_enum_variant({
+      self.HERMETIC: lambda: self._compile_hermetic(
+        jvm_options, ctx, classes_dir, zinc_args, compiler_bridge_classpath_entry,
+        dependency_classpath, scalac_classpath_entries),
+      self.SUBPROCESS: lambda: self._compile_nonhermetic(jvm_options, zinc_args),
+      self.NAILGUN: lambda: self._compile_nonhermetic(jvm_options, zinc_args),
+    })()
+
+  class ZincCompileError(TaskError):
+    """An exception type specifically to signal a failed zinc execution."""
+
+  def _compile_nonhermetic(self, jvm_options, zinc_args):
+    exit_code = self.runjava(classpath=self.get_zinc_compiler_classpath(),
+                             main=Zinc.ZINC_COMPILE_MAIN,
+                             jvm_options=jvm_options,
+                             args=zinc_args,
+                             workunit_name=self.name(),
+                             workunit_labels=[WorkUnitLabel.COMPILER],
+                             dist=self._zinc.dist)
+    if exit_code != 0:
+      raise self.ZincCompileError('Zinc compile failed.', exit_code=exit_code)
+
+  def _compile_hermetic(self, jvm_options, ctx, classes_dir, zinc_args,
+                        compiler_bridge_classpath_entry, dependency_classpath,
+                        scalac_classpath_entries):
+    zinc_relpath = fast_relpath(self._zinc.zinc, get_buildroot())
+
+    snapshots = [
+      self._zinc.snapshot(self.context._scheduler),
+      ctx.target.sources_snapshot(self.context._scheduler),
+    ]
+
+    relevant_classpath_entries = dependency_classpath + [compiler_bridge_classpath_entry]
+    directory_digests = tuple(
+      entry.directory_digest for entry in relevant_classpath_entries if entry.directory_digest
+    )
+    if len(directory_digests) != len(relevant_classpath_entries):
+      for dep in relevant_classpath_entries:
+        if dep.directory_digest is None:
+          logger.warning(
+            "ClasspathEntry {} didn't have a Digest, so won't be present for hermetic "
+            "execution".format(dep)
+          )
+
+    snapshots.extend(
+      classpath_entry.directory_digest for classpath_entry in scalac_classpath_entries
+    )
+
+    # TODO: Extract something common from Executor._create_command to make the command line
+    # TODO: Lean on distribution for the bin/java appending here
+    merged_input_digest = self.context._scheduler.merge_directories(
+      tuple(s.directory_digest for s in snapshots) + directory_digests
+    )
+    argv = ['.jdk/bin/java'] + jvm_options + [
+      '-cp', zinc_relpath,
+      Zinc.ZINC_COMPILE_MAIN
+    ] + zinc_args
+
+    req = ExecuteProcessRequest(
+      argv=tuple(argv),
+      input_files=merged_input_digest,
+      output_directories=(classes_dir,),
+      description="zinc compile for {}".format(ctx.target.address.spec),
+      # TODO: These should always be unicodes
+      # Since this is always hermetic, we need to use `underlying_dist`
+      jdk_home=text_type(self._zinc.underlying_dist.home),
+    )
+    res = self.context.execute_process_synchronously_or_raise(
+      req, self.name(), [WorkUnitLabel.COMPILER])
+
+    # TODO: Materialize as a batch in do_compile or somewhere
+    self.context._scheduler.materialize_directories((
+      DirectoryToMaterialize(get_buildroot(), res.output_directory_digest),
+    ))
+
+    # TODO: This should probably return a ClasspathEntry rather than a Digest
+    return res.output_directory_digest
 
   def get_zinc_compiler_classpath(self):
     """Get the classpath for the zinc compiler JVM tool.

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

@@ -235,6 +235,7 @@ def test_failed_hermetic_incremental_compile(self):
           config,
         )
         self.assert_failure(pants_run)
+        self.assertIn('Please use --no-compile-zinc-incremental', pants_run.stdout_data)
 
   def test_failed_compile_with_hermetic(self):
     with temporary_dir() as cache_dir:
@@ -258,6 +259,36 @@ def test_failed_compile_with_hermetic(self):
           config,
         )
         self.assert_failure(pants_run)
+        self.assertIn('package System2 does not exist', pants_run.stderr_data)
+        self.assertIn(
+          'Failed jobs: compile(testprojects/src/java/org/pantsbuild/testproject/dummies:'
+          'compilation_failure_target)',
+          pants_run.stdout_data)
+
+  def test_failed_compile_with_subprocess(self):
+    with temporary_dir() as cache_dir:
+      config = {
+        'cache.compile.zinc': {'write_to': [cache_dir]},
+        'compile.zinc': {
+          'execution_strategy': 'subprocess',
+          'use_classpath_jars': False,
+          'incremental': False,
+        }
+      }
+
+      with self.temporary_workdir() as workdir:
+        pants_run = self.run_pants_with_workdir(
+          [
+            # NB: We don't use -q here because subprocess squashes the error output
+            # See https://github.com/pantsbuild/pants/issues/5646
+            'compile',
+            'testprojects/src/java/org/pantsbuild/testproject/dummies:compilation_failure_target',
+          ],
+          workdir,
+          config,
+        )
+        self.assert_failure(pants_run)
+        self.assertIn('package System2 does not exist', pants_run.stdout_data)
         self.assertIn(
           'Failed jobs: compile(testprojects/src/java/org/pantsbuild/testproject/dummies:'
           'compilation_failure_target)',