Simplify PRESUBMIT.py and check_package_boundaries.py

Bug: webrtc:6954
Change-Id: Ia93eb8cc8a13bdcba5217fd8d52b72defa108b2f
Reviewed-on: https://webrtc-review.googlesource.com/6021
Commit-Queue: Oleh Prypin <oprypin@webrtc.org>
Reviewed-by: Henrik Kjellander <kjellander@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20158}

PRESUBMIT.py

@@ -9,9 +9,9 @@
 import json
 import os
 import re
-import subprocess
 import sys
 from collections import defaultdict
+from contextlib import contextmanager
 
 
 # Files and directories that are *skipped* by cpplint in the presubmit script.
@@ -107,16 +107,15 @@ SOURCES_RE = re.compile(r'sources \+?= \[(?P<sources>.*?)\]',
 FILE_PATH_RE = re.compile(r'"(?P<file_path>(\w|\/)+)(?P<extension>\.\w+)"')
 
 
-def _RunCommand(command, cwd):
-  """Runs a command and returns the output from that command."""
-  p = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
-                       cwd=cwd)
-  stdout = p.stdout.read()
-  stderr = p.stderr.read()
-  p.wait()
-  p.stdout.close()
-  p.stderr.close()
-  return p.returncode, stdout, stderr
+@contextmanager
+def _AddToPath(*paths):
+  original_sys_path = sys.path
+  sys.path.extend(paths)
+  try:
+    yield
+  finally:
+    # Restore sys.path to what it was before.
+    sys.path = original_sys_path
 
 
 def VerifyNativeApiHeadersListIsValid(input_api, output_api):
@@ -379,15 +378,15 @@ def CheckNoMixingSources(input_api, gn_files, output_api):
 
 def CheckNoPackageBoundaryViolations(input_api, gn_files, output_api):
   cwd = input_api.PresubmitLocalPath()
-  script_path = os.path.join('tools_webrtc', 'presubmit_checks_lib',
-                             'check_package_boundaries.py')
-  command = [sys.executable, script_path, cwd]
-  command += [os.path.join(cwd, gn_file.LocalPath()) for gn_file in gn_files]
-  returncode, _, stderr = _RunCommand(command, cwd)
-  if returncode:
+  with _AddToPath(input_api.os_path.join(
+      cwd, 'tools_webrtc', 'presubmit_checks_lib')):
+    from check_package_boundaries import CheckPackageBoundaries
+  build_files = [os.path.join(cwd, gn_file.LocalPath()) for gn_file in gn_files]
+  errors = CheckPackageBoundaries(cwd, build_files)[:5]
+  if errors:
     return [output_api.PresubmitError(
-        'There are package boundary violations in the following GN files:\n\n'
-        '%s' % stderr)]
+        'There are package boundary violations in the following GN files:',
+        long_text='\n\n'.join(str(err) for err in errors))]
   return []
 
 def CheckGnChanges(input_api, output_api):
@@ -417,21 +416,16 @@ def CheckUnwantedDependencies(input_api, output_api):
   # We need to wait until we have an input_api object and use this
   # roundabout construct to import checkdeps because this file is
   # eval-ed and thus doesn't have __file__.
-  original_sys_path = sys.path
-  try:
-    checkdeps_path = input_api.os_path.join(input_api.PresubmitLocalPath(),
-                                            'buildtools', 'checkdeps')
-    if not os.path.exists(checkdeps_path):
-      return [output_api.PresubmitError(
-          'Cannot find checkdeps at %s\nHave you run "gclient sync" to '
-          'download Chromium and setup the symlinks?' % checkdeps_path)]
-    sys.path.append(checkdeps_path)
+  checkdeps_path = input_api.os_path.join(input_api.PresubmitLocalPath(),
+                                          'buildtools', 'checkdeps')
+  if not os.path.exists(checkdeps_path):
+    return [output_api.PresubmitError(
+        'Cannot find checkdeps at %s\nHave you run "gclient sync" to '
+        'download all the DEPS entries?' % checkdeps_path)]
+  with _AddToPath(checkdeps_path):
     import checkdeps
     from cpp_checker import CppChecker
     from rules import Rule
-  finally:
-    # Restore sys.path to what it was before.
-    sys.path = original_sys_path
 
   added_includes = []
   for f in input_api.AffectedFiles():
@@ -708,15 +702,10 @@ def CheckOrphanHeaders(input_api, output_api):
   error_msg = """Header file {} is not listed in any GN target.
   Please create a target or add it to an existing one in {}"""
   results = []
-  original_sys_path = sys.path
-  try:
-    sys.path = sys.path + [input_api.os_path.join(
-        input_api.PresubmitLocalPath(), 'tools_webrtc', 'presubmit_checks_lib')]
+  with _AddToPath(input_api.os_path.join(
+      input_api.PresubmitLocalPath(), 'tools_webrtc', 'presubmit_checks_lib')):
     from check_orphan_headers import GetBuildGnPathFromFilePath
     from check_orphan_headers import IsHeaderInBuildGn
-  finally:
-    # Restore sys.path to what it was before.
-    sys.path = original_sys_path
 
   for f in input_api.AffectedSourceFiles(input_api.FilterSourceFile):
     if f.LocalPath().endswith('.h') and f.Action() == 'A':

tools_webrtc/presubmit_checks_lib/check_package_boundaries.py

@@ -9,15 +9,12 @@
 # be found in the AUTHORS file in the root of the source tree.
 
 import argparse
-import logging
+import collections
 import os
 import re
 import sys
 
 
-DISPLAY_LEVEL = 1
-IGNORE_LEVEL = 0
-
 # TARGET_RE matches a GN target, and extracts the target name and the contents.
 TARGET_RE = re.compile(r'\d+\$(?P<indent>\s*)\w+\("(?P<target_name>\w+)"\) {'
                        r'(?P<target_contents>.*?)'
@@ -28,27 +25,16 @@ TARGET_RE = re.compile(r'\d+\$(?P<indent>\s*)\w+\("(?P<target_name>\w+)"\) {'
 SOURCES_RE = re.compile(r'sources \+?= \[(?P<sources>.*?)\]',
                         re.MULTILINE | re.DOTALL)
 
-LOG_FORMAT = '%(message)s'
-ERROR_MESSAGE = ("{}:{} in target '{}':\n"
-                 "  Source file '{}'\n"
-                 "  crosses boundary of package '{}'.\n")
+ERROR_MESSAGE = ("{build_file_path}:{line_number} in target '{target_name}':\n"
+                 "  Source file '{source_file}'\n"
+                 "  crosses boundary of package '{subpackage}'.")
 
 
-class Logger(object):
-  def __init__(self, messages_left=None):
-    self.log_level = DISPLAY_LEVEL
-    self.messages_left = messages_left
-
-  def Log(self, build_file_path, line_number, target_name, source_file,
-          subpackage):
-    if self.messages_left is not None:
-      if not self.messages_left:
-        self.log_level = IGNORE_LEVEL
-      else:
-        self.messages_left -= 1
-    message = ERROR_MESSAGE.format(build_file_path, line_number, target_name,
-                                   source_file, subpackage)
-    logging.log(self.log_level, message)
+class PackageBoundaryViolation(
+    collections.namedtuple('PackageBoundaryViolation',
+        'build_file_path line_number target_name source_file subpackage')):
+  def __str__(self):
+    return ERROR_MESSAGE.format(**self._asdict())
 
 
 def _BuildSubpackagesPattern(packages, query):
@@ -70,12 +56,11 @@ def _ReadFileAndPrependLines(file_path):
                    for line_number, line in enumerate(f, 1))
 
 
-def _CheckBuildFile(build_file_path, packages, logger):
-  """Iterates oven all the targets of the given BUILD.gn file, and verifies that
+def _CheckBuildFile(build_file_path, packages):
+  """Iterates over all the targets of the given BUILD.gn file, and verifies that
   the source files referenced by it don't belong to any of it's subpackages.
-  Returns True if a package boundary violation was found.
+  Returns an iterator over PackageBoundaryViolations for this package.
   """
-  found_violations = False
   package = os.path.dirname(build_file_path)
   subpackages_re = _BuildSubpackagesPattern(packages, package)
 
@@ -90,14 +75,11 @@ def _CheckBuildFile(build_file_path, packages, logger):
         source_file = subpackages_match.group('source_file')
         line_number = subpackages_match.group('line_number')
         if subpackage:
-          found_violations = True
-          logger.Log(build_file_path, line_number, target_name, source_file,
-                     subpackage)
-
-  return found_violations
+          yield PackageBoundaryViolation(build_file_path, line_number,
+                                         target_name, source_file, subpackage)
 
 
-def CheckPackageBoundaries(root_dir, logger, build_files=None):
+def CheckPackageBoundaries(root_dir, build_files=None):
   packages = [root for root, _, files in os.walk(root_dir)
               if 'BUILD.gn' in files]
 
@@ -107,11 +89,13 @@ def CheckPackageBoundaries(root_dir, logger, build_files=None):
   else:
     build_files = [os.path.join(package, 'BUILD.gn') for package in packages]
 
-  return any([_CheckBuildFile(build_file_path, packages, logger)
-              for build_file_path in build_files])
+  messages = []
+  for build_file_path in build_files:
+    messages.extend(_CheckBuildFile(build_file_path, packages))
+  return messages
 
 
-def main():
+def main(argv):
   parser = argparse.ArgumentParser(
       description='Script that checks package boundary violations in GN '
                   'build files.')
@@ -127,14 +111,18 @@ def main():
                       help='If set, the maximum number of violations to be '
                            'displayed.')
 
-  args = parser.parse_args()
+  args = parser.parse_args(argv)
 
-  logging.basicConfig(format=LOG_FORMAT)
-  logging.getLogger().setLevel(DISPLAY_LEVEL)
-  logger = Logger(args.max_messages)
+  messages = CheckPackageBoundaries(args.root_dir, args.build_files)
+  messages = messages[:args.max_messages]
 
-  return CheckPackageBoundaries(args.root_dir, logger, args.build_files)
+  for i, message in enumerate(messages):
+    if i > 0:
+      print
+    print message
+
+  return bool(messages)
 
 
 if __name__ == '__main__':
-  sys.exit(main())
+  sys.exit(main(sys.argv[1:]))

tools_webrtc/presubmit_checks_lib/check_package_boundaries_test.py

@@ -25,54 +25,46 @@ def ReadPylFile(file_path):
     return ast.literal_eval(f.read())
 
 
-class Logger(object):
-  def __init__(self, test_dir):
-    self.messages = []
-    self.test_dir = test_dir
-
-  def Log(self, build_file_path, line_number, target_name, source_file,
-          subpackage):
-    build_file_path = os.path.relpath(build_file_path, self.test_dir)
-    build_file_path = build_file_path.replace(os.path.sep, '/')
-    self.messages.append([build_file_path, line_number, target_name,
-                          source_file, subpackage])
-
-
 class UnitTest(unittest.TestCase):
-  def RunTest(self, test_dir, check_all_build_files=False):
-    logger = Logger(test_dir)
+  def _RunTest(self, test_dir, check_all_build_files=False):
     build_files = [os.path.join(test_dir, 'BUILD.gn')]
     if check_all_build_files:
       build_files = None
-    CheckPackageBoundaries(test_dir, logger, build_files)
+
+    messages = []
+    for violation in CheckPackageBoundaries(test_dir, build_files):
+      build_file_path = os.path.relpath(violation.build_file_path, test_dir)
+      build_file_path = build_file_path.replace(os.path.sep, '/')
+      messages.append(violation._replace(build_file_path=build_file_path))
+
     expected_messages = ReadPylFile(os.path.join(test_dir, 'expected.pyl'))
-    self.assertListEqual(sorted(expected_messages), sorted(logger.messages))
+    self.assertListEqual(sorted(expected_messages), sorted(messages))
 
   def testNoErrors(self):
-    self.RunTest(os.path.join(TESTDATA_DIR, 'no_errors'))
+    self._RunTest(os.path.join(TESTDATA_DIR, 'no_errors'))
 
   def testMultipleErrorsSingleTarget(self):
-    self.RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_single_target'))
+    self._RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_single_target'))
 
   def testMultipleErrorsMultipleTargets(self):
-    self.RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_multiple_targets'))
+    self._RunTest(os.path.join(TESTDATA_DIR,
+                               'multiple_errors_multiple_targets'))
 
   def testCommonPrefix(self):
-    self.RunTest(os.path.join(TESTDATA_DIR, 'common_prefix'))
+    self._RunTest(os.path.join(TESTDATA_DIR, 'common_prefix'))
 
   def testAllBuildFiles(self):
-    self.RunTest(os.path.join(TESTDATA_DIR, 'all_build_files'), True)
+    self._RunTest(os.path.join(TESTDATA_DIR, 'all_build_files'), True)
 
   def testSanitizeFilename(self):
     # The `dangerous_filename` test case contains a directory with '++' in its
     # name. If it's not properly escaped, a regex error would be raised.
-    self.RunTest(os.path.join(TESTDATA_DIR, 'dangerous_filename'), True)
+    self._RunTest(os.path.join(TESTDATA_DIR, 'dangerous_filename'), True)
 
   def testRelativeFilename(self):
     test_dir = os.path.join(TESTDATA_DIR, 'all_build_files')
-    logger = Logger(test_dir)
     with self.assertRaises(AssertionError):
-      CheckPackageBoundaries(test_dir, logger, ["BUILD.gn"])
+      CheckPackageBoundaries(test_dir, ["BUILD.gn"])
 
 
 if __name__ == '__main__':

tools_webrtc/presubmit_checks_lib/testdata/all_build_files/expected.pyl

@@ -1,20 +1,20 @@
-[['subpackage2/BUILD.gn',
+[('subpackage2/BUILD.gn',
   '12',
   'error_2',
   'subsubpackage2/dummy_subsubpackage2.cc',
-  'subsubpackage2'],
- ['subpackage2/BUILD.gn',
+  'subsubpackage2'),
+ ('subpackage2/BUILD.gn',
   '13',
   'error_2',
   'subsubpackage2/dummy_subsubpackage2.h',
-  'subsubpackage2'],
- ['subpackage1/BUILD.gn',
+  'subsubpackage2'),
+ ('subpackage1/BUILD.gn',
   '12',
   'error_1',
   'subsubpackage1/dummy_subsubpackage1.cc',
-  'subsubpackage1'],
- ['subpackage1/BUILD.gn',
+  'subsubpackage1'),
+ ('subpackage1/BUILD.gn',
   '13',
   'error_1',
   'subsubpackage1/dummy_subsubpackage1.h',
-  'subsubpackage1']]
+  'subsubpackage1')]

tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/expected.pyl

@@ -1,5 +1,5 @@
-[["BUILD.gn",
+[("BUILD.gn",
   "13",
   "dummy_target",
   "libc++/dummy_subpackage_file.h",
-  "libc++"]]
+  "libc++")]

tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_multiple_targets/expected.pyl

@@ -1,20 +1,20 @@
-[['BUILD.gn',
+[('BUILD.gn',
   '18',
   'error_1',
   'subpackage1/dummy_subpackage1.cc',
-  'subpackage1'],
- ['BUILD.gn',
+  'subpackage1'),
+ ('BUILD.gn',
   '19',
   'error_1',
   'subpackage1/dummy_subpackage1.h',
-  'subpackage1'],
- ['BUILD.gn',
+  'subpackage1'),
+ ('BUILD.gn',
   '25',
   'error_2',
   'subpackage1/dummy_subpackage2.cc',
-  'subpackage1'],
- ['BUILD.gn',
+  'subpackage1'),
+ ('BUILD.gn',
   '26',
   'error_2',
   'subpackage1/dummy_subpackage2.h',
-  'subpackage1']]
+  'subpackage1')]

tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_single_target/expected.pyl

@@ -1,10 +1,10 @@
-[["BUILD.gn",
+[("BUILD.gn",
   "11",
   "dummy_target",
   "subpackage/dummy_subpackage_file.cc",
-  "subpackage"],
- ["BUILD.gn",
+  "subpackage"),
+ ("BUILD.gn",
   "12",
   "dummy_target",
   "subpackage/dummy_subpackage_file.h",
-  "subpackage"]]
+  "subpackage")]