fix/refactor checkstyle

- Renaming BUILD.tools's twitter-checkstyle target to checkstyle since this is not
  twitter specific
- Adding build-support/checkstyle/* to wire up the checkstyle
- A few longer than 100-character per line java files fixes
- pants.ini, fix/change broken [checkstyle] to [compile.checkstyle]
- pants.ini remove suppression_files setting:
  There are a couple reasons for doing this:
  1) OS pants and com.puppycrawl.tools.checkstyle only supports single suppression file.
     The multiple suppression files concept is from a Twitter internal custom filter, which
     isn't applicable here.
  2) But more important, suppression isn't a required component for running checkstyle.
     Someone can perfectly author a coding_style.xml which contains no SuppressionFilter
     filter. And right now the checkstyle internally reads suppression_files list and injects
     them into the properties, which assuming the hardcoded SuppressionFilter. Doesn't make
     any sense. Anyone wants to use suppression, can still put checkstyle.suppression.file
     into properties setting under [compile.checkstyle] in OS pants, or any company specific
     pants.ini
- Adding the checkstyle task into the optional backend so when we run PANTS_DEV=1 ./pants
  in OS pants, it's added, however, it's not by default wired up in pants release. Anyone
  can still wire it up in their own wrapper (like what Twitter currently does)
- In ide_gen, currently it tries to read suppression_files from [checkstyle]. First, like
  I mentioned above, the default open source checkstyle doesn't have multiple suppression
  files concept. Second, the current setting points to an invalid file that doesn't exist
  anyway. So instead, I'm setting it to [] for now and will follow up with ide/idea owners
  later.
- Adding unit tests for checkstyle to prevent future regressions.

Testing Done:
PANTS_DEV=1 ./pants goal compile examples/src/java::
PANTS_DEV=1 ./pants goal test tests/python/pants_test/backend/jvm/tasks:checkstyle

Travis baking:
https://travis-ci.org/jinfeng/jinfeng-pants-fork/builds/43051952

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

BUILD.tools

@@ -67,11 +67,9 @@ jar_library(name = 'scala-library-2.9.3',
               jar(org = 'org.scala-lang', name = 'scala-library', rev = '2.9.3'),
             ])
 
-jar_library(name = 'twitter-checkstyle',
+jar_library(name = 'checkstyle',
             jars = [
               jar(org = 'com.puppycrawl.tools', name = 'checkstyle', rev = '5.6'),
-              jar(org = 'com.twitter.common', name = 'checkstyle', rev = '0.0.1')
-                .exclude(jar(org='com.google.guava', name='guava'))
             ])
 
 jar_library(name = 'junit',

build-support/checkstyle/coding_style.xml

@@ -0,0 +1,20 @@
+<?xml version="1.0"?>
+<!DOCTYPE module PUBLIC
+          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
+          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
+
+<module name="Checker">
+  <!-- See http://checkstyle.sourceforge.net/config_whitespace.html#FileTabCharacter -->
+  <module name="FileTabCharacter"/>
+
+  <module name="TreeWalker">
+    <property name="tabWidth" value="2"/>
+
+    <!-- See http://checkstyle.sourceforge.net/config_sizes.html#LineLength -->
+    <module name="LineLength">
+      <property name="max" value="100"/>
+      <property name="ignorePattern" value="^import"/>
+    </module>
+  </module>
+
+</module>

examples/src/java/com/pants/examples/annotation/processor/ExampleProcessor.java

@@ -61,7 +61,8 @@ public class ExampleProcessor extends AbstractProcessor {
         PrintWriter writer = closer.register(new PrintWriter(outputFile.openWriter()));
         writer.println("{");
         for (TypeElement appAnnotation : annotations) {
-          Set<? extends Element> annotatedElements = roundEnv.getElementsAnnotatedWith(appAnnotation);
+          Set<? extends Element> annotatedElements =
+              roundEnv.getElementsAnnotatedWith(appAnnotation);
           Set<TypeElement> exampleElements = ElementFilter.typesIn(annotatedElements);
           for (Element elem : exampleElements) {
             String typeName = elem.getSimpleName().toString();

pants.ini

@@ -100,10 +100,9 @@ supportdir: bin/ragel
 version: 6.8
 
 
-[checkstyle]
-bootstrap-tools: ["//:twitter-checkstyle"]
+[compile.checkstyle]
+bootstrap_tools: ["//:checkstyle"]
 configuration: %(pants_supportdir)s/checkstyle/coding_style.xml
-suppression_files = [ "%(pants_supportdir)s/commons/checkstyle/checkstyle_suppressions.xml" ]
 
 
 [scalastyle]

src/python/internal_backend/optional/BUILD

@@ -5,6 +5,7 @@ python_library(
   name = 'plugin',
   sources = ['__init__.py', 'register.py'],
   dependencies = [
+    'src/python/pants/backend/jvm/tasks:checkstyle',
     'src/python/pants/backend/jvm/tasks:scalastyle',
     'src/python/pants/goal:task_registrar',
   ]

src/python/internal_backend/optional/register.py

@@ -5,11 +5,16 @@
 from __future__ import (nested_scopes, generators, division, absolute_import, with_statement,
                         print_function, unicode_literals)
 
+from pants.backend.jvm.tasks.checkstyle import Checkstyle
 from pants.backend.jvm.tasks.scalastyle import Scalastyle
 from pants.goal.task_registrar import TaskRegistrar as task
 
 
 def register_goals():
+  task(name='checkstyle', action=Checkstyle,
+       dependencies=['gen', 'resolve']
+  ).install('compile')
+
   task(name='scalastyle', action=Scalastyle,
        dependencies=['bootstrap']
-  ).install('compile').with_description('Scala source code style check.')
+  ).install('compile')

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

@@ -10,48 +10,38 @@
 from pants.backend.jvm.tasks.jvm_tool_task_mixin import JvmToolTaskMixin
 from pants.backend.jvm.tasks.nailgun_task import NailgunTask
 from pants.base.exceptions import TaskError
+from pants.base.target import Target
+from pants.option.options import Options
 from pants.process.xargs import Xargs
 from pants.util.dirutil import safe_open
 
 
-CHECKSTYLE_MAIN = 'com.puppycrawl.tools.checkstyle.Main'
-
-
 class Checkstyle(NailgunTask, JvmToolTaskMixin):
 
+  _CHECKSTYLE_MAIN = 'com.puppycrawl.tools.checkstyle.Main'
+
   _CONFIG_SECTION = 'checkstyle'
 
-  @staticmethod
-  def _is_checked(target):
-    return target.is_java and not target.is_synthetic
+  _JAVA_SOURCE_EXTENSION = '.java'
+
+  _CHECKSTYLE_BOOTSTRAP_KEY = "checkstyle"
 
   @classmethod
   def register_options(cls, register):
     super(Checkstyle, cls).register_options(register)
     register('--skip', action='store_true', help='Skip checkstyle.')
     register('--configuration', help='Path to the checkstyle configuration file.')
-    register('--suppression_files', default=[],
-             help='List of checkstyle supression configuration files.')
-    register('--properties', default={},
+    register('--properties', type=Options.dict, default={},
              help='Dictionary of property mappings to use for checkstyle.properties.')
     register('--confs', default=['default'],
              help='One or more ivy configurations to resolve for this target. This parameter is '
                   'not intended for general use. ')
-    register('--bootstrap-tools', default=['//:twitter-checkstyle'],
+    register('--bootstrap-tools', type=Options.list, default=['//:checkstyle'],
              help='Pants targets used to bootstrap this tool.')
 
   def __init__(self, *args, **kwargs):
     super(Checkstyle, self).__init__(*args, **kwargs)
-
-    self._checkstyle_bootstrap_key = 'checkstyle'
-    self.register_jvm_tool(self._checkstyle_bootstrap_key, self.get_options().bootstrap_tools,
-                           ini_section=self.options_scope,
-                           ini_key='bootstrap-tools')
-
-    suppression_files = self.get_options().supression_files
-    self._properties = self.get_options().properties
-    self._properties['checkstyle.suppression.files'] = ','.join(suppression_files)
-    self._confs = self.context.config.getlist(self._CONFIG_SECTION, 'confs', )
+    self.register_jvm_tool(self._CHECKSTYLE_BOOTSTRAP_KEY, self.get_options().bootstrap_tools)
 
   @property
   def config_section(self):
@@ -65,6 +55,11 @@ def prepare(self, round_manager):
     round_manager.require_data('ivy_jar_products')
     round_manager.require_data('exclusives_groups')
 
+  def _is_checked(self, target):
+    return (isinstance(target, Target) and
+            target.has_sources(self._JAVA_SOURCE_EXTENSION) and
+            (not target.is_synthetic))
+
   def execute(self):
     if self.get_options().skip:
       return
@@ -77,19 +72,20 @@ def execute(self):
       if sources:
         result = self.checkstyle(sources, invalid_targets)
         if result != 0:
-          raise TaskError('java %s ... exited non-zero (%i)' % (CHECKSTYLE_MAIN, result))
+          raise TaskError('java {main} ... exited non-zero ({result})'.format(
+            main=self._CHECKSTYLE_MAIN, result=result))
 
   def calculate_sources(self, targets):
     sources = set()
     for target in targets:
       sources.update(source for source in target.sources_relative_to_buildroot()
-                     if source.endswith('.java'))
+                     if source.endswith(self._JAVA_SOURCE_EXTENSION))
     return sources
 
   def checkstyle(self, sources, targets):
     egroups = self.context.products.get_data('exclusives_groups')
     etag = egroups.get_group_key_for_target(targets[0])
-    classpath = self.tool_classpath(self._checkstyle_bootstrap_key)
+    classpath = self.tool_classpath(self._CHECKSTYLE_BOOTSTRAP_KEY)
     cp = egroups.get_classpath_for_group(etag)
     classpath.extend(jar for conf, jar in cp if conf in self.get_options().confs)
 
@@ -98,17 +94,17 @@ def checkstyle(self, sources, targets):
       '-f', 'plain'
     ]
 
-    if self._properties:
+    if self.get_options().properties:
       properties_file = os.path.join(self.workdir, 'checkstyle.properties')
       with safe_open(properties_file, 'w') as pf:
-        for k, v in self._properties.items():
-          pf.write('%s=%s\n' % (k, v))
+        for k, v in self.get_options().properties.items():
+          pf.write('{key}={value}\n'.format(key=k, value=v))
       args.extend(['-p', properties_file])
 
     # We've hit known cases of checkstyle command lines being too long for the system so we guard
     # with Xargs since checkstyle does not accept, for example, @argfile style arguments.
     def call(xargs):
-      return self.runjava(classpath=classpath, main=CHECKSTYLE_MAIN,
+      return self.runjava(classpath=classpath, main=self._CHECKSTYLE_MAIN,
                           args=args + xargs, workunit_name='checkstyle')
     checks = Xargs(call)
 

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

@@ -132,18 +132,16 @@ def __init__(self, *args, **kwargs):
 
     self.intransitive = self.get_options().intransitive
 
-    self.checkstyle_suppression_files = self.context.config.get('checkstyle',
-      'suppression_files', type=list, default=[])
     # Everywhere else, debug_port is specified in the 'jvm' section. Use that as a default if none
     # is specified in the 'ide' section.
     jvm_config_debug_port = JvmDebugConfig.debug_port(self.context.config)
     self.debug_port = self.context.config.getint('ide', 'debug_port', default=jvm_config_debug_port)
 
     self.checkstyle_bootstrap_key = 'checkstyle'
     self.register_jvm_tool_from_config(self.checkstyle_bootstrap_key, self.context.config,
-                                       ini_section='checkstyle',
-                                       ini_key='bootstrap-tools',
-                                       default=['//:twitter-checkstyle'])
+                                       ini_section='compile.checkstyle',
+                                       ini_key='bootstrap_tools',
+                                       default=['//:checkstyle'])
 
     self.scalac_bootstrap_key = None
     if not self.skip_scala:
@@ -166,12 +164,11 @@ def prepare(self, round_manager):
   def _prepare_project(self):
     targets, self._project = self.configure_project(
         self.context.targets(),
-        self.checkstyle_suppression_files,
         self.debug_port)
 
     self.configure_compile_context(targets)
 
-  def configure_project(self, targets, checkstyle_suppression_files, debug_port):
+  def configure_project(self, targets, debug_port):
     jvm_targets = [t for t in targets if t.has_label('jvm') or t.has_label('java')]
     if self.intransitive:
       jvm_targets = set(self.context.target_roots).intersection(jvm_targets)
@@ -181,7 +178,6 @@ def configure_project(self, targets, checkstyle_suppression_files, debug_port):
                       self.skip_scala,
                       self.use_source_root,
                       get_buildroot(),
-                      checkstyle_suppression_files,
                       debug_port,
                       jvm_targets,
                       not self.intransitive,
@@ -441,8 +437,7 @@ def _collapse_by_source_root(source_sets):
     return collapsed_source_sets
 
   def __init__(self, name, has_python, skip_java, skip_scala, use_source_root, root_dir,
-               checkstyle_suppression_files, debug_port, targets, transitive, workunit_factory,
-               target_util):
+               debug_port, targets, transitive, workunit_factory, target_util):
     """Creates a new, unconfigured, Project based at root_dir and comprised of the sources visible
     to the given targets."""
 
@@ -465,7 +460,6 @@ def __init__(self, name, has_python, skip_java, skip_scala, use_source_root, roo
     self.has_scala = False
     self.has_tests = False
 
-    self.checkstyle_suppression_files = checkstyle_suppression_files  # Absolute paths.
     self.debug_port = debug_port
 
     self.internal_jars = OrderedSet()

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

@@ -215,7 +215,6 @@ def create_content_root(source_set):
       ),
       resource_extensions=list(project.resource_extensions),
       scala=scala,
-      checkstyle_suppression_files=','.join(project.checkstyle_suppression_files),
       checkstyle_classpath=';'.join(project.checkstyle_classpath),
       debug_port=project.debug_port,
       extra_components=[],

src/python/pants/backend/jvm/tasks/templates/idea/project-12.mustache

@@ -90,7 +90,6 @@ Licensed under the Apache License, Version 2.0 (see LICENSE).
         <entry key="check-test-classes" value="test" />
         <entry key="location-0" value="CLASSPATH:/sun_checks.xml:The default CheckStyle rules." />
         <entry key="location-1" value="file://{{project.root_dir}}/build-support/checkstyle/coding_style.xml:Twitter CheckStyle rules." />
-        <entry key="property-1.checkstyle.suppression.files" value="{{project.checkstyle_suppression_files}}"/>
         <entry key="thirdparty-classpath" value="{{project.checkstyle_classpath}}" />
       </map>
     </option>

src/python/pants/option/migrate_config.py

@@ -23,8 +23,6 @@
   ('javadoc-gen', 'include_codegen'): ('gen.javadoc', 'include_codegen'),
   ('scaladoc-gen', 'include_codegen'): ('gen.scaladoc', 'include_codegen'),
 
-  ('DEFAULT', 'checkstyle_suppression_files'): ('checkstyle', 'suppression_files'),
-
   ('nailgun', 'autokill'): ('DEFAULT', 'kill_nailguns'),
 
   ('jvm-run', 'jvm_args'): ('run.jvm', 'jvm_options'),
@@ -41,6 +39,10 @@
 
   ('scala-repl', 'args'): ('repl.scala', 'args'),
 
+  ('checkstyle', 'bootstrap-tools'): ('compile.checkstyle', 'bootstrap_tools'),
+  ('checkstyle', 'configuration'): ('compile.checkstyle', 'configuration'),
+  ('checkstyle', 'properties'): ('compile.checkstyle', 'properties'),
+
   ('scala-compile', 'scalac-plugins'): ('compile.scala', 'plugins'),
   ('scala-compile', 'scalac-plugin-args'): ('compile.scala', 'plugin_args'),
 

testprojects/tests/java/com/pants/testproject/cwdexample/CwdTest.java

@@ -16,7 +16,8 @@
 public class CwdTest {
   @Test
   public void testChangedCwd() {
-    // We don't want this to fail for pants goal testprojects::, so we're going to conditionalize on a system property
+    // We don't want this to fail for pants goal testprojects::, so we're going to
+    // conditionalize on a system property
     String cwdTestEnabledFlag = System.getProperty("cwd.test.enabled");
     boolean cwdTestEnabled = cwdTestEnabledFlag == null ? false :
         cwdTestEnabledFlag.toLowerCase().equals("true");

tests/python/pants_test/backend/jvm/tasks/BUILD

@@ -4,6 +4,7 @@
 python_test_suite(
   name = 'tasks',
   dependencies = [
+    ':checkstyle',
     ':ide_gen',
     ':idea_gen',
     ':junit_run',
@@ -12,6 +13,17 @@ python_test_suite(
   ]
 )
 
+python_tests(
+  name = 'checkstyle',
+  sources = ['test_checkstyle.py'],
+  dependencies = [
+    'src/python/pants/backend/jvm/tasks:checkstyle',
+    'src/python/pants/base:address',
+    'src/python/pants/base:exceptions',
+    'tests/python/pants_test/jvm:nailgun_task_test_base',
+  ]
+)
+
 python_tests(
   name = 'ide_gen',
   sources = ['test_ide_gen.py'],