Remove over 100 lines from checkperms.py.

tools/checkperms/checkperms.py

Index: tools/checkperms/checkperms.py
diff --git a/tools/checkperms/checkperms.py b/tools/checkperms/checkperms.py
index c5fb7c46aee3a73fa4fb30ca9653b204a71a9b35..a88b71be40b4a7d872193044f11a764128740ac8 100755
--- a/tools/checkperms/checkperms.py
+++ b/tools/checkperms/checkperms.py
@@ -11,80 +11,30 @@ see .cc files in green and get confused.
 
 To ignore a particular file, add it to WHITELIST_FILES.
 To ignore a particular extension, add it to WHITELIST_EXTENSIONS.
-To ignore whatever regexps your heart desires, add it WHITELIST_REGEX.
+To ignore a particular path, add it WHITELIST_PATHS.
 
 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 logging
 import optparse
 import os
-import pipes
-import re
 import stat
+import subprocess
 import sys
 
 #### USER EDITABLE SECTION STARTS HERE ####
 
 # Files with these extensions are allowed to have executable permissions.
-WHITELIST_EXTENSIONS = [
-    'bash',
-    'bat',
-    'dll',
-    'dylib',
-    'exe',
-    'pl',
-    'py',
-    'rb',
-    'sed',
-    'sh',
-]
-
-# Files that end the following paths are whitelisted too.
-WHITELIST_FILES = [
-    '/build/gyp_chromium',
-    '/build/linux/dump_app_syms',
-    '/build/linux/pkg-config-wrapper',
-    '/build/mac/strip_from_xcode',
-    '/build/mac/strip_save_dsym',
-    '/chrome/installer/mac/pkg-dmg',
-    '/chrome/test/data/webrtc/linux/peerconnection_server',
-    '/chrome/tools/build/linux/chrome-wrapper',
-    '/chrome/tools/build/mac/build_app_dmg',
-    '/chrome/tools/build/mac/clean_up_old_versions',
-    '/chrome/tools/build/mac/dump_product_syms',
-    '/chrome/tools/build/mac/generate_localizer',
-    '/chrome/tools/build/mac/make_sign_sh',
-    '/chrome/tools/build/mac/verify_order',
-    '/o3d/build/gyp_o3d',
-    '/o3d/gypbuild',
-    '/o3d/installer/linux/debian.in/rules',
-    '/third_party/icu/source/runconfigureicu',
-    '/third_party/gold/gold32',
-    '/third_party/gold/gold64',
-    '/third_party/gold/ld',
-    '/third_party/gold/ld.bfd',
-    '/third_party/lcov/bin/gendesc',
-    '/third_party/lcov/bin/genhtml',
-    '/third_party/lcov/bin/geninfo',
-    '/third_party/lcov/bin/genpng',
-    '/third_party/lcov/bin/lcov',
-    '/third_party/lcov/bin/mcov',
-    '/third_party/lcov-1.9/bin/gendesc',
-    '/third_party/lcov-1.9/bin/genhtml',
-    '/third_party/lcov-1.9/bin/geninfo',
-    '/third_party/lcov-1.9/bin/genpng',
-    '/third_party/lcov-1.9/bin/lcov',
-    '/third_party/libxml/linux/xml2-config',
-    '/third_party/lzma_sdk/executable/7za.exe',
-    '/third_party/swig/linux/swig',
-    '/third_party/tcmalloc/chromium/src/pprof',
-    '/tools/deep_memory_profiler/dmprof',
-    '/tools/git/post-checkout',
-    '/tools/git/post-merge',
-    '/tools/ld_bfd/ld',
-]
+WHITELIST_EXTENSIONS = (
+  'bat',
+  'dll',
+  'dylib',
+  'in',

[Lei Zhang, 2012/04/24 19:34:01]

Why is the '.in' extension whitelisted? Also, 'in' goes after 'exe'.

[M-A Ruel, 2012/04/25 01:30:11]

Replace 'in' with the 2 equivalent exceptions.

+  'exe',
+)
 
 # File names that are always whitelisted.  (These are all autoconf spew.)
 WHITELIST_FILENAMES = set((
@@ -95,256 +45,254 @@ WHITELIST_FILENAMES = set((
   'install-sh',
   'missing',
   'mkinstalldirs',
-  'scons',
   'naclsdk',
+  'scons',
 ))
 
 # File paths that contain these regexps will be whitelisted as well.
-WHITELIST_REGEX = [
-    re.compile('/third_party/openssl/'),
-    re.compile('/third_party/sqlite/'),
-    re.compile('/third_party/xdg-utils/'),
-    re.compile('/third_party/yasm/source/patched-yasm/config'),
-    re.compile('/third_party/ffmpeg/tools'),
-]
+WHITELIST_PATHS = (
+  # TODO(maruel): Detect ELF files.
+  'chrome/test/data/webrtc/linux/peerconnection_server',
+  'native_client_sdk/src/build_tools/sdk_tools/third_party/',
+  'out/',
+  # TODO(maruel): Fix these.
+  'third_party/android_testrunner/',
+  'third_party/closure_linter/',
+  'third_party/devscripts/licensecheck.pl.vanilla',
+  'third_party/hyphen/',
+  'third_party/jemalloc/',
+  'third_party/lcov-1.9/contrib/galaxy/conglomerate_functions.pl',
+  'third_party/lcov-1.9/contrib/galaxy/gen_makefile.sh',
+  'third_party/lcov/contrib/galaxy/conglomerate_functions.pl',
+  'third_party/lcov/contrib/galaxy/gen_makefile.sh',
+  'third_party/libevent/autogen.sh',
+  'third_party/libevent/test/test.sh',
+  'third_party/libxml/linux/xml2-config',
+  'third_party/libxml/src/ltmain.sh',
+  'third_party/mesa/',
+  'third_party/protobuf/',
+  'third_party/python_gflags/gflags.py',
+  'third_party/sqlite/',
+  'third_party/talloc/script/mksyms.sh',
+  'third_party/tcmalloc/',
+  'third_party/tlslite/setup.py',
+)
 
 #### USER EDITABLE SECTION ENDS HERE ####
 
-WHITELIST_EXTENSIONS_REGEX = re.compile(r'\.(%s)' %
-                                        '|'.join(WHITELIST_EXTENSIONS))
-
-WHITELIST_FILES_REGEX = re.compile(r'(%s)$' % '|'.join(WHITELIST_FILES))
-
-# Set to true for more output. This is set by the command line options.
-VERBOSE = False
-
-# Using forward slashes as directory separators, ending in a forward slash.
-# Set by the command line options.
-BASE_DIRECTORY = ''
-
-# The default if BASE_DIRECTORY is not set on the command line.
-DEFAULT_BASE_DIRECTORY = '../../..'
-
-# The directories which contain the sources managed by git.
-GIT_SOURCE_DIRECTORY = set()
-
-# The SVN repository url.
-SVN_REPO_URL = ''
-
-# Whether we are using SVN or GIT.
-IS_SVN = True
-
-# Executable permission mask
-EXECUTABLE_PERMISSION = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
-
-
-def IsWhiteListed(file_path):
-  """Returns True if file_path is in our whitelist of files to ignore."""
-  if WHITELIST_EXTENSIONS_REGEX.match(os.path.splitext(file_path)[1]):
-    return True
-  if WHITELIST_FILES_REGEX.search(file_path):
-    return True
-  if os.path.basename(file_path) in WHITELIST_FILENAMES:
-    return True
-  for regex in WHITELIST_REGEX:
-    if regex.search(file_path):
-      return True
-  return False
-
 
-def CheckFile(file_path):
-  """Checks file_path's permissions.
+def capture(cmd, cwd):
+  """Returns the output of a command.
 
-  Args:
-    file_path: The file path to check.
-
-  Returns:
-    Either a string describing the error if there was one, or None if the file
-    checked out OK.
+  Ignores the error code or stderr.
   """
-  if VERBOSE:
-    print 'Checking file: ' + file_path
-
-  file_path_lower = file_path.lower()
-  if IsWhiteListed(file_path_lower):
-    return None
-
-  # Not whitelisted, stat the file and check permissions.
-  try:
-    st_mode = os.stat(file_path).st_mode
-  except IOError, e:
-    return 'Failed to stat file: %s' % e
-  except OSError, e:
-    return 'Failed to stat file: %s' % e
-
-  if EXECUTABLE_PERMISSION & st_mode:
-    # Look if the file starts with #!/
-    with open(file_path, 'rb') as f:
-      if f.read(3) == '#!/':
-        # That's fine.
+  env = os.environ.copy()
+  env['LANG'] = 'en_us.UTF-8'
+  p = subprocess.Popen(
+      cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, env=env)
+  return p.communicate()[0]
+
+
+class ApiBase(object):
+  def __init__(self, root_dir, bare_output):
+    self.root_dir = root_dir
+    self.bare_output = bare_output
+
+  @staticmethod
+  def is_whitelisted(rel_path):
+    """Returns True if rel_path is in our whitelist of files to ignore."""
+    rel_path = rel_path.lower()
+    return (
+        os.path.splitext(rel_path)[1][1:] in WHITELIST_EXTENSIONS or
+        os.path.basename(rel_path) in WHITELIST_FILENAMES or
+        rel_path.startswith(WHITELIST_PATHS))
+
+  @staticmethod
+  def has_executable_bit(full_path):
+    """Returns if any executable bit is set."""
+    permission = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
+    return bool(permission & os.stat(full_path).st_mode)

[Lei Zhang, 2012/04/24 19:34:01]

Are you expecting os.stat() to not throw any exceptions?

[M-A Ruel, 2012/04/25 01:30:11]

Why would a file in the checkout not be readable by the user?

[Lei Zhang, 2012/04/25 04:01:02]

Ideally that would be the case, but the fact that this script exists seems to
indicate people can't set their file permissions correctly sometimes. Well, we
can just leave it as is and see if we get any complaints.

+
+  @staticmethod
+  def has_shebang(full_path):
+    """Returns if the file starts with #!/.
+
+    file_path is the absolute path to the file.
+    """
+    with open(full_path, 'rb') as f:
+      return f.read(3) == '#!/'
+
+  def check_file(self, rel_path):
+    """Checks file_path's permissions and returns an error if it is
+    inconsistent.
+    """
+    logging.debug('check_file(%s)' % rel_path)
+
+    if self.is_whitelisted(rel_path):
+      return None
+
+    full_path = os.path.join(self.root_dir, rel_path)
+    bit = self.has_executable_bit(full_path)
+    shebang = self.has_shebang(full_path)

[Lei Zhang, 2012/04/24 19:34:01]

I think this is going to slow down checkperms.py significantly, since you have
to read every file now.

[M-A Ruel, 2012/04/25 01:30:11]

All the following is on a standard 500gb HD.

linux / git checkout / cold
real    0m30.113s
user    0m1.660s
sys     0m1.340s

linux / git checkout / warm
real    0m1.854s
user    0m0.950s
sys     0m0.630s

Yes, this is less than 2 seconds, checking over 35k files.


linux / svn checkout / relatively warm
real    2m48.827s
user    0m1.570s
sys     0m0.840s

linux / svn checkout / warm
real    2m45.645s
user    0m1.540s
sys     0m0.840s

All the "real" time is spent in the "svn ls --recursive" call.


linux / no checkout / cold
This is the total worst case, when the svn detection failed and it iterate
inside all the directories, including .svn :)
real    12m37.555s
user    0m17.540s
sys     0m16.940s

linux / no checkout / warm
real    0m18.165s
user    0m8.250s
sys     0m5.640s


As you can see, the very vast majority is spent in "svn ls". It is so ridiculous
that on a warm disk cache, it's faster to check all the files, including
everything under .svn, than calling "svn ls". :(

I'm not sure how to help with that. A quick sampling on "Linux" builder shows it
currently takes ~50 seconds to complete. I'm not sure why it takes almost 3
minutes here. Since it is vastly bound by subversion, I do not expect a
significant slow down on the continuous integration server.

On my workstation, the previous version of checkperms.py takes:


This is on svn 1.6.12.

I tried without --recursive but it is even slower, I cancelled it after 19
minutes...


As a comparison, the old version of checkperms.py with warm cache:
src> time tools/checkperms/checkperms.py
Using base directory: /mnt/seagate1.5tb/maruel/src/svn_stuff/chrome/src
Checking directory: /mnt/seagate1.5tb/maruel/src/svn_stuff/chrome/src

SUCCESS


real    0m32.547s
user    0m1.700s
sys     0m1.880s

It is faster because it never calls "svn ls" and just checks everything.

Note that the old version of checkperms.py has issue with non-English LANGUAGE
variable.

[Lei Zhang, 2012/04/25 04:01:02]

How about we only check files with the executable bit set and see if they have a
shebang? If the file is not set executable, then do not worry about it?

On my machine, the existing script takes 165 seconds with a cold cache, and 90
seconds with a hot cache.
The new script takes 310 seconds with a cold cache, and 190 seconds with a hot
cache.
The new script minus the shebang check takes 140 seconds with a cold cache, and
100 seconds with a hot cache.

So even without the "svn ls" slowness, the new script with the shebang check is
quite a bit slower. 

Below is a micro-benchmark to demonstrate the issue:

----
#!/usr/bin/python

import os

def has_shebang(full_path):
  with open(full_path, 'rb') as f:
    f.read(3) == '#!/'

def list_dir(start_dir):
  return sorted(x for x in os.listdir(start_dir) if not x.startswith('.'))

def check_dir(start_dir):
  items = list_dir(start_dir)
  for item in items:
    full_path = os.path.join(start_dir, item)
    if os.path.isdir(full_path):
      check_dir(full_path)
    else:
      os.stat(full_path)
      has_shebang(full_path)

check_dir('.')
----

With a hot cache, it takes < 1 second whether you call has_shebang() or not.
With a cold cache, if you comment out has_shebang(), it takes 15 seconds. With
has_shebang(), it jumps to 100 seconds.

+    if bit != shebang:
+      if self.bare_output:
+        return rel_path
+      if bit:
+        return '%s: Has executable bit but not shebang' % rel_path
+      else:
+        return '%s: Has shebang but not executable bit' % 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:
+      full_path = os.path.join(self.root_dir, start_dir, item)
+      rel_path = full_path[len(self.root_dir) + 1:]
+      if os.path.isdir(full_path):
+        if self.is_whitelisted(rel_path):
+          continue
+        # Depth first.
+        errors.extend(self.check(rel_path))
+      else:
+        error = self.check_file(rel_path)
+        if error:
+          errors.append(error)
+    return errors
+
+  def list_dir(self, start_dir):
+    """Lists all the files and directory inside start_dir."""
+    return sorted(
+      x for x in os.listdir(os.path.join(self.root_dir, start_dir))
+      if not x.startswith('.')
+    )
+
+
+class ApiSvn(ApiBase):
+  @staticmethod
+  def get_info(dir_path):
+    """Returns svn meta-data for a svn checkout."""
+    if not os.path.isdir(dir_path):
+      return {}
+    out = capture(['svn', 'info', '.'], dir_path)
+    return dict(l.split(': ', 1) for l in out.splitlines() if l)
+
+  @classmethod
+  def get_root(cls, dir_path):
+    """Returns the svn checkout root or None."""
+    svn_url = cls.get_info(dir_path).get('Repository Root:')
+    if not svn_url:
+      return None
+    while True:
+      parent = os.path.dirname(dir_path)
+      if parent == dir_path:
         return None
-    # TODO(maruel): Check that non-executable file do not start with a shebang.
-    error = 'Contains executable permission'
-    if VERBOSE:
-      return '%s: %06o' % (error, st_mode)
-    return error
-  return None
-
-
-def ShouldCheckDirectory(dir_path):
-  """Determine if we should check the content of dir_path."""
-  if not IS_SVN:
-    return dir_path in GIT_SOURCE_DIRECTORY
-  repo_url = GetSvnRepositoryRoot(dir_path)
-  if not repo_url:
-    return False
-  return repo_url == SVN_REPO_URL
-
-
-def CheckDirectory(dir_path):
-  """Check the files in dir_path; recursively check its subdirectories."""
-  # Collect a list of all files and directories to check.
-  files_to_check = []
-  dirs_to_check = []
-  success = True
-  contents = os.listdir(dir_path)
-  for cur in contents:
-    full_path = os.path.join(dir_path, cur)
-    if os.path.isdir(full_path) and ShouldCheckDirectory(full_path):
-      dirs_to_check.append(full_path)
-    elif os.path.isfile(full_path):
-      files_to_check.append(full_path)
-
-  # First check all files in this directory.
-  for cur_file in files_to_check:
-    file_status = CheckFile(cur_file)
-    if file_status is not None:
-      print 'ERROR in %s\n%s' % (cur_file, file_status)
-      success = False
-
-  # Next recurse into the subdirectories.
-  for cur_dir in dirs_to_check:
-    if not CheckDirectory(cur_dir):
-      success = False
-  return success
-
-
-def GetGitSourceDirectory(root):
-  """Returns a set of the directories to be checked.
-
-  Args:
-    root: The repository root where a .git directory must exist.
-
-  Returns:
-    A set of directories which contain sources managed by git.
-  """
-  git_source_directory = set()
-  popen_out = os.popen('cd %s && git ls-files --full-name .' %
-                       pipes.quote(root))
-  for line in popen_out:
-    dir_path = os.path.join(root, os.path.dirname(line))
-    git_source_directory.add(dir_path)
-  git_source_directory.add(root)
-  return git_source_directory
-
-
-def GetSvnRepositoryRoot(dir_path):
-  """Returns the repository root for a directory.
-
-  Args:
-    dir_path: A directory where a .svn subdirectory should exist.
-
-  Returns:
-    The svn repository that contains dir_path or None.
-  """
-  svn_dir = os.path.join(dir_path, '.svn')
-  if not os.path.isdir(svn_dir):
-    return None
-  popen_out = os.popen('cd %s && svn info' % pipes.quote(dir_path))
-  for line in popen_out:
-    if line.startswith('Repository Root: '):
-      return line[len('Repository Root: '):].rstrip()
-  return None
-
-
-def main(argv):
+      if svn_url != cls.get_info(parent).get('Repository Root:'):
+        return dir_path
+      dir_path = parent
+
+  def list_dir(self, start_dir):
+    """Lists all the files and directory inside start_dir."""
+    return sorted(
+        capture(
+            ['svn', 'ls'], os.path.join(self.root_dir, start_dir)).splitlines()
+    )
+
+
+class ApiGit(ApiBase):
+  _files = None
+
+  @staticmethod
+  def get_root(dir_path):
+    """Returns the git checkout root or None."""
+    root = capture(['git', 'rev-parse', '--show-toplevel'], dir_path).strip()
+    if root:
+      return root
+
+  def list_dir(self, start_dir):
+    """Lists all the files and directory inside start_dir."""
+    if self._files is None:
+      # git has no way to get the list of files in a specific directory without
+      # recursing.
+      self._files = capture(['git', 'ls-files'], self.root_dir).splitlines()
+    start_dir = start_dir[len(self.root_dir) + 1:]
+    return sorted(
+      x[len(start_dir):] for x in self._files if x.startswith(start_dir)
+    )
+
+
+def get_scm(dir_path, log, bare):
+  """Returns a properly configured ApiBase instance."""
+  dir_path = os.path.abspath(dir_path)
+  root = ApiSvn.get_root(dir_path)
+  if root:
+    if log:
+      logging.info('Found svn at %s' % root)
+    return ApiSvn(root, bare)
+  root = ApiGit.get_root(dir_path)
+  if root:
+    if log:
+      logging.info('Found git at %s' % root)
+    return ApiGit(root, bare)
+
+  # Returns a non-scm aware checker.
+  if log:
+    logging.warn('Failed to determine the SCM for %s' % dir_path)
+  return ApiBase(dir_path, bare)
+
+
+def main():
   usage = """Usage: python %prog [--root <root>] [tocheck]
   tocheck  Specifies the directory, relative to root, to check. This defaults
            to "." so it checks everything.
 
 Examples:
-  python checkperms.py
-  python checkperms.py --root /path/to/source chrome"""
-
-  option_parser = optparse.OptionParser(usage=usage)
-  option_parser.add_option('--root', dest='base_directory',
-                           default=DEFAULT_BASE_DIRECTORY,
-                           help='Specifies the repository root. This defaults '
-                           'to %default relative to the script file, which '
-                           'will normally be the repository root.')
-  option_parser.add_option('-v', '--verbose', action='store_true',
-                           help='Print debug logging')
-  options, args = option_parser.parse_args()
-
-  global VERBOSE
-  if options.verbose:
-    VERBOSE = True
-
-  # Optional base directory of the repository.
-  global BASE_DIRECTORY
-  if (not options.base_directory or
-      options.base_directory == DEFAULT_BASE_DIRECTORY):
-    BASE_DIRECTORY = os.path.abspath(
-        os.path.join(os.path.abspath(argv[0]), DEFAULT_BASE_DIRECTORY))
-  else:
-    BASE_DIRECTORY = os.path.abspath(argv[2])
-
-  # Figure out which directory we have to check.
-  if not args:
-    # No directory to check specified, use the repository root.
-    start_dir = BASE_DIRECTORY
-  elif len(args) == 1:
-    # Directory specified. Start here. It's supposed to be relative to the
-    # base directory.
-    start_dir = os.path.abspath(os.path.join(BASE_DIRECTORY, args[0]))
-  else:
-    # More than one argument, we don't handle this.
-    option_parser.print_help()
-    return 1
-
-  print 'Using base directory:', BASE_DIRECTORY
-  print 'Checking directory:', start_dir
-
-  BASE_DIRECTORY = BASE_DIRECTORY.replace('\\', '/')
-  start_dir = start_dir.replace('\\', '/')
-
-  success = True
-  if os.path.exists(os.path.join(BASE_DIRECTORY, '.svn')):
-    global SVN_REPO_URL
-    SVN_REPO_URL = GetSvnRepositoryRoot(BASE_DIRECTORY)
-    if not SVN_REPO_URL:
-      print 'Cannot determine the SVN repo URL'
-      success = False
-  elif os.path.exists(os.path.join(BASE_DIRECTORY, '.git')):
-    global IS_SVN
-    IS_SVN = False
-    global GIT_SOURCE_DIRECTORY
-    GIT_SOURCE_DIRECTORY = GetGitSourceDirectory(BASE_DIRECTORY)
-    if not GIT_SOURCE_DIRECTORY:
-      print 'Cannot determine the list of GIT directories'
-      success = False
+  python %prog
+  python %prog --root /path/to/source chrome"""
+
+  parser = optparse.OptionParser(usage=usage)
+  parser.add_option(
+      '--root',
+      default=get_scm('.', False, False).root_dir,
+      help='Specifies the repository root. This defaults '
+           'to "%default" relative to the script file, which '
+           'will normally be the repository root.')
+  parser.add_option(
+      '-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')
+  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 not options.root:
+    parser.error('Must specify --root')
+
+  # Guess again the SCM used.
+  api = get_scm(options.root, True, options.bare)
+  if args:
+    start_dir = args[0]
   else:
-    print 'Cannot determine the SCM used in %s' % BASE_DIRECTORY
-    success = False
+    start_dir = api.root_dir
 
-  if success:
-    success = CheckDirectory(start_dir)
-  if not success:
-    print '\nFAILED\n'
+  errors = api.check(start_dir)
+  if errors:
+    if not options.bare:
+      print '\nFAILED\n'
+    print '\n'.join(errors)
     return 1
-  print '\nSUCCESS\n'
+  if not options.bare:
+    print '\nSUCCESS\n'
   return 0
 
 
 if '__main__' == __name__:
-  sys.exit(main(sys.argv))
+  sys.exit(main())