Get rid of some direct uses of config in jvm_compile.

These are now replaced with registered options.

Got rid of the javac_args config key, as it was basically the same as
--args, except that pants added the -C escapes for you. This was confusing and
inconsistent anyway.

Created --source and --target options, and removed a random reference
to the old way of specifying those from test_rcfile.py, to avoid confusion
(even though the test still passed).

Testing Done:
All relevant unit and integration tests pass.

CI baking.

Reviewed at https://rbcommons.com/s/twitter/r/1357/

pants.ini

@@ -121,10 +121,8 @@ partition_size_hint: 1000000000
 
 jvm_args: ['-Xmx2G']
 
-
-# Extra args that will be -C mapped and passed through jmake to javac.
-# These can be over-ridden (overwrites) via --compile-java-args on the command line
-javac_args: ['-source', '6', '-target', '6']
+source: '6'
+target: '6'
 
 compiler-bootstrap-tools: ['//:java-compiler']
 

src/python/pants/backend/jvm/tasks/jvm_compile/BUILD

@@ -46,11 +46,11 @@ python_library(
     ':analysis_tools',
     ':jvm_compile',
     'src/python/pants/base:build_environment',
+    'src/python/pants/base:config',
     'src/python/pants/base:exceptions',
     'src/python/pants/base:target',
     'src/python/pants/base:workunit',
     'src/python/pants/util:dirutil',
-    'src/python/pants/util:strutil',
   ],
 )
 

src/python/pants/backend/jvm/tasks/jvm_compile/java/java_compile.py

@@ -6,6 +6,7 @@
                         print_function, unicode_literals)
 
 import os
+from pants.base.config import Config
 
 from pants.backend.jvm.tasks.jvm_compile.analysis_tools import AnalysisTools
 from pants.backend.jvm.tasks.jvm_compile.java.jmake_analysis import JMakeAnalysis
@@ -16,7 +17,6 @@
 from pants.base.target import Target
 from pants.base.workunit import WorkUnit
 from pants.util.dirutil import relativize_paths, safe_open
-from pants.util.strutil import safe_shlex_split
 
 
 # From http://kenai.com/projects/jmake/sources/mercurial/content
@@ -43,29 +43,6 @@
 # When executed via a subprocess return codes will be treated as unsigned
 _JMAKE_ERROR_CODES.update((256 + code, msg) for code, msg in _JMAKE_ERROR_CODES.items())
 
-# Overridden by parameter java-compile -> args
-_JAVA_COMPILE_ARGS_DEFAULT = [
-  '-C-encoding', '-CUTF-8',
-  '-C-g',
-  '-C-Tcolor',
-
-  # Don't warn for generated code.
-  '-C-Tnowarnprefixes', '-C%(pants_workdir)s/gen',
-
-  # Suppress the warning for annotations with no processor - we know there are many of these!
-  '-C-Tnowarnregex', '-C^(warning: )?No processor claimed any of these annotations: .*'
-]
-
-# Overridden by parameter java-compile -> warning_args
-_JAVA_COMPILE_WARNING_ARGS_DEFAULT = [
-  '-C-Xlint:all',   '-C-Xlint:-serial',
-  '-C-Xlint:-path', '-C-deprecation',
-]
-
-# Overridden by parameter java-compile ->no_warning_args
-_JAVA_COMPILE_NO_WARNING_ARGS_DEFAULT = [
-  '-C-Xlint:none', '-C-nowarn',
-]
 
 class JavaCompile(JvmCompile):
   _language = 'java'
@@ -77,10 +54,30 @@ class JavaCompile(JvmCompile):
 
   _JMAKE_MAIN = 'com.sun.tools.jmake.Main'
 
+  @classmethod
+  def get_args_default(cls):
+    return ('-C-encoding', '-CUTF-8', '-C-g', '-C-Tcolor',
+            # Don't warn for generated code. Note that we assume that we're using the default
+            # pants_workdir, which is always currently the case. In the future pants_workdir will
+            # be a regular option, and we will be able to get its value here, default or otherwise.
+            '-C-Tnowarnprefixes',
+            '-C{0}'.format(os.path.join(Config.DEFAULT_PANTS_WORKDIR.default, 'gen')),
+            # Suppress warning for annotations with no processor - we know there are many of these!
+            '-C-Tnowarnregex', '-C^(warning: )?No processor claimed any of these annotations: .*')
+
+  @classmethod
+  def get_warning_args_default(cls):
+    return ('-C-Xlint:all',   '-C-Xlint:-serial', '-C-Xlint:-path', '-C-deprecation')
+
+  @classmethod
+  def get_no_warning_args_default(cls):
+    return ('-C-Xlint:none', '-C-nowarn')
+
   @classmethod
   def register_options(cls, register):
     super(JavaCompile, cls).register_options(register)
-    register('--args', action='append', help='Pass these extra args to javac.')
+    register('--source', help='Provide source compatibility with this release.')
+    register('--target', help='Generate class files for this JVM version.')
 
   def __init__(self, *args, **kwargs):
     super(JavaCompile, self).__init__(*args, **kwargs)
@@ -102,18 +99,6 @@ def __init__(self, *args, **kwargs):
                                        ini_key='compiler-bootstrap-tools',
                                        default=['//:java-compiler'])
 
-    self.configure_args(args_defaults=_JAVA_COMPILE_ARGS_DEFAULT,
-                        warning_defaults=_JAVA_COMPILE_WARNING_ARGS_DEFAULT,
-                        no_warning_defaults=_JAVA_COMPILE_WARNING_ARGS_DEFAULT)
-
-    self._javac_opts = []
-    if self.get_options().args:
-      for arg in self.get_options().args:
-        self._javac_opts.extend(safe_shlex_split(arg))
-    else:
-      self._javac_opts.extend(self.context.config.getlist('java-compile',
-                                                          'javac_args', default=[]))
-
   @property
   def config_section(self):
     return self._config_section
@@ -133,20 +118,7 @@ def extra_products(self, target):
   # Make the java target language version part of the cache key hash,
   # this ensures we invalidate if someone builds against a different version.
   def platform_version_info(self):
-    ret = []
-    opts = self._javac_opts
-
-    try:
-      # We only care about the target version for now.
-      target_pos = opts.index('-target')
-      if len(opts) >= target_pos + 2:
-        for t in opts[target_pos:target_pos + 2]:
-          ret.append(t)
-    except ValueError:
-      # No target in javac opts.
-      pass
-
-    return ret
+    return (self.get_options().target,) if self.get_options().target else ()
 
   def compile(self, args, classpath, sources, classes_output_dir, analysis_file):
     relative_classpath = relativize_paths(classpath, self._buildroot)
@@ -158,15 +130,23 @@ def compile(self, args, classpath, sources, classes_output_dir, analysis_file):
       '-pdb-text-format',
       ]
 
-    compiler_classpath = self.tool_classpath(
-      self._compiler_bootstrap_key)
+    compiler_classpath = self.tool_classpath(self._compiler_bootstrap_key)
     args.extend([
       '-jcpath', ':'.join(compiler_classpath),
       '-jcmainclass', 'com.twitter.common.tools.Compiler',
       ])
-    args.extend(map(lambda arg: '-C%s' % arg, self._javac_opts))
 
+    if self.get_options().source:
+      args.extend(['-C-source', '-C{0}'.format(self.get_options().source)])
+    if self.get_options().target:
+      args.extend(['-C-target', '-C{0}'.format(self.get_options().target)])
+
+    if '-C-source' in self._args:
+      raise TaskError("Set the source Java version with the 'source' option, not in 'args'.")
+    if '-C-target' in self._args:
+      raise TaskError("Set the target JVM version with the 'target' option, not in 'args'.")
     args.extend(self._args)
+
     args.extend(sources)
     result = self.runjava(classpath=jmake_classpath,
                           main=JavaCompile._JMAKE_MAIN,

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py

@@ -46,9 +46,18 @@ def register_options(cls, register):
              help='Roughly how many source files to attempt to compile together. Set to a large '
                   'number to compile all sources together. Set to 0 to compile target-by-target.')
 
+    register('--args', action='append', default=list(cls.get_args_default()),
+             help='Args to pass to the compiler.')
+
     register('--warnings', default=True, action='store_true',
              help='Compile with all configured warnings enabled.')
 
+    register('--warning-args', action='append', default=list(cls.get_warning_args_default()),
+             help='Extra compiler args to use when warnings are enabled.')
+
+    register('--no-warning-args', action='append', default=list(cls.get_no_warning_args_default()),
+             help='Extra compiler args to use when warnings are disabled.')
+
     register('--missing-deps', choices=['off', 'warn', 'fatal'], default='warn',
              help='Check for missing dependencies in {0} code. Reports actual dependencies A -> B '
                   'where there is no transitive BUILD file dependency path from A to B. If fatal, '
@@ -86,6 +95,21 @@ def name(cls):
   def product_types(cls):
     return ['classes_by_target', 'classes_by_source', 'resources_by_target']
 
+  @classmethod
+  def get_args_default(cls):
+    """Override to set default for --args option."""
+    return ()
+
+  @classmethod
+  def get_warning_args_default(cls):
+    """Override to set default for --warning-args option."""
+    return ()
+
+  @classmethod
+  def get_no_warning_args_default(cls):
+    """Override to set default for --no-warning-args option."""
+    return ()
+
   def select(self, target):
     return target.has_sources(self._file_suffix)
 
@@ -139,9 +163,6 @@ def __init__(self, *args, **kwargs):
     super(JvmCompile, self).__init__(*args, **kwargs)
     config_section = self.config_section
 
-    # Global workdir.
-    self._pants_workdir = self.context.config.getdefault('pants_workdir')
-
     # Various working directories.
     self._classes_dir = os.path.join(self.workdir, 'classes')
     self._resources_dir = os.path.join(self.workdir, 'resources')
@@ -170,6 +191,12 @@ def __init__(self, *args, **kwargs):
     # The ivy confs for which we're building.
     self._confs = self.context.config.getlist(config_section, 'confs', default=['default'])
 
+    self._args = list(self.get_options().args)
+    if self.get_options().warnings:
+      self._args.extend(self.get_options().warning_args)
+    else:
+      self._args.extend(self.get_options().no_warning_args)
+
     # Set up dep checking if needed.
     def munge_flag(flag):
       flag_value = getattr(self.get_options(), flag, None)
@@ -208,23 +235,6 @@ def munge_flag(flag):
     # Populated in prepare_execute().
     self._sources_by_target = None
 
-  def configure_args(self, args_defaults=None, warning_defaults=None, no_warning_defaults=None):
-   """
-   Setup the compiler command line arguments, optionally providing default values.  It is mandatory
-   to call this from __init__() of your subclass.
-   :param list args_defaults:  compiler flags that should be invoked for all invocations
-   :param list warning_defaults: compiler flags to turn on warnings
-   :param list no_warning_defaults:  compiler flags to turn off all warnings
-   """
-   self._args = self.context.config.getlist(self._config_section, 'args',
-                                       default=args_defaults or [])
-   if self.get_options().warnings:
-     self._args.extend(self.context.config.getlist(self._config_section, 'warning_args',
-                                                   default=warning_defaults or []))
-   else:
-     self._args.extend(self.context.config.getlist(self._config_section, 'no_warning_args',
-                                                   default=no_warning_defaults or[]))
-
   def prepare(self, round_manager):
     # TODO(John Sirois): this is a fake requirement on 'ivy_jar_products' in order to force
     # resolve to run before this goal.  Require a new CompileClasspath product to be produced by

src/python/pants/backend/jvm/tasks/jvm_compile/scala/scala_compile.py

@@ -14,26 +14,23 @@
 from pants.backend.jvm.tasks.jvm_compile.scala.zinc_utils import ZincUtils
 
 
-# Overridden by parameter scala-compile -> args
-_SCALA_COMPILE_ARGS_DEFAULT = [
-  '-S-encoding', '-SUTF-8','-S-g:vars',
-]
-
-# Overridden by parameter scala-compile -> warning_args
-_SCALA_COMPILE_WARNING_ARGS_DEFAULT = [
-  '-S-deprecation', '-S-unchecked',
-]
-
-# Overridden by parameter scala-compile -> no_warning_args
-_SCALA_COMPILE_NO_WARNING_ARGS_DEFAULT = [
-  '-S-nowarn',
-]
-
 class ScalaCompile(JvmCompile):
   _language = 'scala'
   _file_suffix = '.scala'
   _config_section = 'scala-compile'
 
+  @classmethod
+  def get_args_default(cls):
+    return ('-S-encoding', '-SUTF-8','-S-g:vars')
+
+  @classmethod
+  def get_warning_args_default(cls):
+    return ('-S-deprecation', '-S-unchecked')
+
+  @classmethod
+  def get_no_warning_args_default(cls):
+    return ('-S-nowarn',)
+
   @classmethod
   def register_options(cls, register):
     super(ScalaCompile, cls).register_options(register)
@@ -52,10 +49,6 @@ def __init__(self, *args, **kwargs):
                                  color=color,
                                  log_level=self.get_options().level)
 
-    self.configure_args(args_defaults=_SCALA_COMPILE_ARGS_DEFAULT,
-                        warning_defaults=_SCALA_COMPILE_WARNING_ARGS_DEFAULT,
-                        no_warning_defaults=_SCALA_COMPILE_WARNING_ARGS_DEFAULT)
-
   @property
   def config_section(self):
     return self._config_section

src/python/pants/base/config.py

@@ -95,6 +95,8 @@ class Config(object):
     value of var_name.
   """
 
+  # TODO: Replace these with regularly registered new-style options.
+
   DEFAULT_SECTION = ConfigParser.DEFAULTSECT
 
   DEFAULT_PANTS_DISTDIR = ConfigOption.create(

src/python/pants/option/migrate_config.py

@@ -13,6 +13,9 @@
 
 migrations = {
   ('java-compile', 'partition_size_hint'): ('compile.java', 'partition_size_hint'),
+  ('java-compile', 'javac_args'): ('compile.java', 'args'),
+  ('java-compile', 'warning_args'): ('compile.java', 'warning_args'),
+  ('java-compile', 'no_warning_args'): ('compile.java', 'no_warning_args'),
 
   ('javadoc-gen', 'include_codegen'): ('gen.javadoc', 'include_codegen'),
   ('scaladoc-gen', 'include_codegen'): ('gen.scaladoc', 'include_codegen'),
@@ -42,6 +45,12 @@
   ('confluence-publish', 'url'): ('confluence', 'url'),
 }
 
+notes = {
+  ('java-compile', 'javac_args'): 'source and target args should be moved to separate source: and '
+                                  'target: options. Other args should be placed in args: and '
+                                  'prefixed with -C.',
+}
+
 
 def check_config_file(path):
   config = Config.from_cache(configpath=path)
@@ -56,6 +65,8 @@ def section(s):
             '{dst_section}.'.format(src_key=green(src_key), src_section=section(src_section),
                                     dst_key=green(dst_key), dst_section=section(dst_section)),
                                     file=sys.stderr)
+      if (src_section, src_key) in notes:
+        print('  Note: {0}'.format(notes[(src_section, src_key)]))
 
 
 if __name__ == '__main__':

tests/python/pants_test/base/test_rcfile.py

@@ -5,9 +5,7 @@
 from __future__ import (nested_scopes, generators, division, absolute_import, with_statement,
                         print_function, unicode_literals)
 
-from contextlib import contextmanager
 from textwrap import dedent
-import os
 import unittest2 as unittest
 
 from pants.base.rcfile import RcFile
@@ -19,11 +17,11 @@ def test_parse_rcfile(self):
     with temporary_file() as rc:
       rc.write(dedent("""
       [jvm]
-      options: --compile-java-args='-target 7 -source 7'
+      options: --compile-java-args='-verbose -deprecation'
       """))
       rc.close()
       rcfile = RcFile([rc.name], default_prepend=False)
       commands = ['jvm', 'fleem']
       args = ['javac', 'Foo.java']
       new_args = rcfile.apply_defaults(commands, args)
-      self.assertEquals(['javac', 'Foo.java', '--compile-java-args=-target 7 -source 7'], new_args)
+      self.assertEquals(['javac', 'Foo.java', '--compile-java-args=-verbose -deprecation'], new_args)

tests/python/pants_test/tasks/jvm_compile/java/test_java_compile_integration.py

@@ -78,8 +78,8 @@ def test_java_compile_produces_different_artifact_depending_on_java_version(self
 
       # Rerun for java 7
       pants_run = self.run_pants(
-        ['goal', 'compile', 'testprojects/src/java/com/pants/testproject/unicode/main',
-         '--compile-java-args=\'-target 1.7\''],
+        ['goal', 'compile.java', '--target=1.7',
+         'testprojects/src/java/com/pants/testproject/unicode/main'],
         config)
       self.assert_success(pants_run)