Fix _CheckFilePermissions to actually flag presubmit errors

PRESUBMIT.py

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Top-level presubmit script for Chromium.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into gcl.
 """
 
 
 import re

[M-A Ruel, 2014/01/29 21:28:19]

Remove

[adamk, 2014/01/29 21:39:54]

This and the other re comments below seem like they should be done in a separate
change.

 import subprocess

[M-A Ruel, 2014/01/29 21:28:19]

Remove

[adamk, 2014/01/29 21:39:54]

See below, I don't think check_output does what I want (though my python and
PRESUBMIT knowledge is rudimentary).

 import sys
 
 
 _EXCLUDED_PATHS = (
     r"^breakpad[\\\/].*",
     r"^native_client_sdk[\\\/]src[\\\/]build_tools[\\\/]make_rules.py",
     r"^native_client_sdk[\\\/]src[\\\/]build_tools[\\\/]make_simple.py",
     r"^native_client_sdk[\\\/]src[\\\/]tools[\\\/].*.mk",
     r"^net[\\\/]tools[\\\/]spdyshark[\\\/].*",
     r"^skia[\\\/].*",
@@ skipping 495 matching lines @@
         warning_descriptions))
   return results
 
 
 def _CheckFilePermissions(input_api, output_api):
   """Check that all files have their permissions properly set."""
   args = [sys.executable, 'tools/checkperms/checkperms.py', '--root',
           input_api.change.RepositoryRoot()]
   for f in input_api.AffectedFiles():
     args += ['--file', f.LocalPath()]
-  errors = []
-  (errors, stderrdata) = subprocess.Popen(args).communicate()
-
-  results = []
+  checkperms = subprocess.Popen(args, stdout=subprocess.PIPE)

[M-A Ruel, 2014/01/29 21:28:19]

What you want is
errors = input_api.subprocess.check_output(args)

unless checkperms.py normally return non-zero.

[adamk, 2014/01/29 21:39:54]

Not sure what you mean by "normally", but it returns non-zero when there are bad
permissions, so it seems like check_output's not a good match here.

[M-A Ruel, 2014/01/29 21:47:05]

Normally would be in absence of catastrophic failure. Still, do not add
reference to global subprocess import, it was unwarranted. Use
input_api.subprocess instead.

[adamk, 2014/01/29 21:52:54]

Ok, will do, but note that I didn't add this reference, it was here already.

+  errors = checkperms.communicate()[0].strip()
   if errors:
-    results.append(output_api.PresubmitError('checkperms.py failed.',
-                                             errors))
-  return results
+    return [output_api.PresubmitError('checkperms.py failed.',
+                                      errors.splitlines())]
+  return []
 
 
 def _CheckNoAuraWindowPropertyHInHeaders(input_api, output_api):
   """Makes sure we don't include ui/aura/window_property.h
   in header files.
   """
   pattern = input_api.re.compile(r'^#include\s*"ui/aura/window_property.h"')
   errors = []
   for f in input_api.AffectedFiles():
     if not f.LocalPath().endswith('.h'):
@@ skipping 241 matching lines @@
   a specific filename by adding /DEPS. This is chosen as a file that
   will seldom or never be subject to per-file include_rules.
   """
   # We ignore deps entries on auto-generated directories.
   AUTO_GENERATED_DIRS = ['grit', 'jni']
 
   # This pattern grabs the path without basename in the first
   # parentheses, and the basename (if present) in the second. It
   # relies on the simple heuristic that if there is a basename it will
   # be a header file ending in ".h".
   pattern = re.compile(

[M-A Ruel, 2014/01/29 21:28:19]

Replwith with: input_api.re.compile(

       r"""['"]\+([^'"]+?)(/[a-zA-Z0-9_]+\.h)?['"].*""")
   results = set()
   for changed_line in changed_lines:
     m = pattern.match(changed_line)
     if m:
       path = m.group(1)
       if path.split('/')[0] not in AUTO_GENERATED_DIRS:
         if m.group(2):
           results.add('%s%s' % (path, m.group(2)))
         else:
@@ skipping 97 matching lines @@
                      r"bitmaptools.cc$",
                  r"^ui[\\\/]aura[\\\/]bench[\\\/]bench_main\.cc$",))
   source_file_filter = lambda x: input_api.FilterSourceFile(
       x, white_list=(file_inclusion_pattern,), black_list=black_list)
 
   log_info = []
   printf = []
 
   for f in input_api.AffectedSourceFiles(source_file_filter):
     contents = input_api.ReadFile(f, 'rb')
     if re.search(r"\bD?LOG\s*\(\s*INFO\s*\)", contents):

[M-A Ruel, 2014/01/29 21:28:19]

Here too, and up to line 922.

       log_info.append(f.LocalPath())
     elif re.search(r"\bD?LOG_IF\s*\(\s*INFO\s*,", contents):
       log_info.append(f.LocalPath())
 
     if re.search(r"\bprintf\(", contents):
       printf.append(f.LocalPath())
     elif re.search(r"\bfprintf\((stdout|stderr)", contents):
       printf.append(f.LocalPath())
 
   if log_info:
@@ skipping 530 matching lines @@
     trybots.extend(GetDefaultTryConfigs(['cros_x86']))
 
   # The AOSP bot doesn't build the chrome/ layer, so ignore any changes to it
   # unless they're .gyp(i) files as changes to those files can break the gyp
   # step on that bot.
   if (not all(re.search('^chrome', f) for f in files) or
       any(re.search('\.gypi?$', f) for f in files)):
     trybots.extend(GetDefaultTryConfigs(['android_aosp']))
 
   return trybots