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..ef780ed91f9bc0be111dcacb00898f0560c5ea1b 100755
--- a/tools/checkperms/checkperms.py
+++ b/tools/checkperms/checkperms.py
@@ -11,80 +11,29 @@ 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',
+  'exe',
+)
 
 # File names that are always whitelisted.  (These are all autoconf spew.)
 WHITELIST_FILENAMES = set((
@@ -95,256 +44,298 @@ 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',
+  'chrome/installer/mac/sign_app.sh.in',
+  'chrome/installer/mac/sign_versioned_dir.sh.in',
+  '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 = '../../..'
+def capture(cmd, cwd):
+  """Returns the output of a command.
 
-# 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
+  Ignores the error code or stderr.
+  """
+  logging.debug(' '.join(cmd))
+  env = os.environ.copy()
+  env['LANGUAGE'] = 'en_US.UTF-8'

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

I thought $LANG is the right variable to use here. Which command were you
running that depends on $LANGUAGE being set correctly?

[M-A Ruel, 2012/04/27 16:49:17]

svn ls output is localized. :(

+  p = subprocess.Popen(
+      cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, env=env)
+  return p.communicate()[0]
 
 
-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 get_svn_info(dir_path):
+  """Returns svn meta-data for a svn checkout."""
+  if not os.path.isdir(dir_path):
+    return {}
+  out = capture(['svn', 'info', '.', '--non-interactive'], dir_path)
+  return dict(l.split(': ', 1) for l in out.splitlines() if l)
 
 
-def CheckFile(file_path):
-  """Checks file_path's permissions.
+def get_svn_url(dir_path):
+  return get_svn_info(dir_path).get('Repository Root')
 
-  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.
+def get_svn_root(dir_path):
+  """Returns the svn checkout root or None."""
+  svn_url = get_svn_url(dir_path)
+  if not svn_url:
+    return None
+  logging.info('svn url: %s' % svn_url)
+  while True:
+    parent = os.path.dirname(dir_path)
+    if parent == dir_path:
+      return None
+    if svn_url != get_svn_url(parent):
+      return dir_path
+    dir_path = parent
+
+
+def get_git_root(dir_path):
+  """Returns the git checkout root or None."""
+  root = capture(['git', 'rev-parse', '--show-toplevel'], dir_path).strip()
+  if root:
+    return root
+
+
+class ApiBase(object):
+  def __init__(self, root_dir, bare_output):
+    self.root_dir = root_dir
+    self.bare_output = bare_output
+    self.count = 0
+
+  @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)
+
+  @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)
+    self.count += 1
+
+    full_path = os.path.join(self.root_dir, rel_path)
+    try:
+      bit = self.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
+    shebang = self.has_shebang(full_path)
+    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_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:
+      full_path = os.path.join(self.root_dir, start_dir, item)
+      rel_path = full_path[len(self.root_dir) + 1:]
+      if self.is_whitelisted(rel_path):
+        continue
+      if os.path.isdir(full_path):
+        # Depth first.
+        errors.extend(self.check_dir(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 ApiSvnQuick(ApiBase):

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

With this implementation, linux/svn/warm gets down to:
real    0m42.968s
user    0m24.070s
sys     0m17.220s

Still an order of magnitude slower than with git but still better than crawling
everything.

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

I'm fine with this.

+  """Returns all files in svn-versioned directories, independent of the fact if
+  they are versionned.
+
+  Uses svn info in each directory to determine which directories should be
+  crawled.
   """
-  if VERBOSE:
-    print 'Checking file: ' + file_path
+  def __init__(self, *args):
+    super(ApiSvnQuick, self).__init__(*args)
+    self.url = get_svn_info(self.root_dir)['Repository Root']
 
-  file_path_lower = file_path.lower()
-  if IsWhiteListed(file_path_lower):
-    return None
+  def check_dir(self, rel_path):
+    if get_svn_url(os.path.join(self.root_dir, rel_path)) != self.url:
+      return []
+    return super(ApiSvnQuick, self).check_dir(rel_path)
 
-  # 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.
-        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
 
+class ApiAllFilesAtOnceBase(ApiBase):
+  _files = None
 
-def GetSvnRepositoryRoot(dir_path):
-  """Returns the repository root for a directory.
+  def list_dir(self, start_dir):
+    """Lists all the files and directory inside start_dir."""
+    if self._files is None:
+      self._files = sorted(self._get_all_files())
+      if not self.bare_output:
+        print 'Found %s files' % len(self._files)
+    start_dir = start_dir[len(self.root_dir) + 1:]
+    return [
+      x[len(start_dir):] for x in self._files if x.startswith(start_dir)
+    ]
 
-  Args:
-    dir_path: A directory where a .svn subdirectory should exist.
+  def _get_all_files(self):
+    """Lists all the files and directory inside self._root_dir."""
+    raise NotImplementedError()
 
-  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
 
+class ApiSvn(ApiAllFilesAtOnceBase):
+  """Returns all the subversion controlled files.
 
-def main(argv):
+  Warning: svn ls is abnormally slow.
+  """
+  def _get_all_files(self):
+    cmd = ['svn', 'ls', '--non-interactive', '--recursive']
+    return (
+        x for x in capture(cmd, self.root_dir).splitlines()
+        if not x.endswith(os.path.sep))
+
+
+class ApiGit(ApiAllFilesAtOnceBase):
+  def _get_all_files(self):
+    return capture(['git', 'ls-files'], cwd=self.root_dir).splitlines()
+
+
+def get_scm(dir_path, bare):
+  """Returns a properly configured ApiBase instance."""
+  dir_path = os.path.abspath(dir_path)
+  root = get_svn_root(dir_path)
+  if root:
+    if not bare:
+      print('Found subversion checkout at %s' % root)
+    return ApiSvnQuick(root, bare)
+  root = get_git_root(dir_path)
+  if root:
+    if not bare:
+      print('Found git repository at %s' % root)
+    return ApiGit(root, bare)
+
+  # Returns a non-scm aware checker.
+  if not bare:
+    print('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))
+  python %prog
+  python %prog --root /path/to/source chrome"""
+
+  parser = optparse.OptionParser(usage=usage)
+  parser.add_option(
+      '--root',
+      help='Specifies the repository root. This defaults '
+           'to the checkout 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')
+
+  # Guess again the SCM used.
+  api = get_scm(options.root or '.', options.bare)
+  if args:
+    start_dir = args[0]
   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
+    start_dir = api.root_dir
 
-  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
-  else:
-    print 'Cannot determine the SCM used in %s' % BASE_DIRECTORY
-    success = False
+  errors = api.check(start_dir)
+
+  if not options.bare:
+    print 'Processed %s files' % api.count
 
-  if success:
-    success = CheckDirectory(start_dir)
-  if not success:
-    print '\nFAILED\n'
+  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())