Skip to content

Commit

Permalink
Upgrade zinc's sbt dependency to 1.0.0: python portion
Browse files Browse the repository at this point in the history
This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064

This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340

It has everything from rb/4064:

* Deprecate the name-hashing flag: see buddy review.
* Update zinc parser for new analysis headers.
* Pass an explicit -cache-dir for zinc to compile the compiler-bridge into.
* Bump implementation version of zinc to account for the analysis format change.
* Don't iterate over source files while computing per-target deps.

Plus a few other changes:

* A target flag `zinc_file_manager` to turn off zinc provided file manager, implementation is similar to `fatal_warnings`
* Add a jmh test for the new `zinc_file_manager` target flag.
* Fixed `test_zinc_analysis`, regenerated test data.

Known issues:

* Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants
* Unreported dependencies from local anonymous classes sbt/zinc#192
* Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure.

Will follow up the above issues

Testing Done:
https://travis-ci.org/peiyuwang/pants/builds/174567118
https://travis-ci.org/pantsbuild/pants/builds/174984898
https://travis-ci.org/pantsbuild/pants/builds/176920777

Bugs closed: 3962, 4042

Reviewed at https://rbcommons.com/s/twitter/r/4342/
  • Loading branch information
peiyuwang committed Nov 18, 2016
1 parent 53daf15 commit 0b03c97
Show file tree
Hide file tree
Showing 18 changed files with 562 additions and 282 deletions.
6 changes: 6 additions & 0 deletions 3rdparty/jvm/org/openjdk/jmh/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
jar_library(name='jmh',
jars=[
jar(org='org.openjdk.jmh', name='jmh-core', rev='1.12'),
jar(org='org.openjdk.jmh', name='jmh-generator-annprocess', rev='1.12'),
],
)
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ def strict_deps(self):
def fatal_warnings(self):
return False

@property
def zinc_file_manager(self):
return False

@property
def platform(self):
return JvmPlatform.global_instance().get_platform_for_target(self)
11 changes: 11 additions & 0 deletions src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ def register_options(cls, register):
fingerprint=True,
help='The default for the "fatal_warnings" argument for targets of this language.')

register('--zinc-file-manager', advanced=True, default=True, type=bool,
fingerprint=True,
help='Use zinc provided file manager to ensure transactional rollback.')

@property
def strict_deps(self):
"""When True, limits compile time deps to those that are directly declared by a target.
Expand All @@ -35,3 +39,10 @@ def fatal_warnings(self):
:rtype: bool
"""
return self.get_options().fatal_warnings

@property
def zinc_file_manager(self):
"""If false, the default file manager will be used instead of the zinc provided one.
:rtype: bool
"""
return self.get_options().zinc_file_manager
14 changes: 14 additions & 0 deletions src/python/pants/backend/jvm/targets/jvm_target.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ def __init__(self,
platform=None,
strict_deps=None,
fatal_warnings=None,
zinc_file_manager=None,
**kwargs):
"""
:API: public
Expand Down Expand Up @@ -69,6 +70,9 @@ def __init__(self,
:param fatal_warnings: Whether to turn warnings into errors for this target. If present,
takes priority over the language's fatal-warnings option.
:type fatal_warnings: bool
:param zinc_file_manager: Whether to use zinc provided file manager that allows transactional
rollbacks, but in certain cases may conflict with user libraries.
:type zinc_file_manager: bool
"""
self.address = address # Set in case a TargetDefinitionException is thrown early
payload = payload or Payload()
Expand All @@ -81,6 +85,7 @@ def __init__(self,
'platform': PrimitiveField(platform),
'strict_deps': PrimitiveField(strict_deps),
'fatal_warnings': PrimitiveField(fatal_warnings),
'zinc_file_manager': PrimitiveField(zinc_file_manager),
})
self._resource_specs = self.assert_list(resources, key_arg='resources')

Expand Down Expand Up @@ -111,6 +116,15 @@ def fatal_warnings(self):
"""
return self.payload.fatal_warnings

@property
def zinc_file_manager(self):
"""If false, the default file manager will be used instead of the zinc provided one.
:return: See constructor.
:rtype: bool or None
"""
return self.payload.zinc_file_manager

@property
def platform(self):
"""Platform associated with this target.
Expand Down
11 changes: 8 additions & 3 deletions src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ def create_analysis_tools(self):
raise NotImplementedError()

def compile(self, args, classpath, sources, classes_output_dir, upstream_analysis, analysis_file,
log_file, settings, fatal_warnings, javac_plugins_to_exclude):
log_file, settings, fatal_warnings, zinc_file_manager, javac_plugins_to_exclude):
"""Invoke the compiler.
Must raise TaskError on compile failure.
Expand All @@ -267,6 +267,7 @@ def compile(self, args, classpath, sources, classes_output_dir, upstream_analysi
:param JvmPlatformSettings settings: platform settings determining the -source, -target, etc for
javac to use.
:param fatal_warnings: whether to convert compilation warnings to errors.
:param zinc_file_manager: whether to use zinc provided file manager.
:param javac_plugins_to_exclude: A list of names of javac plugins that mustn't be used in
this compilation, even if requested (typically because
this compilation is building those same plugins).
Expand Down Expand Up @@ -477,7 +478,8 @@ def _record_compile_classpath(self, classpath, targets, outdir):
f.write(text.encode('utf-8'))

def _compile_vts(self, vts, sources, analysis_file, upstream_analysis, classpath, outdir,
log_file, progress_message, settings, fatal_warnings, counter):
log_file, progress_message, settings, fatal_warnings, zinc_file_manager,
counter):
"""Compiles sources for the given vts into the given output dir.
vts - versioned target set
Expand Down Expand Up @@ -518,7 +520,8 @@ def _compile_vts(self, vts, sources, analysis_file, upstream_analysis, classpath
# If compiling a plugin, don't try to use it on itself.
javac_plugins_to_exclude = (t.plugin for t in vts.targets if isinstance(t, JavacPlugin))
self.compile(self._args, classpath, sources, outdir, upstream_analysis, analysis_file,
log_file, settings, fatal_warnings, javac_plugins_to_exclude)
log_file, settings, fatal_warnings, zinc_file_manager,
javac_plugins_to_exclude)

def check_artifact_cache(self, vts):
"""Localizes the fetched analysis for targets we found in the cache."""
Expand Down Expand Up @@ -717,6 +720,7 @@ def work_for_vts(vts, ctx):

tgt, = vts.targets
fatal_warnings = self._compute_language_property(tgt, lambda x: x.fatal_warnings)
zinc_file_manager = self._compute_language_property(tgt, lambda x: x.zinc_file_manager)
self._compile_vts(vts,
ctx.sources,
ctx.analysis_file,
Expand All @@ -727,6 +731,7 @@ def work_for_vts(vts, ctx):
progress_message,
tgt.platform,
fatal_warnings,
zinc_file_manager,
counter)
self._analysis_tools.relativize(ctx.analysis_file, ctx.portable_analysis_file)

Expand Down
91 changes: 51 additions & 40 deletions src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import re
import textwrap
from contextlib import closing
from hashlib import sha1
from xml.etree import ElementTree

from pants.backend.jvm.subsystems.java import Java
Expand Down Expand Up @@ -116,6 +117,10 @@ def _get_zinc_arguments(settings):
zinc_args.extend(settings_args)
return zinc_args

@classmethod
def implementation_version(cls):
return super(BaseZincCompile, cls).implementation_version() + [('BaseZincCompile', 3)]

@classmethod
def compiler_plugin_types(cls):
"""A tuple of target types which are compiler plugins."""
Expand Down Expand Up @@ -150,12 +155,6 @@ def get_fatal_warnings_disabled_args_default(cls):
@classmethod
def register_options(cls, register):
super(BaseZincCompile, cls).register_options(register)
# TODO: disable by default because it breaks dependency parsing:
# https://github.com/pantsbuild/pants/issues/2224
# ...also, as of sbt 0.13.9, it is significantly slower for cold builds.
register('--name-hashing', advanced=True, type=bool, fingerprint=True,
help='Use zinc name hashing.')

register('--whitelisted-args', advanced=True, type=dict,
default={
'-S.*': False,
Expand All @@ -173,54 +172,49 @@ def register_options(cls, register):
'changed targets will be compiled with an empty output directory, as if after '
'running clean-all.')

# TODO: Defaulting to false due to a few upstream issues for which we haven't pulled down fixes:
# https://github.com/sbt/sbt/pull/2085
# https://github.com/sbt/sbt/pull/2160
register('--incremental-caching', advanced=True, type=bool,
help='When set, the results of incremental compiles will be written to the cache. '
'This is unset by default, because it is generally a good precaution to cache '
'only clean/cold builds.')

def sbt_jar(name, **kwargs):
return JarDependency(org='org.scala-sbt', name=name, rev='1.0.0-X5', **kwargs)

shader_rules = [
# The compiler-interface and compiler-bridge tool jars carry xsbt and
# xsbti interfaces that are used across the shaded tool jar boundary so
# we preserve these root packages wholesale along with the core scala
# APIs.
Shader.exclude_package('scala', recursive=True),
Shader.exclude_package('xsbt', recursive=True),
Shader.exclude_package('xsbti', recursive=True),
]

cls.register_jvm_tool(register,
'zinc',
classpath=[
# NB: This is explicitly a `_2.10` JarDependency rather than a
# ScalaJarDependency. The latter would pick up the platform in a users'
# repo, whereas this binary is shaded and independent of the target
# platform version.
JarDependency('org.pantsbuild', 'zinc_2.10', '0.0.4')
JarDependency('org.pantsbuild', 'zinc_2.10', '0.0.5'),
],
main=cls._ZINC_MAIN,
custom_rules=[
# The compiler-interface and sbt-interface tool jars carry xsbt and
# xsbti interfaces that are used across the shaded tool jar boundary so
# we preserve these root packages wholesale along with the core scala
# APIs.
Shader.exclude_package('scala', recursive=True),
Shader.exclude_package('xsbt', recursive=True),
Shader.exclude_package('xsbti', recursive=True),
])

def sbt_jar(name, **kwargs):
return JarDependency(org='com.typesafe.sbt', name=name, rev='0.13.9', **kwargs)
custom_rules=shader_rules)

cls.register_jvm_tool(register,
'compiler-interface',
'compiler-bridge',
classpath=[
sbt_jar(name='compiler-interface',
sbt_jar(name='compiler-bridge_2.10',
classifier='sources',
# We just want the single compiler-interface jar and not its
# dep on scala-lang
intransitive=True)
])
cls.register_jvm_tool(register,
'sbt-interface',
'compiler-interface',
classpath=[
sbt_jar(name='sbt-interface',
# We just want the single sbt-interface jar and not its dep
# on scala-lang
intransitive=True)
])
sbt_jar(name='compiler-interface')
],
# NB: We force a noop-jarjar'ing of the interface, since it is now broken
# up into multiple jars, but zinc does not yet support a sequence of jars
# for the interface.
main='no.such.main.Main',
custom_rules=shader_rules)

@classmethod
def prepare(cls, options, round_manager):
Expand Down Expand Up @@ -306,8 +300,22 @@ def _write_processor_info(self, processor_info_file, processors):
for processor in processors:
f.write('{}\n'.format(processor.strip()))

@memoized_property
def _zinc_cache_dir(self):
"""A directory where zinc can store compiled copies of the `compiler-bridge`.
The compiler-bridge is specific to each scala version, and is lazily computed by zinc if the
appropriate version does not exist. Eventually it would be great to just fetch this rather
than compiling it.
"""
hasher = sha1()
for tool in ['zinc', 'compiler-interface', 'compiler-bridge']:
hasher.update(os.path.relpath(self.tool_jar(tool), self.get_options().pants_workdir))
key = hasher.hexdigest()[:12]
return os.path.join(self.get_options().pants_bootstrapdir, 'zinc', key)

def compile(self, args, classpath, sources, classes_output_dir, upstream_analysis, analysis_file,
log_file, settings, fatal_warnings, javac_plugins_to_exclude):
log_file, settings, fatal_warnings, zinc_file_manager, javac_plugins_to_exclude):
self._verify_zinc_classpath(classpath)
self._verify_zinc_classpath(upstream_analysis.keys())

Expand All @@ -321,13 +329,12 @@ def compile(self, args, classpath, sources, classes_output_dir, upstream_analysi
])
if not self.get_options().colors:
zinc_args.append('-no-color')
if not self.get_options().name_hashing:
zinc_args.append('-no-name-hashing')
if log_file:
zinc_args.extend(['-capture-log', log_file])

zinc_args.extend(['-compiler-interface', self.tool_jar('compiler-interface')])
zinc_args.extend(['-sbt-interface', self.tool_jar('sbt-interface')])
zinc_args.extend(['-compiler-bridge', self.tool_jar('compiler-bridge')])
zinc_args.extend(['-zinc-cache-dir', self._zinc_cache_dir])
zinc_args.extend(['-scala-path', ':'.join(self.scalac_classpath())])

zinc_args.extend(self.javac_plugin_args(javac_plugins_to_exclude))
Expand All @@ -338,12 +345,16 @@ def compile(self, args, classpath, sources, classes_output_dir, upstream_analysi

zinc_args.extend(args)
zinc_args.extend(self._get_zinc_arguments(settings))
zinc_args.append('-transactional')

if fatal_warnings:
zinc_args.extend(self.get_options().fatal_warnings_enabled_args)
else:
zinc_args.extend(self.get_options().fatal_warnings_disabled_args)

if not zinc_file_manager:
zinc_args.append('-no-zinc-file-manager')

jvm_options = []

if self.javac_classpath():
Expand Down
5 changes: 2 additions & 3 deletions src/python/pants/backend/jvm/tasks/jvm_dependency_usage.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ def cache_target_dirs(self):
return True

def create_dep_usage_node(self, target, analyzer, classes_by_source, targets_by_file, transitive_deps):
buildroot = analyzer.buildroot
product_deps_by_src = analyzer.product_deps_by_src
declared_deps_with_aliases = set(analyzer.resolve_aliases(target))
eligible_unused_deps = set(d for d, _ in analyzer.resolve_aliases(target, scope=Scopes.DEFAULT))
Expand All @@ -249,8 +248,8 @@ def _construct_edge(dep_tgt, products_used):
# Record the used products and undeclared Edges for this target. Note that some of
# these may be self edges, which are considered later.
target_product_deps_by_src = product_deps_by_src.get(target, {})
for src in target.sources_relative_to_buildroot():
for product_dep in target_product_deps_by_src.get(os.path.join(buildroot, src), []):
for product_deps in target_product_deps_by_src.values():
for product_dep in product_deps:
for dep_tgt in targets_by_file.get(product_dep, []):
derived_from = dep_tgt.concrete_derived_from
if not self._select(derived_from):
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/jvm/zinc/zinc_analysis.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class ZincAnalysis(object):
those yourself.
"""

FORMAT_VERSION_LINE = b'format version: 5\n'
FORMAT_VERSION_LINE = b'format version: 6\n'

def __init__(self, compile_setup, relations, stamps, apis, source_infos, compilations):
(self.compile_setup, self.relations, self.stamps, self.apis, self.source_infos, self.compilations) = \
Expand Down
Loading

0 comments on commit 0b03c97

Please sign in to comment.