Fix scalastyle task, wire it up, make configs optional + refactoring …

…and unit tests

- fix the task, which has slipped out of maintenance.
- wire it up in OS pants.
- but make it flexible so that pants_internal or other private wrapper of OS pants won't fail if they don't have scalastyle config and excludes present (making config/exclude optional)
- some refactoring so I can write unit tests easily.
- unit tests and end to end tests

Testing Done:
- CI passed: https://travis-ci.org/jinfeng/jinfeng-pants-fork/builds/37824308

- - PANTS_DEV=1 ./pants goal test tests::

- positive manual test:

$ PANTS_DEV=1 ./pants goal compile examples/src/scala/com/pants/example/hello/welcome
...
...
...
16:00:13 00:02   [compile]
16:00:13 00:02     [jvm]
...
...
16:00:22 00:11     [scalastyle]
16:00:22 00:11       [bootstrap-scalastyle]
16:00:23 00:12       [org.scalastyle.Main]
               Waiting for background workers to finish.
               SUCCESS

- negative manual test (with the examples/src/scala/com/pants/example/Welcome.scala to break the ImportGroupingChecker rule:

$ PANTS_DEV=1 ./pants goal compile examples/src/scala/com/pants/example/hello/welcome
...
...
...
16:03:42 00:00   [compile]
16:03:42 00:00     [jvm]
...
...
16:03:44 00:02     [scalastyle]
16:03:44 00:02       [org.scalastyle.Main]
                     ==== stderr ====

                     ==== stdout ====
                     error file=/Users/jinfeng/workspace/github-pants-jinfeng/examples/src/scala/com/pants/example/hello/welcome/Welcome.scala message=Imports should be grouped together  line=13 column=0
                     Processed 1 file(s)
                     Found 1 errors
                     Found 0 warnings
                     Finished in 56 ms

FAILURE: java org.scalastyle.Main ... exited non-zero (1)

               Waiting for background workers to finish.
               FAILURE

Bugs closed: 647

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

build-support/scalastyle/scalastyle_config.xml

@@ -0,0 +1,4 @@
+<scalastyle commentFilter="enabled">
+ <name>Default Scalastyle configuration</name>
+ <check level="error" class="org.scalastyle.scalariform.ImportGroupingChecker" enabled="true"></check>
+</scalastyle>

pants.ini

@@ -113,9 +113,6 @@ properties: {
 
 [scalastyle]
 config: %(buildroot)s/build-support/scalastyle/scalastyle_config.xml
-# TODO(John Sirois): The excludes are currently global for all scala files but
-# they should be trimmed back or eliminated when scalastyle is restricted to
-# non code-gen targets:  https://jira.twitter.biz/browse/AWESOME-6870
 excludes: %(buildroot)s/build-support/scalastyle/excludes.txt
 
 
@@ -208,6 +205,7 @@ requirements: ['ipython==1.0.0']
 
 [backends]
 packages: [
-    'internal_backend.sitegen',
+    'internal_backend.optional',
     'internal_backend.repositories',
+    'internal_backend.sitegen',
   ]

src/python/internal_backend/BUILD

@@ -4,7 +4,8 @@
 target(
   name='plugins',
   dependencies = [
-    'src/python/internal_backend/sitegen:plugin',
+    'src/python/internal_backend/optional:plugin',
     'src/python/internal_backend/repositories:plugin',
+    'src/python/internal_backend/sitegen:plugin',
   ]
 )

src/python/internal_backend/optional/BUILD

@@ -0,0 +1,12 @@
+# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+python_library(
+  name = 'plugin',
+  sources = ['__init__.py', 'register.py'],
+  dependencies = [
+    'src/python/pants/backend/jvm/tasks:scalastyle',
+    'src/python/pants/goal:task_registrar',
+  ]
+)
+

src/python/internal_backend/optional/register.py

@@ -0,0 +1,15 @@
+# coding=utf-8
+# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from __future__ import (nested_scopes, generators, division, absolute_import, with_statement,
+                        print_function, unicode_literals)
+
+from pants.backend.jvm.tasks.scalastyle import Scalastyle
+from pants.goal.task_registrar import TaskRegistrar as task
+
+
+def register_goals():
+  task(name='scalastyle', action=Scalastyle,
+       dependencies=['bootstrap']
+  ).install('compile').with_description('Scala source code style check.')

src/python/pants/backend/jvm/register.py

@@ -5,8 +5,6 @@
 from __future__ import (nested_scopes, generators, division, absolute_import, with_statement,
                         print_function, unicode_literals)
 
-import os
-
 from pants.backend.core.tasks.group_task import GroupTask
 from pants.backend.jvm.artifact import Artifact
 from pants.backend.jvm.repository import Repository

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

@@ -19,19 +19,32 @@
 class Scalastyle(NailgunTask, JvmToolTaskMixin):
   """Checks scala source files to ensure they're stylish.
 
+  Scalastyle only checks against scala sources in non-synthetic
+  targets.
+
   Scalastyle is configured via the 'scalastyle' pants.ini section.
 
-  * ``config`` - Required path of the scalastyle configuration file.
+  * ``config`` - Required path of the scalastyle configuration
+    file. If the file doesn't exist, the task will throw.
   * ``excludes`` - Optional path of an excludes file that contains
     lines of regular expressions used to exclude matching files
     from style checks. File names matched against these regular
     expressions are relative to the repository root
-    (e.g.: com/twitter/mybird/MyBird.scala).
+    (e.g.: com/twitter/mybird/MyBird.scala). If not specified,
+    all scala sources in the targets will be checked. If the file
+    doesn't exist, the task will throw.
   """
 
   _CONFIG_SECTION = 'scalastyle'
+  _CONFIG_SECTION_CONFIG_OPTION = 'config'
+  _CONFIG_SECTION_EXCLUDES_OPTION = 'excludes'
+  _SCALA_SOURCE_EXTENSION = '.scala'
+
   _MAIN = 'org.scalastyle.Main'
 
+  _scalastyle_config = None
+  _scalastyle_excludes = None
+
   @classmethod
   def setup_parser(cls, option_group, args, mkflag):
     super(Scalastyle, cls).setup_parser(option_group, args, mkflag)
@@ -43,53 +56,115 @@ def setup_parser(cls, option_group, args, mkflag):
 
   def __init__(self, *args, **kwargs):
     super(Scalastyle, self).__init__(*args, **kwargs)
-    self._scalastyle_config = self.context.config.get_required(self._CONFIG_SECTION, 'config')
-    if not os.path.exists(self._scalastyle_config):
+
+    self._initialize_config()
+    self._scalastyle_bootstrap_key = 'scalastyle'
+    self.register_jvm_tool(self._scalastyle_bootstrap_key, [':scalastyle'])
+
+  @property
+  def config_section(self):
+    return self._CONFIG_SECTION
+
+  def _initialize_config(self):
+    scalastyle_config = self.context.config.get(
+      self._CONFIG_SECTION, self._CONFIG_SECTION_CONFIG_OPTION)
+
+    # Scalastyle task by default isn't wired up in pants, but if it is installed
+    # via plugin, then the config file setting is required.
+    if not scalastyle_config:
+      raise Config.ConfigError(
+        'Scalastyle config is missing from section[{section}] option[{setting}] in '
+        'pants.ini.'.format(
+          section=self._CONFIG_SECTION,
+          setting=self._CONFIG_SECTION_CONFIG_OPTION))
+
+    # And the config setting value must be a valid file.
+    if not os.path.exists(scalastyle_config):
       raise Config.ConfigError(
-          'Scalastyle config file does not exist: %s' % self._scalastyle_config)
+        'Scalastyle config file specified in section[{section}] option[{setting}] in pants.ini '
+        'does not exist: {file}'.format(
+          section=self._CONFIG_SECTION,
+          setting=self._CONFIG_SECTION_CONFIG_OPTION,
+          file=scalastyle_config))
 
-    excludes_file = self.context.config.get(self._CONFIG_SECTION, 'excludes')
-    self._excludes = set()
+    excludes_file = self.context.config.get(
+      self._CONFIG_SECTION, self._CONFIG_SECTION_EXCLUDES_OPTION)
+
+    scalastyle_excludes = set()
     if excludes_file:
+      # excludes setting is optional, but if specified, must be a valid file.
       if not os.path.exists(excludes_file):
-        raise Config.ConfigError('Scalastyle excludes file does not exist: %s' % excludes_file)
-      self.context.log.debug('Using scalastyle excludes file %s' % excludes_file)
+        raise Config.ConfigError(
+          'Scalastyle excludes file specified in section[{section}] option[{setting}] in '
+          'pants.ini does not exist: {file}'.format(
+            section=self._CONFIG_SECTION,
+            setting=self._CONFIG_SECTION_EXCLUDES_OPTION,
+            file=excludes_file))
       with open(excludes_file) as fh:
         for pattern in fh.readlines():
-          self._excludes.add(re.compile(pattern.strip()))
-
-    self._scalastyle_bootstrap_key = 'scalastyle'
-    self.register_jvm_tool(self._scalastyle_bootstrap_key, [':scalastyle'])
+          scalastyle_excludes.add(re.compile(pattern.strip()))
+          self.context.log.debug(
+            'Scalastyle file exclude pattern: {pattern}'.format(pattern=pattern))
+    else:
+      # excludes setting is optional.
+      self.context.log.debug(
+        'Unable to get section[{section}] option[{setting}] value in pants.ini. '
+        'All scala sources will be checked.'.format(
+          section=self._CONFIG_SECTION, setting=self._CONFIG_SECTION_EXCLUDES_OPTION))
+
+    # Only transfer to local variables to the state at the end to minimize side effects.
+    self._scalastyle_config = scalastyle_config or None
+    self._scalastyle_excludes = scalastyle_excludes or None
 
   @property
-  def config_section(self):
-    return self._CONFIG_SECTION
+  def _should_skip(self):
+    return self.context.options.scalastyle_skip
+
+  def _get_non_synthetic_scala_targets(self, targets):
+    return filter(
+      lambda target: isinstance(target, Target)
+                     and target.has_sources(self._SCALA_SOURCE_EXTENSION)
+                     and (not target.is_synthetic),
+      targets)
+
+  def _should_include_source(self, source_filename):
+    if not self._scalastyle_excludes:
+      return True
+    for exclude in self._scalastyle_excludes:
+      if exclude.match(source_filename):
+        return False
+    return True
+
+  def _get_non_excluded_scala_sources(self, scala_targets):
+    # Get all the sources from the targets with the path relative to build root.
+    scala_sources = list()
+    for target in scala_targets:
+      scala_sources.extend(target.sources_relative_to_buildroot())
+
+    # make sure only the sources with scala extension stay.
+    scala_sources = filter(
+      lambda filename: filename.endswith(self._SCALA_SOURCE_EXTENSION),
+      scala_sources)
+
+    # filter out all sources matching exclude patterns, if specified in config.
+    scala_sources = filter(self._should_include_source, scala_sources)
+
+    return scala_sources
 
   def execute(self):
-    if self.context.options.scalastyle_skip:
-      self.context.log.debug('Skipping checkstyle.')
+    if self._should_skip:
+      self.context.log.info('Skipping scalastyle.')
       return
 
-    check_targets = list()
-    targets = self.context.targets()
+    targets = self._get_non_synthetic_scala_targets(self.context.targets())
+    self.context.log.debug('Non synthetic scala targets to be checked:')
     for target in targets:
-      for tgt in target.resolve():
-        if isinstance(tgt, Target) and tgt.has_sources('.scala'):
-          check_targets.append(tgt)
-
-    def filter_excludes(filename):
-      if self._excludes:
-        for exclude in self._excludes:
-          if exclude.match(filename):
-            return False
-      return True
+      self.context.log.debug('  {address_spec}'.format(address_spec=target.address.spec))
 
-    scala_sources = list()
-    for target in check_targets:
-      def collect(filename):
-        if filename.endswith('.scala'):
-          scala_sources.append(os.path.join(target.target_base, filename))
-      map(collect, filter(filter_excludes, target.sources))
+    scala_sources = self._get_non_excluded_scala_sources(targets)
+    self.context.log.debug('Non excluded scala sources to be checked:')
+    for source in scala_sources:
+      self.context.log.debug('  {source}'.format(source=source))
 
     if scala_sources:
       def call(srcs):
@@ -99,4 +174,5 @@ def call(srcs):
                             args=['-c', self._scalastyle_config] + srcs)
       result = Xargs(call).execute(scala_sources)
       if result != 0:
-        raise TaskError('java %s ... exited non-zero (%i)' % (Scalastyle._MAIN, result))
+        raise TaskError('java {entry} ... exited non-zero ({exit_code})'.format(
+          entry=Scalastyle._MAIN, exit_code=result))

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

@@ -7,6 +7,7 @@ python_test_suite(
     ':ide_gen',
     ':idea_gen',
     ':junit_run',
+    ':scalastyle',
     'tests/python/pants_test/backend/jvm/tasks/jvm_compile',
   ]
 )
@@ -49,3 +50,15 @@ python_tests(
   ]
 )
 
+python_tests(
+  name = 'scalastyle',
+  sources = ['test_scalastyle.py'],
+  dependencies = [
+    'src/python/pants/backend/jvm/targets:scala',
+    'src/python/pants/backend/jvm/tasks:scalastyle',
+    'src/python/pants/base:address',
+    'src/python/pants/base:config',
+    'src/python/pants/base:exceptions',
+    'tests/python/pants_test/jvm:nailgun_task_test_base',
+  ]
+)