Implement ability to specify temporarily-allowed dependencies in DEPS

tools/checkdeps/checkdeps.py

Index: tools/checkdeps/checkdeps.py
diff --git a/tools/checkdeps/checkdeps.py b/tools/checkdeps/checkdeps.py
index 9624812086a0d732db98a5a0e5825d4bad09b739..870e26230a7e68c487768333024385d15d9e5c01 100755
--- a/tools/checkdeps/checkdeps.py
+++ b/tools/checkdeps/checkdeps.py
@@ -18,9 +18,17 @@ gclient. An example would be:
     "base":"http://foo.bar/trunk/base"
   }
 
-DEPS files not in the top-level of a module won't need this. Then you have
-any additional include rules. You can add (using "+") or subtract (using "-")
-from the previously specified rules (including module-level deps).
+DEPS files not in the top-level of a module won't need this. Then you
+have any additional include rules. You can add (using "+") or subtract
+(using "-") from the previously specified rules (including
+module-level deps). You can also specify a path that is allowed for
+now but that we intend to remove, using "!"; this is treated the same
+as "+" when check_deps is run by our bots, but a presubmit step will
+show a warning if you add a new include of a file that is only allowed
+by "!".
+
+Note that for .java files, there is currently no difference between
+"+" and "!", even in the presubmit step.
 
   include_rules = {
     # Code should be able to use base (it's specified in the module-level
@@ -32,7 +40,11 @@ from the previously specified rules (including module-level deps).
 
     # And it can include files from this other directory even though there is
     # no deps rule for it.
-    "+tools/crime_fighter"
+    "+tools/crime_fighter",
+
+    # This dependency is allowed for now but work is ongoing to remove it,
+    # so you shouldn't add further dependencies on it.
+    "!base/evil/ok_for_now.h",
   }
 
 DEPS files may be placed anywhere in the tree. Each one applies to all
@@ -54,12 +66,13 @@ only lowercase.
 
 import os
 import optparse
-import pipes
+import subprocess
 import sys
 import copy
 
 import cpp_checker
 import java_checker
+from rules import Rule, Rules
 
 
 # Variable name used in the DEPS file to add or subtract include files from
@@ -73,92 +86,14 @@ SKIP_SUBDIRS_VAR_NAME = "skip_child_includes"
 # Set to true for more output. This is set by the command line options.
 VERBOSE = False
 
-# In lowercase, using forward slashes as directory separators, ending in a
-# forward slash. Set by the command line options.
+# OS-compatible path for base directory (e.g. C:\chrome\src). Set
+# implicitly or by command-line option.
 BASE_DIRECTORY = ""
 
 # The directories which contain the sources managed by git.
 GIT_SOURCE_DIRECTORY = set()
 
 
-# Specifies a single rule for an include, which can be either allow or disallow.
-class Rule(object):
-  def __init__(self, allow, directory, source):
-    self.allow = allow
-    self._dir = directory
-    self._source = source
-
-  def __str__(self):
-    if (self.allow):
-      return '"+%s" from %s.' % (self._dir, self._source)
-    return '"-%s" from %s.' % (self._dir, self._source)
-
-  def ParentOrMatch(self, other):
-    """Returns true if the input string is an exact match or is a parent
-    of the current rule. For example, the input "foo" would match "foo/bar"."""
-    return self._dir == other or self._dir.startswith(other + "/")
-
-  def ChildOrMatch(self, other):
-    """Returns true if the input string would be covered by this rule. For
-    example, the input "foo/bar" would match the rule "foo"."""
-    return self._dir == other or other.startswith(self._dir + "/")
-
-
-def ParseRuleString(rule_string, source):
-  """Returns a tuple of a boolean indicating whether the directory is an allow
-  rule, and a string holding the directory name.
-  """
-  if len(rule_string) < 1:
-    raise Exception('The rule string "%s" is too short\nin %s' %
-                    (rule_string, source))
-
-  if rule_string[0] == "+":
-    return (True, rule_string[1:])
-  if rule_string[0] == "-":
-    return (False, rule_string[1:])
-  raise Exception('The rule string "%s" does not begin with a "+" or a "-"' %
-                  rule_string)
-
-
-class Rules:
-  def __init__(self):
-    """Initializes the current rules with an empty rule list."""
-    self._rules = []
-
-  def __str__(self):
-    ret = "Rules = [\n"
-    ret += "\n".join([" %s" % x for x in self._rules])
-    ret += "]\n"
-    return ret
-
-  def AddRule(self, rule_string, source):
-    """Adds a rule for the given rule string.
-
-    Args:
-      rule_string: The include_rule string read from the DEPS file to apply.
-      source: A string representing the location of that string (filename, etc.)
-              so that we can give meaningful errors.
-    """
-    (add_rule, rule_dir) = ParseRuleString(rule_string, source)
-    # Remove any existing rules or sub-rules that apply. For example, if we're
-    # passed "foo", we should remove "foo", "foo/bar", but not "foobar".
-    self._rules = [x for x in self._rules if not x.ParentOrMatch(rule_dir)]
-    self._rules.insert(0, Rule(add_rule, rule_dir, source))
-
-  def DirAllowed(self, allowed_dir):
-    """Returns a tuple (success, message), where success indicates if the given
-    directory is allowed given the current set of rules, and the message tells
-    why if the comparison failed."""
-    for rule in self._rules:
-      if rule.ChildOrMatch(allowed_dir):
-        # This rule applies.
-        if rule.allow:
-          return (True, "")
-        return (False, rule.__str__())
-    # No rules apply, fail.
-    return (False, "no rule applying")
-
-
 def ApplyRules(existing_rules, includes, cur_dir):
   """Applies the given include rules, returning the new rules.
 
@@ -174,10 +109,9 @@ def ApplyRules(existing_rules, includes, cur_dir):
   rules = copy.copy(existing_rules)
 
   # First apply the implicit "allow" rule for the current directory.
-  if cur_dir.lower().startswith(BASE_DIRECTORY):
+  if os.path.normcase(os.path.normpath(cur_dir)).startswith(

[M-A Ruel, 2012/07/26 13:06:03]

normcase is totally useless:
http://docs.python.org/library/os.path.html#os.path.normcase

I implemented the whole thing but it's "complicated". See the 3
get_native_path_case() implementations in:
http://src.chromium.org/viewvc/chrome/trunk/src/tools/isolate/trace_inputs.py...

This function is unit tested to work on each OS we care about. But that's
totally overkill in your case. The reason is simple: we do not allow in the
source tree 2 files in a directory where file1.lower() == file2.lower(). As
such, you can reduce the check to:

if os.path.normpath(cur_dir).lower().startswith(
    os.path.normpath(BASE_DIRECTORY).lower()):

[Jói, 2012/07/26 13:32:13]

Done.

+      os.path.normcase(os.path.normpath(BASE_DIRECTORY))):
     relative_dir = cur_dir[len(BASE_DIRECTORY) + 1:]
-    # Normalize path separators to slashes.
-    relative_dir = relative_dir.replace("\\", "/")
     source = relative_dir
     if len(source) == 0:
       source = "top level"  # Make the help string a little more meaningful.
@@ -219,7 +153,7 @@ def ApplyDirectoryRules(existing_rules, dir_name):
   # contained in git source direcotries. This will tell us if it's a source
   # directory and should be checked.
   if not (os.path.exists(os.path.join(dir_name, ".svn")) or
-          (dir_name.lower() in GIT_SOURCE_DIRECTORY)):
+          (dir_name in GIT_SOURCE_DIRECTORY)):

[M-A Ruel, 2012/07/26 13:06:03]

Same here:
any(dir_name.lower() == i.lower() for i in GIT_SOURCE_DIRECTORY)

[Jói, 2012/07/26 13:32:13]

Done, by making the paths lower-case when adding to GIT_SOURCE_DIRECTORY and
checking dir_name.lower() in GIT_SOURCE_DIRECTORY here.

     return (None, [])
 
   # Check the DEPS file in this directory.
@@ -297,6 +231,102 @@ def CheckDirectory(parent_rules, checkers, dir_name):
   return success
 
 
+def CheckAddedIncludes(added_includes):
+  """This is used from PRESUBMIT.py to check new #include statements added in
+  the change being presubmit checked.
+
+  Args:
+    added_includes: ((file_path, (include_line, include_line, ...), ...)
+
+  Return:
+    A list of tuples, (bad_file_path, rule_type, rule_description)
+    where rule_type is one of Rule.DISALLOW or Rule.TEMP_ALLOW and
+    rule_description is human-readable. Empty if no problems.
+  """
+  CommonSetup()
+
+  # Map of directory paths to rules to use for those directories, or
+  # None for directories that should be skipped.
+  directory_rules = {}
+
+  def ApplyDirectoryRulesAndSkipSubdirs(parent_rules, dir_path):
+    rules_tuple = ApplyDirectoryRules(parent_rules, dir_path)
+    directory_rules[dir_path] = rules_tuple[0]
+    for subdir in rules_tuple[1]:
+      directory_rules[os.path.join(dir_path, subdir)] = None
+
+  ApplyDirectoryRulesAndSkipSubdirs(Rules(), BASE_DIRECTORY)
+
+  def GetDirectoryRules(dir_path):
+    """Returns a Rules object to use for the given directory, or None
+    if the given directory should be skipped.
+    """
+    if not dir_path.startswith(BASE_DIRECTORY):
+      dir_path = os.path.join(BASE_DIRECTORY, dir_path)
+
+    parent_dir = os.path.dirname(dir_path)
+    parent_rules = None
+    if not dir_path in directory_rules:
+      parent_rules = GetDirectoryRules(parent_dir)
+
+    # We need to check for an entry for our dir_path again, in case we
+    # are at a path e.g. A/B/C where A/B/DEPS specifies the C
+    # subdirectory to be skipped; in this case, the invocation to
+    # GetDirectoryRules(parent_dir) has already filled in an entry for
+    # A/B/C.
+    if not dir_path in directory_rules:
+      if not parent_rules:
+        # If the parent directory should be skipped, then the current
+        # directory should also be skipped.
+        directory_rules[dir_path] = None
+      else:
+        ApplyDirectoryRulesAndSkipSubdirs(parent_rules, dir_path)
+    return directory_rules[dir_path]
+
+  cpp = cpp_checker.CppChecker(VERBOSE)
+
+  problems = []
+  for file_path, include_lines in added_includes:
+    # TODO(joi): Make this cover Java as well.
+    if not os.path.splitext(file_path)[1] in cpp.EXTENSIONS:
+      pass
+    rules_for_file = GetDirectoryRules(os.path.dirname(file_path))
+    if rules_for_file:
+      for line in include_lines:
+        is_include, line_status, rule_type = cpp.CheckLine(
+            rules_for_file, line, True)
+        if rule_type != Rule.ALLOW:
+          problems.append((file_path, rule_type, line_status))
+  return problems
+
+
+def TestCheckAddedIncludes():

[M-A Ruel, 2012/07/26 13:06:03]

This function should be in a checkdeps_test.py and then calls from a
corresponding PRESUBMIT.py in the directory.

[Jói, 2012/07/26 13:32:13]

Made it a separate checkdeps_manualtest.py file (manualtest since it requires
manual validation of results, and may require updates to the test data based on
updates to checked-in files in the main Chrome source tree).  I don't think it's
right to put this test in a presubmit step because of the manual nature of
validation.

+  """Run this by giving the argument 'manualtest' to checkdeps."""

[M-A Ruel, 2012/07/26 13:06:03]

Runs this

What is "this" ?

[Jói, 2012/07/26 13:32:13]

Removed this comment, it is meaningless in the new location of the test.

+  # Note that these tests depend on the rules currently in DEPS files
+  # checked into the tree, which may change.  Therefore the test is
+  # only run manually and may need to be updated from time to time.
+  problems = CheckAddedIncludes(
+    # The first of these should fail as a DISALLOW, the second should
+    # fail as a TEMP_ALLOW, because of rules in chrome/browser/DEPS.
+    [['chrome/browser/bingo.cc',
+      ['#include "chrome/browser/ui/views/boondog.h"',
+       '#include "chrome/browser/ui/views/ash/panel_view_aura.h"']],
+     # This should fail because of no rule applying.
+     ['chrome/browser/lifetime/bongo.cc', ['#include "fantastic/food.h"']],
+     # The following two shouldn't fail as they are under a directory
+     # that is skipped for checking, in src/DEPS.
+     ['breakpad/fooblat.cc', ['#include "fantastic/food.h"']],
+     ['breakpad/bingo/bongo/fooblat.cc', ['#include "fantastic/food.h"']],
+   ])
+
+  for problem in problems:
+    print problem[0]
+    print problem[1]
+    print problem[2]
+    print
+    print
+
+
 def GetGitSourceDirectory(root):
   """Returns a set of the directories to be checked.
 
@@ -308,7 +338,7 @@ def GetGitSourceDirectory(root):
   """
   git_source_directory = set()
   popen_out = os.popen("cd %s && git ls-files --full-name ." %
-                       pipes.quote(root))
+                       subprocess.list2cmdline([root]))
   for line in popen_out.readlines():
     dir_name = os.path.join(root, os.path.dirname(line))
     # Add the directory as well as all the parent directories.
@@ -319,6 +349,23 @@ def GetGitSourceDirectory(root):
   return git_source_directory
 
 
+def CommonSetup(base_dir=None):
+  """Setup that is in common between the main() entrypoint to this
+  script and the CheckAddedIncludes (presubmit check) entrypoint.
+  """
+  global BASE_DIRECTORY
+  global GIT_SOURCE_DIRECTORY
+
+  if base_dir:
+    BASE_DIRECTORY = base_dir
+  else:
+    BASE_DIRECTORY = os.path.abspath(
+        os.path.join(os.path.abspath(os.path.dirname(__file__)), "../.."))
+
+  if os.path.exists(os.path.join(BASE_DIRECTORY, ".git")):
+    GIT_SOURCE_DIRECTORY = GetGitSourceDirectory(BASE_DIRECTORY)
+
+
 def PrintUsage():
   print """Usage: python checkdeps.py [--root <root>] [tocheck]
   --root   Specifies the repository root. This defaults to "../../.." relative
@@ -335,17 +382,16 @@ Examples:
 
 
 def checkdeps(options, args):
+  global BASE_DIRECTORY

[M-A Ruel, 2012/07/26 13:06:03]

It'd be simpler if it was not a global. Same for GIT_SOURCE_DIRECTORY, globals
really complicate importing this file.

[Jói, 2012/07/26 13:32:13]

I'm not sure I follow, I get that it's harder to use checkdeps as a module
because of the globals if you want to invoke multiple times on it, but it seems
fine for our current purposes?  Maybe I'm missing some complexity about
importing a module with globals that I'm unaware of?  They are really just
module scoped, not truly global, as far as I understand.

[M-A Ruel, 2012/07/26 13:50:51]

global as in module global, since this is python.

The problem is that CommonSetup() makes the calls implicitly stateful. So, if
two different PRESUBMIT.py imports checkdeps, the second one could (or could
not, depends if the module reference is leaked) reuse the state from the first
one.

This can be problematic for example if 2 presubmits are run for multiple
checkouts, since GIT_SOURCE_DIRECTORY will be wrong on the second one. This may
happen with complex gcl changes, but is rare with git cl.

Overall, this is a potential for Major Fun. This is why I first recommended for
shelling out instead of importing. I'd recommend to do one or the other:
1. shell out in PRESUBMIT
2. Get rid of BASE_DIRECTORY and GIT_SOURCE_DIRECTORY and pass them as
parameters. VERBOSE is borderline in that case, probably a good idea to do it
too.

[Jói, 2012/07/26 17:05:52]

Done, I made these members of a checkdeps.DepsChecker object that you
instantiate.

   global VERBOSE
   if options.verbose:
     VERBOSE = True
 
   # Optional base directory of the repository.
-  global BASE_DIRECTORY
   if not options.base_directory:
-    BASE_DIRECTORY = os.path.abspath(
-        os.path.join(os.path.abspath(os.path.dirname(__file__)), "../.."))
+    CommonSetup()
   else:
-    BASE_DIRECTORY = os.path.abspath(options.base_directory)
+    CommonSetup(os.path.abspath(options.base_directory))
 
   # Figure out which directory we have to check.
   if len(args) == 0:
@@ -364,19 +410,6 @@ def checkdeps(options, args):
   print "Checking:", start_dir
 
   base_rules = Rules()
-
-  # The base directory should be lower case from here on since it will be used
-  # for substring matching on the includes, and we compile on case-insensitive
-  # systems. Plus, we always use slashes here since the include parsing code
-  # will also normalize to slashes.
-  BASE_DIRECTORY = BASE_DIRECTORY.lower()
-  BASE_DIRECTORY = BASE_DIRECTORY.replace("\\", "/")
-  start_dir = start_dir.replace("\\", "/")
-
-  if os.path.exists(os.path.join(BASE_DIRECTORY, ".git")):
-    global GIT_SOURCE_DIRECTORY
-    GIT_SOURCE_DIRECTORY = GetGitSourceDirectory(BASE_DIRECTORY)
-
   java = java_checker.JavaChecker(BASE_DIRECTORY, VERBOSE)
   cpp = cpp_checker.CppChecker(VERBOSE)
   checkers = dict(
@@ -399,7 +432,10 @@ def main():
   option_parser.add_option("-v", "--verbose", action="store_true",
                            default=False, help="Print debug logging")
   options, args = option_parser.parse_args()
-  return checkdeps(options, args)
+  if args and args[0] == "manualtest":
+    return TestCheckAddedIncludes()
+  else:
+    return checkdeps(options, args)
 
 
 if '__main__' == __name__: