Don't require DEPS OWNERS when moving lines around in a DEPS file.

As a bonus, less regex than before and also correctly handles the
'!' prefix in DEPS files now.

BUG=702851
R=dpranke@chromium.org

Review-Url: https://codereview.chromium.org/2768063004
Cr-Commit-Position: refs/heads/master@{#461266}

PRESUBMIT.py

@@ -1060,7 +1060,57 @@ def _CheckNoAbbreviationInPngFileName(input_api, output_api):
   return results
 
 
-def _FilesToCheckForIncomingDeps(re, changed_lines):
+def _ExtractAddRulesFromParsedDeps(parsed_deps):
+  """Extract the rules that add dependencies from a parsed DEPS file.
+
+  Args:
+    parsed_deps: the locals dictionary from evaluating the DEPS file."""
+  add_rules = set()
+  add_rules.update([
+      rule[1:] for rule in parsed_deps.get('include_rules', [])
+      if rule.startswith('+') or rule.startswith('!')
+  ])
+  for specific_file, rules in parsed_deps.get('specific_include_rules',
+                                              {}).iteritems():
+    add_rules.update([
+        rule[1:] for rule in rules
+        if rule.startswith('+') or rule.startswith('!')
+    ])
+  return add_rules
+
+
+def _ParseDeps(contents):
+  """Simple helper for parsing DEPS files."""
+  # Stubs for handling special syntax in the root DEPS file.
+  def FromImpl(*_):
+    pass  # NOP function so "From" doesn't fail.
+
+  def FileImpl(_):
+    pass  # NOP function so "File" doesn't fail.
+
+  class _VarImpl:
+
+    def __init__(self, local_scope):
+      self._local_scope = local_scope
+
+    def Lookup(self, var_name):
+      """Implements the Var syntax."""
+      try:
+        return self._local_scope['vars'][var_name]
+      except KeyError:
+        raise Exception('Var is not defined: %s' % var_name)
+
+  local_scope = {}
+  global_scope = {
+      'File': FileImpl,
+      'From': FromImpl,
+      'Var': _VarImpl(local_scope).Lookup,
+  }
+  exec contents in global_scope, local_scope
+  return local_scope
+
+
+def _CalculateAddedDeps(os_path, old_contents, new_contents):
   """Helper method for _CheckAddedDepsHaveTargetApprovals. Returns
   a set of DEPS entries that we should look up.
 
@@ -1071,22 +1121,20 @@ def _FilesToCheckForIncomingDeps(re, changed_lines):
   # We ignore deps entries on auto-generated directories.
   AUTO_GENERATED_DIRS = ['grit', 'jni']
 
-  # This pattern grabs the path without basename in the first
-  # parentheses, and the basename (if present) in the second. It
-  # relies on the simple heuristic that if there is a basename it will
-  # be a header file ending in ".h".
-  pattern = re.compile(
-      r"""['"]\+([^'"]+?)(/[a-zA-Z0-9_]+\.h)?['"].*""")
+  old_deps = _ExtractAddRulesFromParsedDeps(_ParseDeps(old_contents))
+  new_deps = _ExtractAddRulesFromParsedDeps(_ParseDeps(new_contents))
+
+  added_deps = new_deps.difference(old_deps)
+
   results = set()
-  for changed_line in changed_lines:
-    m = pattern.match(changed_line)
-    if m:
-      path = m.group(1)
-      if path.split('/')[0] not in AUTO_GENERATED_DIRS:
-        if m.group(2):
-          results.add('%s%s' % (path, m.group(2)))
-        else:
-          results.add('%s/DEPS' % path)
+  for added_dep in added_deps:
+    if added_dep.split('/')[0] in AUTO_GENERATED_DIRS:
+      continue
+    # Assume that a rule that ends in .h is a rule for a specific file.
+    if added_dep.endswith('.h'):
+      results.add(added_dep)
+    else:
+      results.add(os_path.join(added_dep, 'DEPS'))
   return results
 
 
@@ -1096,22 +1144,19 @@ def _CheckAddedDepsHaveTargetApprovals(input_api, output_api):
   target file or directory, to avoid layering violations from being
   introduced. This check verifies that this happens.
   """
-  changed_lines = set()
+  virtual_depended_on_files = set()
 
   file_filter = lambda f: not input_api.re.match(
       r"^third_party[\\\/]WebKit[\\\/].*", f.LocalPath())
   for f in input_api.AffectedFiles(include_deletes=False,
                                    file_filter=file_filter):
     filename = input_api.os_path.basename(f.LocalPath())
     if filename == 'DEPS':
-      changed_lines |= set(line.strip()
-                           for line_num, line
-                           in f.ChangedContents())
-  if not changed_lines:
-    return []
+      virtual_depended_on_files.update(_CalculateAddedDeps(
+          input_api.os_path,
+          '\n'.join(f.OldContents()),
+          '\n'.join(f.NewContents())))
 
-  virtual_depended_on_files = _FilesToCheckForIncomingDeps(input_api.re,
-                                                           changed_lines)
   if not virtual_depended_on_files:
     return []
 

PRESUBMIT_test.py

@@ -3,6 +3,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import os.path
 import re
 import subprocess
 import unittest
@@ -469,41 +470,77 @@ def testValidIfDefinedMacroNames(self):
 
 
 class CheckAddedDepsHaveTetsApprovalsTest(unittest.TestCase):
-  def testFilesToCheckForIncomingDeps(self):
-    changed_lines = [
-      '"+breakpad",',
-      '"+chrome/installer",',
-      '"+chrome/plugin/chrome_content_plugin_client.h",',
-      '"+chrome/utility/chrome_content_utility_client.h",',
-      '"+chromeos/chromeos_paths.h",',
-      '"+components/crash/content",',
-      '"+components/nacl/common",',
-      '"+content/public/browser/render_process_host.h",',
-      '"+jni/fooblat.h",',
-      '"+grit",  # For generated headers',
-      '"+grit/generated_resources.h",',
-      '"+grit/",',
-      '"+policy",  # For generated headers and source',
-      '"+sandbox",',
-      '"+tools/memory_watcher",',
-      '"+third_party/lss/linux_syscall_support.h",',
+
+  def calculate(self, old_include_rules, old_specific_include_rules,
+                new_include_rules, new_specific_include_rules):
+    return PRESUBMIT._CalculateAddedDeps(
+        os.path, 'include_rules = %r\nspecific_include_rules = %r' % (
+            old_include_rules, old_specific_include_rules),
+        'include_rules = %r\nspecific_include_rules = %r' % (
+            new_include_rules, new_specific_include_rules))
+
+  def testCalculateAddedDeps(self):
+    old_include_rules = [
+        '+base',
+        '-chrome',
+        '+content',
+        '-grit',
+        '-grit/",',
+        '+jni/fooblat.h',
+        '!sandbox',
     ]
-    files_to_check = PRESUBMIT._FilesToCheckForIncomingDeps(re, changed_lines)
+    old_specific_include_rules = {
+        'compositor\.*': {
+            '+cc',
+        },
+    }
+
+    new_include_rules = [
+        '-ash',
+        '+base',
+        '+chrome',
+        '+components',
+        '+content',
+        '+grit',
+        '+grit/generated_resources.h",',
+        '+grit/",',
+        '+jni/fooblat.h',
+        '+policy',
+        '+third_party/WebKit',
+    ]
+    new_specific_include_rules = {
+        'compositor\.*': {
+            '+cc',
+        },
+        'widget\.*': {
+            '+gpu',
+        },
+    }
+
     expected = set([
-      'breakpad/DEPS',
-      'chrome/installer/DEPS',
-      'chrome/plugin/chrome_content_plugin_client.h',
-      'chrome/utility/chrome_content_utility_client.h',
-      'chromeos/chromeos_paths.h',
-      'components/crash/content/DEPS',
-      'components/nacl/common/DEPS',
-      'content/public/browser/render_process_host.h',
-      'policy/DEPS',
-      'sandbox/DEPS',
-      'tools/memory_watcher/DEPS',
-      'third_party/lss/linux_syscall_support.h',
+        'chrome/DEPS',
+        'gpu/DEPS',
+        'components/DEPS',
+        'policy/DEPS',
+        'third_party/WebKit/DEPS',
     ])
-    self.assertEqual(expected, files_to_check);
+    self.assertEqual(
+        expected,
+        self.calculate(old_include_rules, old_specific_include_rules,
+                       new_include_rules, new_specific_include_rules))
+
+  def testCalculateAddedDepsIgnoresPermutations(self):
+    old_include_rules = [
+        '+base',
+        '+chrome',
+    ]
+    new_include_rules = [
+        '+chrome',
+        '+base',
+    ]
+    self.assertEqual(set(),
+                     self.calculate(old_include_rules, {}, new_include_rules,
+                                    {}))
 
 
 class JSONParsingTest(unittest.TestCase):