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 dae44c336da50728f03e8f3a6c5973cf773fc5b6..45873aea240593a7dd8f4af87c9637034546fdb0 100755
--- a/tools/checkdeps/checkdeps.py
+++ b/tools/checkdeps/checkdeps.py
@@ -18,9 +18,14 @@ 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

[jam, 2012/07/20 20:08:49]

it seems that this should be an error for presubmit so that no new instances are
added? btw this is what i did for things like scopedallowio etc

[Jói, 2012/07/20 21:57:39]

In some cases, adding no new includes may be difficult to do in practice until
dependency breaking has been achieved.  I thought a warning would be good to
begin with, it's considerably more than what we have now (which is documentation
in a DEPS file that you may or may not see) without going all the way to a more
draconian rule (that may get overridden anyway by --bypass-hooks).  I'm pretty
much fine either way though.

[jam, 2012/07/20 23:23:34]

I don't feel strongly about this either, it's a personal preference (I would
have done it that way), but since you wrote it, feel free to pick whatever you
prefer.

+by "!".
 
   include_rules = {
     # Code should be able to use base (it's specified in the module-level
@@ -32,7 +37,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,8 +63,8 @@ only lowercase.
 
 import os
 import optparse
-import pipes
 import re
+import subprocess
 import sys
 import copy
 
@@ -81,8 +90,8 @@ VERBOSE = False
 # statements.
 EXTRACT_INCLUDE_PATH = re.compile('[ \t]*#[ \t]*(?:include|import)[ \t]+"(.*)"')
 
-# 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.
@@ -91,15 +100,21 @@ GIT_SOURCE_DIRECTORY = set()
 
 # Specifies a single rule for an include, which can be either allow or disallow.
 class Rule(object):
+
+  # These are the prefixes used to indicate each type of rule. These
+  # are also used as values for self._allow to indicate which type of
+  # rule this is.
+  ALLOW = "+"
+  DISALLOW = "-"
+  TEMP_ALLOW = "!"
+
   def __init__(self, allow, dir, source):
     self._allow = allow
     self._dir = dir
     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)
+    return '"%s%s" from %s.' % (self._allow, self._dir, self._source)
 
   def ParentOrMatch(self, other):
     """Returns true if the input string is an exact match or is a parent
@@ -120,12 +135,12 @@ def ParseRuleString(rule_string, source):
     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)
+  if not rule_string[0] in [Rule.ALLOW, Rule.DISALLOW, Rule.TEMP_ALLOW]:
+    raise Exception(
+      'The rule string "%s" does not begin with a "+", "-" or "!".' %
+      rule_string)
+
+  return (rule_string[0], rule_string[1:])
 
 
 class Rules:
@@ -160,11 +175,12 @@ class Rules:
     for rule in self._rules:
       if rule.ChildOrMatch(allowed_dir):
         # This rule applies.
-        if rule._allow:
-          return (True, "")
-        return (False, rule.__str__())
+        why_failed = ""
+        if rule._allow != Rule.ALLOW:
+          why_failed = rule.__str__()
+        return (rule._allow, why_failed)
     # No rules apply, fail.
-    return (False, "no rule applying")
+    return (Rule.DISALLOW, "no rule applying")
 
 
 def ApplyRules(existing_rules, includes, cur_dir):
@@ -182,10 +198,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(
+      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.
@@ -227,7 +242,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/20 19:57:01]

are you sure?

[Jói, 2012/07/20 20:01:41]

Yes, or Linux (and Mac, I guess) is broken for checkouts that reside at a path
that includes capital letters.

     return (None, [])
 
   # Check the DEPS file in this directory.
@@ -282,15 +297,16 @@ def ShouldCheckFile(file_name):
   return extension in checked_extensions
 
 
-def CheckLine(rules, line):
-  """Checks the given file with the given rule set.
-  Returns a tuple (is_include, illegal_description).
+def CheckLine(rules, line, fail_on_temp_allow=False):
+  """Checks the given line with the given rule set.
+  Returns a triplet (is_include, illegal_description, rule_type).
   If the line is an #include directive the first value will be True.
   If it is also an illegal include, the second value will be a string describing
-  the error.  Otherwise, it will be None."""
+  the error.  Otherwise, it will be None.
+  """
   found_item = EXTRACT_INCLUDE_PATH.match(line)
   if not found_item:
-    return False, None  # Not a match
+    return False, None, Rule.ALLOW  # Not a match
 
   include_path = found_item.group(1)
 
@@ -302,18 +318,19 @@ def CheckLine(rules, line):
     # strict about this in the future.
     if VERBOSE:
       print " WARNING: directory specified with no path: " + include_path
-    return True, None
+    return True, None, Rule.ALLOW
 
   (allowed, why_failed) = rules.DirAllowed(include_path)
-  if not allowed:
+  if (allowed == Rule.DISALLOW or
+      (fail_on_temp_allow and allowed == Rule.TEMP_ALLOW)):
     if VERBOSE:
       retval = "\nFor " + rules.__str__()
     else:
       retval = ""
     return True, retval + ('Illegal include: "%s"\n    Because of %s' %
-        (include_path, why_failed))
+        (include_path, why_failed)), allowed
 
-  return True, None
+  return True, None, Rule.ALLOW
 
 
 def CheckFile(rules, file_name):
@@ -354,7 +371,7 @@ def CheckFile(rules, file_name):
           in_if0 -= 1
         continue
 
-      is_include, line_status = CheckLine(rules, cur_line)
+      is_include, line_status, rule_type = CheckLine(rules, cur_line)
       if is_include:
         last_include = line_num
       if line_status is not None:
@@ -408,6 +425,99 @@ def CheckDirectory(parent_rules, 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]
+
+  problems = []
+  for file_path, include_lines in added_includes:
+    if not ShouldCheckFile(file_path):
+      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 = CheckLine(
+            rules_for_file, line, True)
+        if rule_type != Rule.ALLOW:
+          problems.append((file_path, rule_type, line_status))
+  return problems
+
+
+def TestCheckAddedIncludes():
+  """Run this by giving the argument 'manualtest' to checkdeps."""
+  # 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.
 
@@ -419,7 +529,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.
@@ -430,6 +540,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
@@ -446,17 +573,16 @@ Examples:
 
 
 def checkdeps(options, args):
+  global BASE_DIRECTORY
   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:
@@ -475,19 +601,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)
-
   success = CheckDirectory(base_rules, start_dir)
   if not success:
     print "\nFAILED\n"
@@ -505,7 +618,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__: