Add presubmit check to prevent package boundary violations.

tools-webrtc/check_package_boundaries.py

Index: tools-webrtc/check_package_boundaries.py
diff --git a/tools-webrtc/check_package_boundaries.py b/tools-webrtc/check_package_boundaries.py
new file mode 100644
index 0000000000000000000000000000000000000000..3d93ad4d80d6c2f06cead3a9a2fb57bc52e74161
--- /dev/null
+++ b/tools-webrtc/check_package_boundaries.py
@@ -0,0 +1,144 @@
+#!/usr/bin/env python
+
+# Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+#
+# Use of this source code is governed by a BSD-style license
+# that can be found in the LICENSE file in the root of the source
+# tree. An additional intellectual property rights grant can be found
+# in the file PATENTS.  All contributing project authors may
+# be found in the AUTHORS file in the root of the source tree.
+
+import argparse
+import logging
+import os
+import sys
+import re

[kjellander_webrtc, 2017/01/16 07:26:10]

sort

[ehmaldonado_webrtc, 2017/01/16 08:52:50]

Acknowledged.

+
+from collections import defaultdict
+
+
+DISPLAY_LEVEL = 1
+IGNORE_LEVEL = 0
+
+TARGET_RE = re.compile(r'\s*\w*\("(\w*)"\) {$')
+SOURCES_RE = re.compile(r'\s*sources \+?= \[$')
+SOURCE_FILE_RE = re.compile(r'\s*"([\w\./]*)",$')
+INLINE_SOURCES_RE = re.compile(r'\s*sources \+?= \[ "([\w\./]*)" ]$')
+
+LOG_FORMAT = '%(message)s'
+ERROR_MESSAGE = ("{}:{} in target '{}'\n"
+                 "\tSource file '{}'\n"
+                 "\tCrosses boundary of package '{}'.")
+
+
+class Logger(object):
+  def __init__(self, messagesLeft=None):
+    self.logLevel = DISPLAY_LEVEL

[kjellander_webrtc, 2017/01/16 07:26:10]

The style guide says local variables and class/global variables should be named
lowercase with underscores. The Chromium exception with CapsWords style only
applies to function and method names:
http://dev.chromium.org/chromium-os/python-style-guidelines

I realize style violations like that aren't checked by PyLint, which is
unfortunate, but AFAIK there's no check for that available either (we're already
using stricter checks than Chromium by our
https://chromium.googlesource.com/external/webrtc/+/master/pylintrc file - maybe
it's possible to tighten it up further?).

[ehmaldonado_webrtc, 2017/01/16 08:52:51]

Acknowledged.

+    self.messagesLeft = messagesLeft
+
+  def log(self, message):
+    if self.messagesLeft is not None:
+      if not self.messagesLeft:
+        self.logLevel = IGNORE_LEVEL
+      else:
+        self.messagesLeft -= 1
+    logging.log(self.logLevel, message)
+
+
+class RegexAux(object):

[kjellander_webrtc, 2017/01/16 07:26:10]

I'm a bit confused by the name Aux, is RegexUtil better? Since AFAICS this class
only exists to make the code more readable?

[ehmaldonado_webrtc, 2017/01/16 08:52:51]

Acknowledged.

+  def __init__(self):
+    self.match = None
+
+  def matches(self, regex, target):

[kjellander_webrtc, 2017/01/16 07:26:10]

Rename target->line to avoid confusion here since we're dealing with GN targets
in this script.

[ehmaldonado_webrtc, 2017/01/16 08:52:51]

Acknowledged.

+    match_obj = regex.match(target)
+    if match_obj and match_obj.groups():
+      self.match = match_obj.groups()[0]
+    return match_obj
+
+
+def _BuildTrie(sequences):
+  trie = defaultdict(list)
+  for sequence in sequences:
+    if sequence:
+      head = sequence.pop()
+      trie[head].append(sequence)
+  return dict((k, _BuildTrie(v)) for k, v in trie.items())
+
+def _LongestPrefix(trie, query):

[kjellander_webrtc, 2017/01/16 07:26:10]

+1 blank line for top level methods

[ehmaldonado_webrtc, 2017/01/16 08:52:50]

Acknowledged.

+  if not query:
+    return []
+  head = query.pop()
+  if head not in trie:
+    return []
+  return [head] + _LongestPrefix(trie[head], query)
+
+def _CheckBuildFile(trie, buildFilePath, logger):
+  targetName = None
+  foundErrors = False
+  processingSourceFiles = False
+
+  regexAux = RegexAux()
+  package = os.path.dirname(buildFilePath)
+
+  with open(buildFilePath) as buildFile:
+    for lineNumber, line in enumerate(buildFile):
+      sourceFile = None
+
+      if regexAux.matches(TARGET_RE, line):

[kjellander_webrtc, 2017/01/16 07:26:10]

I find it hard to follow what's going on here since there's a lot of state etc.
Is it not possible to solve this problem easier with multiline-regexp, like in
_CheckNoSourcesAbove in
https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#231? 

Either way, I'd like to see unit tests to prove that this code works for a few
simple cases. For this I think you need to build a testdata dir that looks
something like this:
testdata/BIULD.gn - with two targets: one that has no sources and one that has
one: dummy.cc, which depends on subdir/BUILD.gn
tesdata/dummy.cc - includes subdir/subdir2/dummy3.h (package boundary violation)
testdata/subdir/BUILD.gn - with one target that includes dummy2.{h,cc}
testdata/subdir/dummy2.cc
testdata/subdir/dummy2.h
testdata/subdir/subdir2/dummy3.h

[ehmaldonado_webrtc, 2017/01/16 08:52:51]

I'll try to make this clearer.
You're right, this is missing test cases :)

+        targetName = regexAux.match
+      elif regexAux.matches(SOURCES_RE, line):
+        processingSourceFiles = True
+      elif (regexAux.matches(INLINE_SOURCES_RE, line) or
+            (processingSourceFiles and regexAux.matches(SOURCE_FILE_RE, line))):
+        sourceFile = regexAux.match
+      else:
+        processingSourceFiles = False
+
+      if not sourceFile:
+        continue
+
+      query = os.path.join(package, sourceFile).split(os.path.sep)[::-1]
+      realPackage = os.path.sep.join(_LongestPrefix(trie, query))
+      if realPackage != package:
+        foundErrors = True
+        logger.log(ERROR_MESSAGE.format(
+            buildFilePath, lineNumber, targetName, sourceFile, realPackage))
+
+  return foundErrors
+
+
+def main():
+  parser = argparse.ArgumentParser(
+      description='Script that checks package boundary violations in GN '
+                  'build files.')
+
+  parser.add_argument('root_dir', metavar='ROOT_DIR',
+                      help='The root directory that contains all BUILD.gn '
+                           'files to be processed.')
+  parser.add_argument('build_files', metavar='BUILD_FILE', nargs='*',
+                      help='A list of BUILD.gn files to be processed. If no '
+                           'files are given, all BUILD.gn files under ROOT_DIR '
+                           'will be processed.')
+  parser.add_argument('--max_messages', type=int, default=None,
+                      help='If set, the maximum number of violations to be '

[kjellander_webrtc, 2017/01/16 07:26:10]

Does this exist only as a performance optimization, i.e. to fail fast when a
violation is found? Is it not better to return all errors to the user so one
doesn't have to rerun presubmit checks many times?

[ehmaldonado_webrtc, 2017/01/16 08:52:50]

No, it's not for performance optimization.
If you don't pass the argument, then all errors will be shown. I just find it
useful to focus on a few of them at a time.

+                           'displayed.')
+
+  args = parser.parse_args()
+
+  logging.basicConfig(format=LOG_FORMAT)
+  logging.getLogger().setLevel(DISPLAY_LEVEL)
+  logger = Logger(args.max_messages)
+
+  packages = [root for root, _, files in os.walk(args.root_dir)
+              if 'BUILD.gn' in files]
+  trie = _BuildTrie(package.split(os.path.sep)[::-1]
+                    for package in packages)
+  defaultBuildFiles = [os.path.join(package, 'BUILD.gn')
+                       for package in packages]
+
+  buildFiles = args.build_files or defaultBuildFiles
+  return any(_CheckBuildFile(trie, buildFilePath, logger)
+             for buildFilePath in buildFiles)
+
+if __name__ == '__main__':
+  sys.exit(main())