Skip to content

Commit

Permalink
[zinc-compile] fully adopt enum based switches for hermetic/not; test…
Browse files Browse the repository at this point in the history 
… coverage

@cosmicexplorer wrote this as part of pantsbuild#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
  • Loading branch information
@baroquebobcat
baroquebobcat committed Feb 20, 2019
1 parent 0e6a144 commit bcc77d8
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 59 deletions.
137 changes: 78 additions & 59 deletions src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
31 changes: 31 additions & 0 deletions tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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)',
Expand Down

0 comments on commit bcc77d8

Please sign in to comment.