Implement ability to specify temporarily-allowed dependencies in DEPS

files, and use this ability in a few DEPS files where appropriate.
This has no effect on the normal running of checkdeps; "!"
dependencies are treated just like "+" dependencies when checkdeps is
run on our bots.

An upcoming change will use the new checkdeps.CheckAddedIncludes
function, and will error out if you add a new include that violates a
"-" rule, and show a presubmit warning when you add a new include that
violates a "!" rule (the warning will say something like "We are in
the process of removing dependencies from this directory to that file,
can you avoid adding more?"

While I was in there, fixed path handling so that checkdeps will work
on case-sensitive platforms with paths that include upper-case
characters (e.g. a checkout of Chrome at ~/c/Chrome/src rather than
~/c/chrome/src).

Since the pipes.quote method seems unreliable on Windows (it failed on
my setup), switched to subprocess.list2cmdline which I believe is
stable.

Added a small manual testing mode to checkdeps.  It currently only
verifies the CheckAddedIncludes function.

TBR=jam@chromium.org
BUG=138280

Review URL: https://chromiumcodereview.appspot.com/10805042

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@149163 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/DEPS

@@ -32,24 +32,24 @@ include_rules = [
 
   # TODO(jam): Need to remove all these and use only content/public. BUG=98716
   # DO NOT ADD ANY MORE ITEMS TO THE LIST BELOW!
-  "+content/browser/geolocation/wifi_data_provider_common.h",
+  "!content/browser/geolocation/wifi_data_provider_common.h",
   # DO NOT ADD ANY MORE ITEMS TO THE ABOVE LIST!
 
   "-chrome/browser/ui/views",
   # TODO(tfarina): Remove all these. crbug.com/125846.
   # DO NOT ADD ANY MORE ITEMS TO THE LIST BELOW!
-  "+chrome/browser/ui/views/ash/panel_view_aura.h",
-  "+chrome/browser/ui/views/constrained_window_views.h",
-  "+chrome/browser/ui/views/extensions/extension_view.h",
-  "+chrome/browser/ui/views/frame/browser_view.h",
-  "+chrome/browser/ui/views/location_bar/location_bar_view.h",
-  "+chrome/browser/ui/views/location_bar/location_icon_view.h",
-  "+chrome/browser/ui/views/notifications/balloon_view.h",
-  "+chrome/browser/ui/views/notifications/balloon_view_host.h",
-  "+chrome/browser/ui/views/page_info_bubble_view.h",
-  "+chrome/browser/ui/views/reload_button.h",
-  "+chrome/browser/ui/views/select_file_dialog_extension.h",
-  "+chrome/browser/ui/views/unhandled_keyboard_event_handler.h",
+  "!chrome/browser/ui/views/ash/panel_view_aura.h",
+  "!chrome/browser/ui/views/constrained_window_views.h",
+  "!chrome/browser/ui/views/extensions/extension_view.h",
+  "!chrome/browser/ui/views/frame/browser_view.h",
+  "!chrome/browser/ui/views/location_bar/location_bar_view.h",
+  "!chrome/browser/ui/views/location_bar/location_icon_view.h",
+  "!chrome/browser/ui/views/notifications/balloon_view.h",
+  "!chrome/browser/ui/views/notifications/balloon_view_host.h",
+  "!chrome/browser/ui/views/page_info_bubble_view.h",
+  "!chrome/browser/ui/views/reload_button.h",
+  "!chrome/browser/ui/views/select_file_dialog_extension.h",
+  "!chrome/browser/ui/views/unhandled_keyboard_event_handler.h",
   # DO NOT ADD ANY MORE ITEMS TO THE ABOVE LIST!
 
   # Other libraries.
@@ -70,5 +70,5 @@ include_rules = [
 
   # FIXME: this is used by a browser_test. We need to find a better structure
   # for this include.
-  "+chrome/renderer/visitedlink_slave.h",
+  "!chrome/renderer/visitedlink_slave.h",
 ]

chrome/browser/ui/DEPS

@@ -2,15 +2,15 @@ include_rules = [
  "-chrome/browser/ui/views",
  # TODO(tfarina): Remove all these. crbug.com/125846.
  # DO NOT ADD ANY MORE ITEMS TO THE LIST BELOW!
- "+chrome/browser/ui/views/ash/chrome_shell_delegate.h",
- "+chrome/browser/ui/views/ash/panel_view_aura.h",
- "+chrome/browser/ui/views/find_bar_host.h",
- "+chrome/browser/ui/views/frame/browser_frame.h",
- "+chrome/browser/ui/views/frame/browser_view.h",
- "+chrome/browser/ui/views/frame/browser_non_client_frame_view.h",
- "+chrome/browser/ui/views/sad_tab_view.h",
- "+chrome/browser/ui/views/tab_icon_view.h",
- "+chrome/browser/ui/views/tab_icon_view_model.h",
- "+chrome/browser/ui/views/tab_modal_confirm_dialog_views.h",
+ "!chrome/browser/ui/views/ash/chrome_shell_delegate.h",
+ "!chrome/browser/ui/views/ash/panel_view_aura.h",
+ "!chrome/browser/ui/views/find_bar_host.h",
+ "!chrome/browser/ui/views/frame/browser_frame.h",
+ "!chrome/browser/ui/views/frame/browser_view.h",
+ "!chrome/browser/ui/views/frame/browser_non_client_frame_view.h",
+ "!chrome/browser/ui/views/sad_tab_view.h",
+ "!chrome/browser/ui/views/tab_icon_view.h",
+ "!chrome/browser/ui/views/tab_icon_view_model.h",
+ "!chrome/browser/ui/views/tab_modal_confirm_dialog_views.h",
  # DO NOT ADD ANY MORE ITEMS TO THE ABOVE LIST!
 ]

chrome/test/DEPS

@@ -12,9 +12,9 @@ include_rules = [
   # TODO(joi): Need to remove all of these and use only content/public, see
   # http://crbug.com/98716
   # DO NOT ADD ANY MORE ITEMS TO THE LIST BELOW!
-  "+content/common/debug_flags.h",
-  "+content/common/injection_test_dll.h",
-  "+content/renderer/mock_content_renderer_client.h",
+  "!content/common/debug_flags.h",
+  "!content/common/injection_test_dll.h",
+  "!content/renderer/mock_content_renderer_client.h",
   # DO NOT ADD ANY MORE ITEMS TO THE ABOVE LIST!
 
   "+grit",  # For generated headers

tools/checkdeps/DEPS

+skip_child_includes = [
+  "testdata",
+]

tools/checkdeps/PRESUBMIT.py

+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Presubmit script for checkdeps tool.
+"""
+
+
+def CheckChange(input_api, output_api):
+  results = []
+  results.extend(input_api.canned_checks.RunUnitTests(
+      input_api, output_api,
+      [input_api.os_path.join(input_api.PresubmitLocalPath(),
+                              'checkdeps_test.py')]))
+  return results
+
+
+# Mandatory entrypoint.
+def CheckChangeOnUpload(input_api, output_api):
+  return CheckChange(input_api, output_api)
+
+
+# Mandatory entrypoint.
+def CheckChangeOnCommit(input_api, output_api):
+  return CheckChange(input_api, output_api)

tools/checkdeps/checkdeps_test.py

+#!/usr/bin/env python
+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Tests for checkdeps.
+"""
+
+import os
+import unittest
+
+
+import checkdeps
+
+
+class CheckDepsTest(unittest.TestCase):
+
+  def setUp(self):
+    self.deps_checker = checkdeps.DepsChecker(being_tested=True)
+
+  def testRegularCheckDepsRun(self):
+    problems = self.deps_checker.CheckDirectory(
+        os.path.join(self.deps_checker.base_directory,
+                     'tools/checkdeps/testdata'))
+    self.failUnlessEqual(3, len(problems))
+
+    def VerifySubstringsInProblems(key_path, substrings_in_sequence):
+      found = False
+      key_path = os.path.normpath(key_path)
+      for problem in problems:
+        index = problem.find(key_path)
+        if index != -1:
+          for substring in substrings_in_sequence:
+            index = problem.find(substring, index + 1)
+            self.failUnless(index != -1)
+          found = True
+          break
+      if not found:
+        self.fail('Found no problem for file %s' % key_path)
+
+    VerifySubstringsInProblems('testdata/allowed/test.h',
+                               ['-tools/checkdeps/testdata/disallowed',
+                                '-third_party/explicitly_disallowed',
+                                'Because of no rule applying'])
+    VerifySubstringsInProblems('testdata/disallowed/test.h',
+                               ['-third_party/explicitly_disallowed',
+                                'Because of no rule applying',
+                                'Because of no rule applying'])
+    VerifySubstringsInProblems('disallowed/allowed/test.h',
+                               ['-third_party/explicitly_disallowed',
+                                'Because of no rule applying',
+                                'Because of no rule applying'])
+
+  def testCheckAddedIncludesAllGood(self):
+    problems = self.deps_checker.CheckAddedCppIncludes(
+      [['tools/checkdeps/testdata/allowed/test.cc',
+        ['#include "tools/checkdeps/testdata/allowed/good.h"',
+         '#include "tools/checkdeps/testdata/disallowed/allowed/good.h"']
+      ]])
+    self.failIf(problems)
+
+  def testCheckAddedIncludesManyGarbageLines(self):
+    garbage_lines = ["My name is Sam%d\n" % num for num in range(50)]
+    problems = self.deps_checker.CheckAddedCppIncludes(
+      [['tools/checkdeps/testdata/allowed/test.cc', garbage_lines]])
+    self.failIf(problems)
+
+  def testCheckAddedIncludesNoRule(self):
+    problems = self.deps_checker.CheckAddedCppIncludes(
+      [['tools/checkdeps/testdata/allowed/test.cc',
+        ['#include "no_rule_for_this/nogood.h"']
+      ]])
+    self.failUnless(problems)
+
+  def testCheckAddedIncludesSkippedDirectory(self):
+    problems = self.deps_checker.CheckAddedCppIncludes(
+      [['tools/checkdeps/testdata/disallowed/allowed/skipped/test.cc',
+        ['#include "whatever/whocares.h"']
+      ]])
+    self.failIf(problems)
+
+  def testCheckAddedIncludesTempAllowed(self):
+    problems = self.deps_checker.CheckAddedCppIncludes(
+      [['tools/checkdeps/testdata/allowed/test.cc',
+        ['#include "tools/checkdeps/testdata/disallowed/temporarily_allowed.h"']
+      ]])
+    self.failUnless(problems)
+
+
+if __name__ == '__main__':
+  unittest.main()

tools/checkdeps/cpp_checker.py

@@ -7,6 +7,8 @@
 import codecs
 import re
 
+from rules import Rule
+
 
 class CppChecker(object):
 
@@ -32,38 +34,50 @@ class CppChecker(object):
   def __init__(self, verbose):
     self._verbose = verbose
 
-  def _CheckLine(self, rules, line):
-    """Checks the given file with the given rule set.
-    Returns a tuple (is_include, illegal_description).
+  def CheckLine(self, 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."""
+    If it is also an illegal include, the second value will be a
+    string describing the error.  Otherwise, it will be None.  If
+    fail_on_temp_allow is False, only Rule.DISALLOW rules will cause a
+    problem to be reported.  If it is true, both Rule.DISALLOW and
+    Rule.TEMP_ALLOW will cause an error.
+
+    The last item in the triplet returns the type of rule that
+    applied, one of Rule.ALLOW (which implies the second item is
+    None), Rule.DISALLOW (which implies that the second item is not
+    None) and Rule.TEMP_ALLOW (in which case the second item will be
+    None only if fail_on_temp_allow is False).
+    """
     found_item = self._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)
 
     if '\\' in include_path:
-      return True, 'Include paths may not include backslashes'
+      return True, 'Include paths may not include backslashes', Rule.DISALLOW
 
     if '/' not in include_path:
       # Don't fail when no directory is specified. We may want to be more
       # strict about this in the future.
       if self._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 self._verbose:
         retval = '\nFor %s' % rules
       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, allowed
 
   def CheckFile(self, rules, filepath):
     if self._verbose:
@@ -90,7 +104,7 @@ class CppChecker(object):
             in_if0 -= 1
           continue
 
-        is_include, line_status = self._CheckLine(rules, line)
+        is_include, line_status, rule_type = self.CheckLine(rules, line)
         if is_include:
           last_include = line_num
         if line_status is not None:

tools/checkdeps/java_checker.py

@@ -8,6 +8,8 @@ import codecs
 import os
 import re
 
+from rules import Rule
+
 
 class JavaChecker(object):
   """Import checker for Java files.
@@ -94,7 +96,7 @@ class JavaChecker(object):
           # Convert Windows paths to Unix style, as used in DEPS files.
           include_path = include_path.replace(os.path.sep, '/')
           (allowed, why_failed) = rules.DirAllowed(include_path)
-          if not allowed:
+          if allowed == Rule.DISALLOW:
             if self._verbose:
               result += '\nFor %s' % rules
             result += 'Illegal include: "%s"\n    Because of %s\n' % (

tools/checkdeps/rules.py

+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Base classes to represent dependency rules, used by checkdeps.py"""
+
+
+class Rule(object):
+  """Specifies a single rule for an include, which can be one of
+  ALLOW, DISALLOW and TEMP_ALLOW.
+  """
+
+  # 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, directory, source):
+    self.allow = allow
+    self._dir = directory
+    self._source = source
+
+  def __str__(self):
+    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
+    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 not rule_string:
+    raise Exception('The rule string "%s" is empty\nin %s' %
+                    (rule_string, source))
+
+  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(object):
+  def __init__(self):
+    """Initializes the current rules with an empty rule list."""
+    self._rules = []
+
+  def __str__(self):
+    return 'Rules = [\n%s]' % '\n'.join(' %s' % x for x in self._rules)
+
+  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.
+        why_failed = ""
+        if rule.allow != Rule.ALLOW:
+          why_failed = str(rule)
+        return (rule.allow, why_failed)
+    # No rules apply, fail.
+    return (Rule.DISALLOW, "no rule applying")

tools/checkdeps/testdata/DEPS

+include_rules = [
+  "-tools/checkdeps/testdata/disallowed",
+  "+tools/checkdeps/testdata/allowed",
+  "-third_party/explicitly_disallowed",
+]

tools/checkdeps/testdata/allowed/DEPS

+include_rules = [
+  "+tools/checkdeps/testdata/disallowed/allowed",
+  "!tools/checkdeps/testdata/disallowed/temporarily_allowed.h",
+  "+third_party/allowed_may_use",
+]

tools/checkdeps/testdata/allowed/test.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "tools/checkdeps/testdata/allowed/good.h"
+#include "tools/checkdeps/testdata/disallowed/bad.h"
+#include "tools/checkdeps/testdata/disallowed/allowed/good.h"
+#include "tools/checkdeps/testdata/disallowed/temporarily_allowed.h"
+#include "third_party/explicitly_disallowed/bad.h"
+#include "third_party/allowed_may_use/good.h"
+#include "third_party/no_rule/bad.h"

tools/checkdeps/testdata/disallowed/allowed/DEPS

+skip_child_includes = [
+  "skipped",
+]

tools/checkdeps/testdata/disallowed/allowed/skipped/test.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "whatever/whocares/ok.h"

tools/checkdeps/testdata/disallowed/allowed/test.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "tools/checkdeps/testdata/allowed/good.h"
+// Always allowed to include self and parents.
+#include "tools/checkdeps/testdata/disallowed/good.h"
+#include "tools/checkdeps/testdata/disallowed/allowed/good.h"
+#include "third_party/explicitly_disallowed/bad.h"
+#include "third_party/allowed_may_use/bad.h"
+#include "third_party/no_rule/bad.h"

tools/checkdeps/testdata/disallowed/test.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "tools/checkdeps/testdata/allowed/good.h"
+// Always allowed to include self.
+#include "tools/checkdeps/testdata/disallowed/good.h"
+#include "tools/checkdeps/testdata/disallowed/allowed/good.h"
+#include "third_party/explicitly_disallowed/bad.h"
+// Only allowed for code under allowed/.
+#include "third_party/allowed_may_use/bad.h"
+#include "third_party/no_rule/bad.h"