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
Change-Id: Ic086cf0984bd96ba429dfdcac7dcce53616eab2d
Reviewed-on: https://chromium-review.googlesource.com/476026
Commit-Queue: Daniel Cheng <[email protected]>
Reviewed-by: Dirk Pranke <[email protected]>
Cr-Commit-Position: refs/heads/master@{#464648}diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index ce91a438..819bf733 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -1079,7 +1079,57 @@
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.
@@ -1090,22 +1140,20 @@
# 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
@@ -1115,7 +1163,7 @@
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())
@@ -1123,14 +1171,11 @@
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 []
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
index 7f3ee91a..fdca995 100755
--- a/PRESUBMIT_test.py
+++ b/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 @@
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):
