Skip to content

Commit

Permalink
Enable passing option sets to the compiler (pantsbuild#6065)
Browse files Browse the repository at this point in the history
### Problem

The issue at pantsbuild#5360 

### Solution

I've added another option, `--compiler-option-sets`, which takes a list of option sets which then get translated to the actual flags that you want to pass to the compiler. These flags should be defined in `pants.ini` or through the command line.

### Result

Users can now enable compiler flags on a project or session basis instead of the whole repo.

PS. This is a work in progress, existing tests are failing and new ones have not been implemented. Making the PR now so that it can be reviewed as it progresses.
  • Loading branch information
alanbato authored and Stu Hood committed Jul 12, 2018
1 parent 450de70 commit 9f13b5e
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from pants.backend.jvm.subsystems.jvm_platform import JvmPlatform
from pants.base.payload import Payload
from pants.base.payload_field import PrimitiveField
from pants.base.payload_field import PrimitiveField, SetOfPrimitivesField

from pants.contrib.scalajs.subsystems.scala_js_platform import ScalaJSPlatform

Expand All @@ -23,6 +23,7 @@ def __init__(self, address=None, payload=None, **kwargs):
payload = payload or Payload()
payload.add_fields({
'platform': PrimitiveField(None),
'compiler_option_sets': SetOfPrimitivesField(None)
})
super(ScalaJSTarget, self).__init__(address=address, payload=payload, **kwargs)

Expand All @@ -41,6 +42,15 @@ def strict_deps(self):
def fatal_warnings(self):
return False

@property
def compiler_option_sets(self):
"""For every element in this list, enable the corresponding flags on compilation
of targets.
:return: See constructor.
:rtype: list
"""
return self.payload.compiler_option_sets

@property
def zinc_file_manager(self):
return False
Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/backend/jvm/subsystems/dependency_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ def defaulted_property(self, target, selector):
if target_property_selected is not None:
return target_property_selected

prop = False
prop = None
if target.has_sources('.java'):
prop |= selector(Java.global_instance())
prop = prop or selector(Java.global_instance())
if target.has_sources('.scala'):
prop |= selector(ScalaPlatform.global_instance())
prop = prop or selector(ScalaPlatform.global_instance())
return prop


Expand Down
20 changes: 20 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 @@ -4,6 +4,8 @@

from __future__ import absolute_import, division, print_function, unicode_literals

from pants.base.deprecated import deprecated


class ZincLanguageMixin(object):
"""A mixin for subsystems for languages compiled with Zinc."""
Expand All @@ -19,8 +21,14 @@ def register_options(cls, register):

register('--fatal-warnings', advanced=True, type=bool,
fingerprint=True,
removal_version='1.11.0.dev0',
removal_hint='Use --compiler-option-sets=fatal_warnings instead of fatal_warnings',
help='The default for the "fatal_warnings" argument for targets of this language.')

register('--compiler-option-sets', advanced=True, default=[], type=list,
fingerprint=True,
help='The option sets to be enabled for the compilation of this target.')

register('--zinc-file-manager', advanced=True, default=True, type=bool,
fingerprint=True,
help='Use zinc provided file manager to ensure transactional rollback.')
Expand All @@ -33,12 +41,24 @@ def strict_deps(self):
return self.get_options().strict_deps

@property
@deprecated('1.11.0.dev0', 'Consume fatal_warnings from compiler_option_sets instead.')
def fatal_warnings(self):
"""If true, make warnings fatal for targets that do not specify fatal_warnings.
:rtype: bool
"""
return self.get_options().fatal_warnings

@property
def compiler_option_sets(self):
"""For every element in this list, enable the corresponding flags on compilation
of targets.
:rtype: list
"""
option_sets = self.get_options().compiler_option_sets
if 'fatal_warnings' not in option_sets and self.fatal_warnings:
option_sets.append('fatal_warnings')
return option_sets

@property
def zinc_file_manager(self):
"""If false, the default file manager will be used instead of the zinc provided one.
Expand Down
35 changes: 32 additions & 3 deletions src/python/pants/backend/jvm/targets/jvm_target.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from pants.backend.jvm.subsystems.jvm_platform import JvmPlatform
from pants.backend.jvm.targets.jar_library import JarLibrary
from pants.backend.jvm.targets.jarable import Jarable
from pants.base.deprecated import deprecated_conditional
from pants.base.payload import Payload
from pants.base.payload_field import ExcludesField, PrimitiveField, PrimitivesSetField
from pants.build_graph.resources import Resources
Expand Down Expand Up @@ -39,6 +40,7 @@ def __init__(self,
strict_deps=None,
exports=None,
fatal_warnings=None,
compiler_option_sets=None,
zinc_file_manager=None,
# Some subclasses can have both .java and .scala sources
# (e.g., JUnitTests, JvmBinary, even ScalaLibrary), so it's convenient
Expand Down Expand Up @@ -82,7 +84,7 @@ def __init__(self,
if A exports B, and B exports C, then any targets that depends on A will
have access to both B and C.
:param bool fatal_warnings: Whether to turn warnings into errors for this target. If present,
takes priority over the language's fatal-warnings option.
takes priority over the language's fatal-warnings option. Deprecated.
:param bool zinc_file_manager: Whether to use zinc provided file manager that allows
transactional rollbacks, but in certain cases may conflict with
user libraries.
Expand All @@ -95,14 +97,29 @@ def __init__(self,
self.address = address # Set in case a TargetDefinitionException is thrown early
payload = payload or Payload()
excludes = ExcludesField(self.assert_list(excludes, expected_type=Exclude, key_arg='excludes'))
deprecated_conditional(
lambda: fatal_warnings is not None,
removal_version='1.11.0dev0',
entity_description='fatal_warnings',
hint_message="fatal_warnings should be defined as part of the target compiler_option_sets"
)
if fatal_warnings is not None:
compiler_option_sets = [] if compiler_option_sets is None else compiler_option_sets
if fatal_warnings:
compiler_option_sets.append('fatal_warnings')
else:
try:
compiler_option_sets.remove('fatal_warnings')
except ValueError:
pass
payload.add_fields({
'sources': self.create_sources_field(sources, address.spec_path, key_arg='sources'),
'provides': provides,
'excludes': excludes,
'platform': PrimitiveField(platform),
'strict_deps': PrimitiveField(strict_deps),
'exports': PrimitivesSetField(exports or []),
'fatal_warnings': PrimitiveField(fatal_warnings),
'compiler_option_sets': PrimitivesSetField(compiler_option_sets),
'zinc_file_manager': PrimitiveField(zinc_file_manager),
'javac_plugins': PrimitivesSetField(javac_plugins or []),
'javac_plugin_args': PrimitiveField(javac_plugin_args),
Expand Down Expand Up @@ -136,7 +153,19 @@ def fatal_warnings(self):
:return: See constructor.
:rtype: bool or None
"""
return self.payload.fatal_warnings
if self.payload.compiler_option_sets is not None:
return 'fatal_warnings' in self.payload.compiler_option_sets
else:
return False

@memoized_property
def compiler_option_sets(self):
"""For every element in this list, enable the corresponding flags on compilation
of targets.
:return: See constructor.
:rtype: list
"""
return self.payload.compiler_option_sets

@property
def zinc_file_manager(self):
Expand Down
20 changes: 14 additions & 6 deletions src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ def register_options(cls, register):
default=list(cls.get_fatal_warnings_disabled_args_default()),
help='Extra compiler args to use when fatal warnings are disabled.')

register('--compiler-option-sets-enabled-args', advanced=True, type=dict, fingerprint=True,
default={'fatal_warnings': list(cls.get_fatal_warnings_enabled_args_default())},
help='Extra compiler args to use for each enabled option set.')

register('--compiler-option-sets-disabled-args', advanced=True, type=dict, fingerprint=True,
default={'fatal_warnings': list(cls.get_fatal_warnings_disabled_args_default())},
help='Extra compiler args to use for each disabled option set.')

register('--debug-symbols', type=bool, fingerprint=True,
help='Compile with debug symbol enabled.')

Expand Down Expand Up @@ -226,7 +234,7 @@ def select_source(self, source_file_path):
raise NotImplementedError()

def compile(self, ctx, args, classpath, upstream_analysis,
settings, fatal_warnings, zinc_file_manager,
settings, compiler_option_sets, zinc_file_manager,
javac_plugin_map, scalac_plugin_map):
"""Invoke the compiler.
Expand Down Expand Up @@ -448,8 +456,8 @@ def _record_compile_classpath(self, classpath, targets, outdir):
with open(path, 'w') as f:
f.write(text.encode('utf-8'))

def _compile_vts(self, vts, ctx, upstream_analysis, classpath, progress_message, settings, fatal_warnings,
zinc_file_manager, counter):
def _compile_vts(self, vts, ctx, upstream_analysis, classpath, progress_message, settings,
compiler_option_sets, zinc_file_manager, counter):
"""Compiles sources for the given vts into the given output dir.
:param vts: VersionedTargetSet with one entry for the target.
Expand Down Expand Up @@ -488,7 +496,7 @@ def _compile_vts(self, vts, ctx, upstream_analysis, classpath, progress_message,
classpath,
upstream_analysis,
settings,
fatal_warnings,
compiler_option_sets,
zinc_file_manager,
self._get_plugin_map('javac', Java.global_instance(), ctx.target),
self._get_plugin_map('scalac', ScalaPlatform.global_instance(), ctx.target),
Expand Down Expand Up @@ -677,7 +685,7 @@ def work_for_vts(vts, ctx):

dep_context = DependencyContext.global_instance()
tgt, = vts.targets
fatal_warnings = dep_context.defaulted_property(tgt, lambda x: x.fatal_warnings)
compiler_option_sets = dep_context.defaulted_property(tgt, lambda x: x.compiler_option_sets)
zinc_file_manager = dep_context.defaulted_property(tgt, lambda x: x.zinc_file_manager)
with Timer() as timer:
self._compile_vts(vts,
Expand All @@ -686,7 +694,7 @@ def work_for_vts(vts, ctx):
cp_entries,
progress_message,
tgt.platform,
fatal_warnings,
compiler_option_sets,
zinc_file_manager,
counter)
self._record_target_stats(tgt,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def _zinc_cache_dir(self):
return os.path.join(self.get_options().pants_bootstrapdir, 'zinc', key)

def compile(self, ctx, args, classpath, upstream_analysis,
settings, fatal_warnings, zinc_file_manager,
settings, compiler_option_sets, zinc_file_manager,
javac_plugin_map, scalac_plugin_map):
self._verify_zinc_classpath(classpath)
self._verify_zinc_classpath(upstream_analysis.keys())
Expand Down Expand Up @@ -319,10 +319,17 @@ def compile(self, ctx, args, classpath, upstream_analysis,
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)
for option_set in compiler_option_sets:
enabled_args = self.get_options().compiler_option_sets_enabled_args.get(option_set, [])
if option_set == 'fatal_warnings':
enabled_args = self.get_options().fatal_warnings_enabled_args
zinc_args.extend(enabled_args)

for option_set, disabled_args in self.get_options().compiler_option_sets_disabled_args.items():
if option_set not in compiler_option_sets:
if option_set == 'fatal_warnings':
disabled_args = self.get_options().fatal_warnings_disabled_args
zinc_args.extend(disabled_args)

if not self._clear_invalid_analysis:
zinc_args.append('-no-clear-invalid-analysis')
Expand Down

0 comments on commit 9f13b5e

Please sign in to comment.