diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 084301d86643b5..76d217c78f8b69 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -3450,6 +3450,32 @@ def Lookup(self, var_name): return local_scope +def _FindAllDepsFilesForSubpath(input_api, subpath): + ret = [] + while subpath: + cur = input_api.os_path.join(input_api.change.RepositoryRoot(), subpath, 'DEPS') + if input_api.os_path.exists(cur): + ret.append(cur) + subpath = input_api.os_path.dirname(subpath) + return ret + + +def _FindAddedDepsThatRequireReview(input_api, depended_on_paths): + """Filters to those whose DEPS set new_usages_require_review=True""" + ret = set() + cache = {} + for target_path in depended_on_paths: + for subpath in _FindAllDepsFilesForSubpath(input_api, target_path): + config = cache.get(subpath) + if config is None: + config = _ParseDeps(input_api.ReadFile(subpath)) + cache[subpath] = config + if config.get('new_usages_require_review'): + ret.add(target_path) + break + return ret + + def _CalculateAddedDeps(os_path, old_contents, new_contents): """Helper method for CheckAddedDepsHaveTargetApprovals. Returns a set of DEPS entries that we should look up. @@ -3599,7 +3625,9 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api): return [output_api.PresubmitPromptWarning( 'Failed to retrieve owner override status - %s' % str(e))] - virtual_depended_on_files = set() + # A set of paths (that might not exist) that are being added as DEPS + # (via lines like "+foo/bar/baz"). + depended_on_paths = set() # Consistently use / as path separator to simplify the writing of regex # expressions. @@ -3610,12 +3638,14 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api): file_filter=file_filter): filename = input_api.os_path.basename(f.LocalPath()) if filename == 'DEPS': - virtual_depended_on_files.update( + depended_on_paths.update( _CalculateAddedDeps(input_api.os_path, '\n'.join(f.OldContents()), '\n'.join(f.NewContents()))) - if not virtual_depended_on_files: + # Requiring reviews is opt-in as of https://crbug.com/365797506 + depended_on_paths = _FindAddedDepsThatRequireReview(input_api, depended_on_paths) + if not depended_on_paths: return [] if input_api.is_committing: @@ -3650,10 +3680,10 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api): owner_email = owner_email or input_api.change.author_email approval_status = input_api.owners_client.GetFilesApprovalStatus( - virtual_depended_on_files, reviewers.union([owner_email]), []) + depended_on_paths, reviewers.union([owner_email]), []) missing_files = [ - f for f in virtual_depended_on_files - if approval_status[f] != input_api.owners_client.APPROVED + p for p in depended_on_paths + if approval_status[p] != input_api.owners_client.APPROVED ] # We strip the /DEPS part that was added by diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index 8a1bdff3757c89..d8eda3613f2d6b 100755 --- a/PRESUBMIT_test.py +++ b/PRESUBMIT_test.py @@ -445,6 +445,29 @@ def testCalculateAddedDepsIgnoresPermutations(self): set(), self.calculate(old_include_rules, {}, new_include_rules, {})) + def testFindAddedDepsThatRequireReview(self): + caring = ['new_usages_require_review = True'] + self.input_api.InitFiles([ + MockAffectedFile('cares/DEPS', caring), + MockAffectedFile('cares/inherits/DEPS', []), + MockAffectedFile('willynilly/DEPS', []), + MockAffectedFile('willynilly/butactually/DEPS', caring), + ]) + + expected = { + 'cares': True, + 'cares/sub/sub': True, + 'cares/inherits': True, + 'cares/inherits/sub': True, + 'willynilly': False, + 'willynilly/butactually': True, + 'willynilly/butactually/sub': True, + } + results = PRESUBMIT._FindAddedDepsThatRequireReview( + self.input_api, set(expected)) + actual = {k: k in results for k in expected} + self.assertEqual(expected, actual) + class FakeOwnersClient(object): APPROVED = "APPROVED" PENDING = "PENDING" @@ -487,6 +510,7 @@ def mock(*args, **kwargs): def testApprovedAdditionalDep(self): self.input_api.InitFiles([ MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']), + MockAffectedFile('v8/DEPS', ['new_usages_require_review=True']), ]) # mark the additional dep as approved. @@ -501,6 +525,7 @@ def testApprovedAdditionalDep(self): def testUnapprovedAdditionalDep(self): self.input_api.InitFiles([ MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']), + MockAffectedFile('v8/DEPS', ['new_usages_require_review=True']), ]) # pending. diff --git a/PRESUBMIT_test_mocks.py b/PRESUBMIT_test_mocks.py index 6a28ea9a24f260..e8872a655258d4 100644 --- a/PRESUBMIT_test_mocks.py +++ b/PRESUBMIT_test_mocks.py @@ -106,6 +106,7 @@ def InitFiles(self, files): def mock_exists(path): if not os.path.isabs(path): path = os.path.join(self.presubmit_local_path, path) + path = os.path.normpath(path) return path in files_that_exist def mock_glob(pattern, *args, **kwargs): @@ -169,8 +170,10 @@ def PresubmitLocalPath(self): def ReadFile(self, filename, mode='r'): if hasattr(filename, 'AbsoluteLocalPath'): filename = filename.AbsoluteLocalPath() + norm_filename = os.path.normpath(filename) for file_ in self.files: - if filename in (file_.LocalPath(), file_.AbsoluteLocalPath()): + to_check = (file_.LocalPath(), file_.AbsoluteLocalPath()) + if filename in to_check or norm_filename in to_check: return '\n'.join(file_.NewContents()) # Otherwise, file is not in our mock API. raise IOError("No such file or directory: '%s'" % filename)