From da9479f3d2ff217723adc834581d41671c18d06c Mon Sep 17 00:00:00 2001 From: dcheng Date: Fri, 31 Mar 2017 16:09:37 -0700 Subject: [PATCH] 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 | 93 +++++++++++++++++++++++++++++++----------- PRESUBMIT_test.py | 101 +++++++++++++++++++++++++++++++--------------- 2 files changed, 138 insertions(+), 56 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 344175b25e9ca5..cdb330ee68f6c8 100644 --- a/PRESUBMIT.py +++ b/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,7 +1144,7 @@ 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()) @@ -1104,14 +1152,11 @@ def _CheckAddedDepsHaveTargetApprovals(input_api, output_api): 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 7f3ee91a4bbdbd..fdca995486edb6 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 @@ 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):