Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable passing option sets to the compiler #6065

Merged
merged 7 commits into from Jul 12, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -7,7 +7,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 @@ -24,6 +24,7 @@ def __init__(self, address=None, payload=None, **kwargs):
payload = payload or Payload()
payload.add_fields({
'platform': PrimitiveField(None),
'compiler_option_sets': SetOfPrimitivesField(None)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also use PrimitivesSetField.

})
super(ScalaJSTarget, self).__init__(address=address, payload=payload, **kwargs)

Expand All @@ -42,6 +43,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
Expand Up @@ -62,11 +62,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
Expand Up @@ -5,6 +5,8 @@
from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

from pants.base.deprecated import deprecated


class ZincLanguageMixin(object):
"""A mixin for subsystems for languages compiled with Zinc."""
Expand All @@ -20,8 +22,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.')
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, the --compiler-option-sets flag will end up defining the "default" options sets that are used for targets of a particular type (the ZincLanguageMixin is mixed in for "scala" and "java").

If a target passes the compiler_option_sets= argument, they'll be overriding the options sets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!
But 🤔
So we don't want them to pass down the compiler_option_sets= argument?
Either way, we can't hardcode the default option sets just yet, can we?

I'm not entirely sure where you were going with this comment 😅

PS. I'm using a list instead of a set because sets are not json serializable!

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't want them to pass down the compiler_option_sets= argument?

Yes, they are allowed to do that. But if they do not do that, we use the default from this option.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the same as with fatal_warnings... the option defined in ZincLanguageMixin is the default value of fatal_warnings for a target... if they don't set the option explicitly on their target, we use the default.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help for this option should likely be updated to align with the help for the deprecated fatal_warnings option: ie:

The default for the "compiler_option_sets" 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.')
Expand All @@ -34,12 +42,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
32 changes: 29 additions & 3 deletions src/python/pants/backend/jvm/targets/jvm_target.py
Expand Up @@ -11,6 +11,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 @@ -40,6 +41,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 @@ -83,7 +85,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 @@ -96,14 +98,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:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bunch of code is handling the case where fatal_warnings is set to false on the target, but it might be also either in the compiler_option_sets target params or CLI options.
This is the minimal way I could think of, thoughts?
I'd like to see this all removed once fatal_warnings is removed as an actual option/param :P

Copy link
Sponsor Member

@stuhood stuhood Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option might be to have the internal representation of compiler_options_sets temporarily be a dict from key to bool?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...oh. This is already nicer than that.

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 @@ -137,7 +154,16 @@ def fatal_warnings(self):
:return: See constructor.
:rtype: bool or None
"""
return self.payload.fatal_warnings
return 'fatal_warnings' in self.payload.compiler_option_sets

@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
Expand Up @@ -83,6 +83,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 @@ -227,7 +235,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 @@ -449,8 +457,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 @@ -489,7 +497,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 @@ -678,7 +686,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 @@ -687,7 +695,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
Expand Up @@ -274,7 +274,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 @@ -320,10 +320,13 @@ 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, [])
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:
zinc_args.extend(disabled_args)

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