Make checkstyle PRESUBMIT use warnings and errors annotations.

BUG=318404
R=maruel@chromium.org

Review URL: https://codereview.chromium.org/652433004

Cr-Commit-Position: refs/heads/master@{#299513}

2 files changed, 39 insertions, 38 deletions

diff --git a/tools/android/checkstyle/checkstyle.py b/tools/android/checkstyle/checkstyle.py
index df9014d..6b54c77 100644
--- a/tools/android/checkstyle/checkstyle.py
+++ b/tools/android/checkstyle/checkstyle.py
@@ -6,6 +6,7 @@
 
 import os
 import subprocess
+import xml.dom.minidom
 
 
 CHROMIUM_SRC = os.path.normpath(
@@ -34,11 +35,9 @@ def RunCheckstyle(input_api, output_api, style_file):
     check = subprocess.Popen(['java', '-cp',
                               CHECKSTYLE_ROOT,
                               'com.puppycrawl.tools.checkstyle.Main', '-c',
-                              style_file] + java_files,
+                              style_file, '-f', 'xml'] + java_files,
                              stdout=subprocess.PIPE, env=checkstyle_env)
     stdout, _ = check.communicate()
-    if check.returncode == 0:
-      return []
   except OSError as e:
     import errno
     if e.errno == errno.ENOENT:
@@ -46,24 +45,32 @@ def RunCheckstyle(input_api, output_api, style_file):
                        'build/install-build-deps-android.sh')
       return [output_api.PresubmitPromptWarning(install_error)]
 
-  # Remove non-error values from stdout
-  errors = stdout.splitlines()
+  result_errors = []
+  result_warnings = []
 
-  if errors and errors[0] == 'Starting audit...':
-    del errors[0]
-  if errors and errors[-1] == 'Audit done.':
-    del errors[-1]
+  local_path = input_api.PresubmitLocalPath()
+  root = xml.dom.minidom.parseString(stdout)
+  for fileElement in root.getElementsByTagName('file'):
+    fileName = fileElement.attributes['name'].value
+    fileName = os.path.relpath(fileName, local_path)
+    errors = fileElement.getElementsByTagName('error')
+    for error in errors:
+      line = error.attributes['line'].value
+      column = ''
+      if error.hasAttribute('column'):
+        column = '%s:' % (error.attributes['column'].value)
+      message = error.attributes['message'].value
+      result = '  %s:%s:%s %s' % (fileName, line, column, message)
 
-  # Filter out warnings
-  errors = [x for x in errors if 'warning: ' not in x]
-  if not errors:
-    return []
+      severity = error.attributes['severity'].value
+      if severity == 'error':
+        result_errors.append(result)
+      elif severity == 'warning':
+        result_warnings.append(result)
 
-  local_path = input_api.PresubmitLocalPath()
-  output = []
-  for error in errors:
-    # Change the full file path to relative path in the output lines
-    full_path, end = error.split(':', 1)
-    rel_path = os.path.relpath(full_path, local_path)
-    output.append('  %s:%s' % (rel_path, end))
-  return [output_api.PresubmitPromptWarning('\n'.join(output))]
+  result = []
+  if result_warnings:
+    result.append(output_api.PresubmitPromptWarning('\n'.join(result_warnings)))
+  if result_errors:
+    result.append(output_api.PresubmitError('\n'.join(result_errors)))
+  return result
diff --git a/tools/android/checkstyle/chromium-style-5.0.xml b/tools/android/checkstyle/chromium-style-5.0.xml
index bc40f87..141f93b 100644
--- a/tools/android/checkstyle/chromium-style-5.0.xml
+++ b/tools/android/checkstyle/chromium-style-5.0.xml
@@ -12,25 +12,27 @@
       <property name="severity" value="error"/>
     </module>
     <module name="IllegalCatch">
-      <property name="severity" value="warning"/>
+      <property name="severity" value="info"/>
     </module>
     <module name="RedundantImport">
       <message key="import.redundant" value="Redundant import: {0}. Use :JavaImportOrganize (ECLIM) or Ctrl+Shift+O (Eclipse) to sort imports"/>
-      <property name="severity" value="error"/>
+      <!-- This check is set to a warning because JNI generator sometimes requires to leave an unused import -->
+      <property name="severity" value="warning"/>
     </module>
     <module name="UnusedImports">
-      <property name="severity" value="error"/>
-      <property name="processJavadoc" value="true"/>
       <message key="import.unused" value="Unused import: {0}. Use :JavaImportOrganize (ECLIM) or Ctrl+Shift+O (Eclipse) to sort imports"/>
+      <!-- This check is set to a warning because JNI generator sometimes requires to leave an unused import -->
+      <property name="severity" value="warning"/>
+      <property name="processJavadoc" value="true"/>
     </module>
     <module name="JavadocType">
-      <property name="severity" value="error"/>
+      <property name="severity" value="warning"/>
       <property name="tokens" value="INTERFACE_DEF, CLASS_DEF"/>
       <property name="scope" value="public"/>
       <message key="javadoc.missing" value="Public classes and interfaces require JavaDoc comments."/>
     </module>
     <module name="JavadocMethod">
-      <property name="severity" value="warning"/>
+      <property name="severity" value="info"/>
       <property name="scope" value="public"/>
       <property name="allowMissingParamTags" value="true"/>
       <property name="allowMissingPropertyJavadoc" value="true"/>
@@ -99,7 +101,7 @@
       <property name="severity" value="error"/>
     </module>
     <module name="NeedBraces">
-      <property name="severity" value="warning"/>
+      <property name="severity" value="info"/>
       <property name="tokens" value="LITERAL_FOR, LITERAL_WHILE, LITERAL_DO"/>
     </module>
     <module name="EmptyBlock">
@@ -140,7 +142,7 @@
       <property name="severity" value="error"/>
     </module>
     <module name="NoFinalizer">
-      <property name="severity" value="warning"/>
+      <property name="severity" value="info"/>
     </module>
     <module name="ParenPad">
       <property name="severity" value="error"/>
@@ -155,7 +157,7 @@
     </module>
     <!-- TODO(aurimas): make indentation an error once https://github.com/checkstyle/checkstyle/issues/255 is fixed. -->
     <module name="Indentation">
-      <property name="severity" value="warning"/>
+      <property name="severity" value="info"/>
       <property name="basicOffset" value="4"/>
       <property name="throwsIndent" value="8"/>
     </module>
@@ -164,14 +166,6 @@
     <property name="severity" value="error"/>
   </module>
   <module name="RegexpSingleline">
-    <property name="format" value="((//.*)|(\*.*))FIXME"/>
-    <property name="message" value="TODO is preferred to FIXME.  e.g. &quot;TODO(johndoe):"/>
-  </module>
-  <module name="RegexpSingleline">
-    <property name="format" value="((//.*)|(\*.*))(?&lt;!TODO\(.{0,100})(TODO[^(])|(TODO\([^)]*$)"/>
-    <property name="message" value="All TODOs should be named.  e.g. &quot;TODO(johndoe):"/>
-  </module>
-  <module name="RegexpSingleline">
     <property name="severity" value="error"/>
     <property name="format" value="[ \t]+$"/>
     <property name="message" value="Trailing whitespace"/>