Add support for JSON output to checkperms.py

tools/checkperms/checkperms.py

 #!/usr/bin/env python
 # 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.
 
 """Makes sure files have the right permissions.
 
 Some developers have broken SCM configurations that flip the svn:executable
 permission on for no good reason. Unix developers who run ls --color will then
 see .cc files in green and get confused.
 
 - For file extensions that must be executable, add it to EXECUTABLE_EXTENSIONS.
 - For file extensions that must not be executable, add it to
   NOT_EXECUTABLE_EXTENSIONS.
 - To ignore all the files inside a directory, add it to IGNORED_PATHS.
 - For file base name with ambiguous state and that should not be checked for
   shebang, add it to IGNORED_FILENAMES.
 
 Any file not matching the above will be opened and looked if it has a shebang
 or an ELF header. If this does not match the executable bit on the file, the
 file will be flagged.
 
 Note that all directory separators must be slashes (Unix-style) and not
 backslashes. All directories should be relative to the source root and all
 file paths should be only lowercase.
 """
 
+import json
 import logging
 import optparse
 import os
 import stat
 import string
 import subprocess
 import sys
 
 #### USER EDITABLE SECTION STARTS HERE ####
 
@@ skipping 253 matching lines @@
 def has_shebang_or_is_elf(full_path):
   """Returns if the file starts with #!/ or is an ELF binary.
 
   full_path is the absolute path to the file.
   """
   with open(full_path, 'rb') as f:
     data = f.read(4)
     return (data[:3] == '#!/', data == '\x7fELF')
 
 
-def check_file(root_path, rel_path, bare_output):
+def check_file(root_path, rel_path):
   """Checks the permissions of the file whose path is root_path + rel_path and
   returns an error if it is inconsistent.

[M-A Ruel, 2014/06/05 15:00:47]

It's not clear that it returns None when there's no error.

[Paweł Hajdan Jr., 2014/06/05 15:43:54]

Done.

 
   It is assumed that the file is not ignored by is_ignored().
 
   If the file name is matched with must_be_executable() or
   must_not_be_executable(), only its executable bit is checked.
   Otherwise, the first few bytes of the file are read to verify if it has a
   shebang or ELF header and compares this with the executable bit on the file.
   """
   full_path = os.path.join(root_path, rel_path)
+  def result_dict(error):
+    return {
+      'full_path': full_path,
+      'rel_path': rel_path,

[M-A Ruel, 2014/06/05 15:00:47]

sort keys

[Paweł Hajdan Jr., 2014/06/05 15:43:54]

Done.

+      'error': error,
+    }
   try:
     bit = has_executable_bit(full_path)
   except OSError:
     # It's faster to catch exception than call os.path.islink(). Chromium
     # tree happens to have invalid symlinks under
     # third_party/openssl/openssl/test/.
     return None
 
   if must_be_executable(rel_path):
     if not bit:
-      if bare_output:
-        return full_path
-      return '%s: Must have executable bit set' % full_path
+      return result_dict('Must have executable bit set')
     return
   if must_not_be_executable(rel_path):
     if bit:
-      if bare_output:
-        return full_path
-      return '%s: Must not have executable bit set' % full_path
+      return result_dict('Must not have executable bit set')
     return
 
   # For the others, it depends on the file header.
   (shebang, elf) = has_shebang_or_is_elf(full_path)
   if bit != (shebang or elf):
-    if bare_output:
-      return full_path
     if bit:
-      return '%s: Has executable bit but not shebang or ELF header' % full_path
+      return result_dict('Has executable bit but not shebang or ELF header')
     if shebang:
-      return '%s: Has shebang but not executable bit' % full_path
-    return '%s: Has ELF header but not executable bit' % full_path
+      return result_dict('Has shebang but not executable bit')
+    return result_dict('Has ELF header but not executable bit')
 
 
-def check_files(root, files, bare_output):
+def check_files(root, files):
   errors = []

[M-A Ruel, 2014/06/05 15:00:47]

This function should be two lines instead of 10:
gen = (check_file(root, f) for f in files if not is_ignored(f))
return filter(None, gen)

[Paweł Hajdan Jr., 2014/06/05 15:43:54]

Done.

   for rel_path in files:
     if is_ignored(rel_path):
       continue
 
-    error = check_file(root, rel_path, bare_output)
+    error = check_file(root, rel_path)
     if error:
       errors.append(error)
 
   return errors
 
 
 class ApiBase(object):
   def __init__(self, root_dir, bare_output):
     self.root_dir = root_dir
     self.bare_output = bare_output
     self.count = 0
     self.count_read_header = 0
 
   def check_file(self, rel_path):
     logging.debug('check_file(%s)' % rel_path)
     self.count += 1
 
     if (not must_be_executable(rel_path) and
         not must_not_be_executable(rel_path)):
       self.count_read_header += 1
 
-    return check_file(self.root_dir, rel_path, self.bare_output)
+    return check_file(self.root_dir, rel_path)
 
   def check_dir(self, rel_path):
     return self.check(rel_path)
 
   def check(self, start_dir):
     """Check the files in start_dir, recursively check its subdirectories."""
     errors = []
     items = self.list_dir(start_dir)
     logging.info('check(%s) -> %d' % (start_dir, len(items)))
     for item in items:
@@ skipping 110 matching lines @@
       '-v', '--verbose', action='count', default=0, help='Print debug logging')
   parser.add_option(
       '--bare',
       action='store_true',
       default=False,
       help='Prints the bare filename triggering the checks')
   parser.add_option(
       '--file', action='append', dest='files',
       help='Specifics a list of files to check the permissions of. Only these '
       'files will be checked')
+  parser.add_option('--json', help='Path to JSON output file')
   options, args = parser.parse_args()
 
   levels = [logging.ERROR, logging.INFO, logging.DEBUG]
   logging.basicConfig(level=levels[min(len(levels) - 1, options.verbose)])
 
   if len(args) > 1:
     parser.error('Too many arguments used')
 
   if options.root:
     options.root = os.path.abspath(options.root)
 
   if options.files:
-    errors = check_files(options.root, options.files, options.bare)
-    print '\n'.join(errors)
-    return bool(errors)
+    # --file implies --bare (for PRESUBMIT.py).
+    options.bare = True
 
-  api = get_scm(options.root, options.bare)
-  if args:
-    start_dir = args[0]
+    errors = check_files(options.root, options.files)
   else:
-    start_dir = api.root_dir
+    api = get_scm(options.root, options.bare)
+    if args:

[M-A Ruel, 2014/06/05 15:00:47]

start_dir = args[0] if args else api.root_dir

[Paweł Hajdan Jr., 2014/06/05 15:43:54]

Done.

+      start_dir = args[0]
+    else:
+      start_dir = api.root_dir
 
-  errors = api.check(start_dir)
+    errors = api.check(start_dir)
 
-  if not options.bare:
-    print 'Processed %s files, %d files where tested for shebang/ELF header' % (
-        api.count, api.count_read_header)
+    if not options.bare:
+      print ('Processed %s files, %d files where tested for shebang/ELF '

[M-A Ruel, 2014/06/05 15:00:47]

print(' ...

it's a function call now.

[Paweł Hajdan Jr., 2014/06/05 15:43:54]

Done.

+             'header' % (api.count, api.count_read_header))
+
+  if options.json:
+    with open(options.json, 'w') as f:

[M-A Ruel, 2014/06/05 15:00:47]

'wb'

CR have no place in this world.

[Paweł Hajdan Jr., 2014/06/05 15:43:54]

Keeping 'w' per offline discussion and consistency with other json call sites.

+      json.dump(errors, f)
 
   if errors:
-    if not options.bare:
+    if options.bare:
+      print '\n'.join([e['full_path'] for e in errors])

[M-A Ruel, 2014/06/05 15:00:47]

Remove [], serialization of a generator into a list is unnecessary.

[Paweł Hajdan Jr., 2014/06/05 15:43:54]

Done.

+    else:
       print '\nFAILED\n'
-    print '\n'.join(errors)
+      print '\n'.join(['%s: %s' % (e['full_path'], e['error']) for e in errors])

[M-A Ruel, 2014/06/05 15:00:47]

Same.

[Paweł Hajdan Jr., 2014/06/05 15:43:54]

Done.

     return 1
   if not options.bare:
     print '\nSUCCESS\n'
   return 0
 
 
 if '__main__' == __name__:
   sys.exit(main())