From c06b63347949cba5a03f34e8dc9e6cd3b8d2c9c9 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 15 Apr 2019 13:23:11 -0700 Subject: [PATCH] Revert "[zinc-compile] fully adopt enum based switches for hermetic/not; test coverage (#7268)" This reverts commit cd4c77362cf4e89cc722cd345b934f0b5a629d9f. --- .../tasks/jvm_compile/zinc/zinc_compile.py | 137 ++++++++---------- .../java/test_zinc_compile_integration.py | 31 ---- 2 files changed, 59 insertions(+), 109 deletions(-) diff --git a/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py b/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py index a447cdb0af1..4cdef91fb23 100644 --- a/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py +++ b/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py @@ -386,90 +386,71 @@ 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') - 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 + 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.') def get_zinc_compiler_classpath(self): """Get the classpath for the zinc compiler JVM tool. diff --git a/tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py b/tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py index 8d10295183f..ce342b50d0a 100644 --- a/tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py +++ b/tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py @@ -235,7 +235,6 @@ 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: @@ -259,36 +258,6 @@ 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)',