From 2517f89cdf145753593c49bf6dcc9389b328a1dd Mon Sep 17 00:00:00 2001 From: Josip Sokcevic Date: Thu, 20 Jun 2024 18:11:10 +0000 Subject: [PATCH] Reland "[presubmit checks] Check if files are written to a dep dir" This is a reland of commit 561772c44867241a009b445a3732162821fcce60 Original change's description: > [presubmit checks] Check if files are written to a dep dir > > No files should be written to a directory that's used by CIPD or GCS, as > defined in DEPS. Git already doesn't allow files to be written to a > directory that's a gitlink. > > R=jojwang@google.com > > Bug: 343199633 > Change-Id: I8d3414eac728580eaf9ac7e337bb22bca3989e4e > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5633187 > Reviewed-by: Gavin Mak > Commit-Queue: Josip Sokcevic Bug: 343199633 Change-Id: Ifc3f6c4df328cdd215ceb7d0333f68223e9a1ccb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5643922 Reviewed-by: Gavin Mak Commit-Queue: Gavin Mak Auto-Submit: Josip Sokcevic --- presubmit_canned_checks.py | 43 +++++++++++++++++ tests/presubmit_canned_checks_test.py | 67 +++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index b0d13c730..6dd4c3c29 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -1766,6 +1766,9 @@ def PanProjectChecks(input_api, input_api, output_api)) if global_checks: + results.extend( + input_api.canned_checks.CheckNoNewGitFilesAddedInDependencies( + input_api, output_api)) if input_api.change.scm == 'git': snapshot("checking for commit objects in tree") results.extend( @@ -2136,6 +2139,46 @@ def CheckForRecursedeps(input_api, output_api): return errors +def _readDeps(input_api): + """Read DEPS file from the checkout disk. Extracted for testability.""" + deps_file = input_api.os_path.join(input_api.PresubmitLocalPath(), 'DEPS') + with open(deps_file) as f: + return f.read() + + +def CheckNoNewGitFilesAddedInDependencies(input_api, output_api): + """Check if there are Git files in any DEPS dependencies. Error is returned + if there are.""" + try: + deps = _ParseDeps(_readDeps(input_api)) + except FileNotFoundError: + # If there's no DEPS file, there is nothing to check. + return [] + + dependency_paths = set() + for path, dep in deps.get('deps', {}).items(): + if 'condition' in dep and 'non_git_source' in dep['condition']: + # TODO(crbug.com/40738689): Remove src/ prefix + dependency_paths.add(path[4:]) # 4 == len('src/') + + errors = [] + for file in input_api.AffectedFiles(include_deletes=False): + path = file.LocalPath() + # We are checking path, and all paths below up to root. E.g. if path is + # a/b/c, we start with path == "a/b/c", followed by "a/b" and "a". + while path: + if path in dependency_paths: + errors.append( + output_api.PresubmitError( + 'You cannot place files tracked by Git inside a ' + 'first-party DEPS dependency (deps).\n' + f'Dependency: {path}\n' + f'File: {file.LocalPath()}')) + path = _os.path.dirname(path) + + return errors + + @functools.lru_cache(maxsize=None) def _ParseDeps(contents): """Simple helper for parsing DEPS files.""" diff --git a/tests/presubmit_canned_checks_test.py b/tests/presubmit_canned_checks_test.py index 83e4c8b2e..8d4a75f0b 100644 --- a/tests/presubmit_canned_checks_test.py +++ b/tests/presubmit_canned_checks_test.py @@ -7,14 +7,13 @@ import os.path import subprocess import sys import unittest +from unittest import mock ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, ROOT_DIR) -from testing_support.presubmit_canned_checks_test_mocks import MockFile -from testing_support.presubmit_canned_checks_test_mocks import (MockInputApi, - MockOutputApi) -from testing_support.presubmit_canned_checks_test_mocks import MockChange +from testing_support.presubmit_canned_checks_test_mocks import ( + MockFile, MockAffectedFile, MockInputApi, MockOutputApi, MockChange) import presubmit_canned_checks @@ -441,5 +440,65 @@ class CheckUpdateOwnersFileReferences(unittest.TestCase): self.assertEqual(0, len(results)) +class CheckNoNewGitFilesAddedInDependenciesTest(unittest.TestCase): + + @mock.patch('presubmit_canned_checks._readDeps') + def testNonNested(self, readDeps): + readDeps.return_value = '''deps = { + 'src/foo': {'url': 'bar', 'condition': 'non_git_source'}, + 'src/components/foo/bar': {'url': 'bar', 'condition': 'non_git_source'}, + }''' + + input_api = MockInputApi() + input_api.files = [ + MockFile('components/foo/file1.java', ['otherFunction']), + MockFile('components/foo/file2.java', ['hasSyncConsent']), + MockFile('chrome/foo/file3.java', ['canSyncFeatureStart']), + MockFile('chrome/foo/file4.java', ['isSyncFeatureEnabled']), + MockFile('chrome/foo/file5.java', ['isSyncFeatureActive']), + ] + results = presubmit_canned_checks.CheckNoNewGitFilesAddedInDependencies( + input_api, MockOutputApi()) + + self.assertEqual(0, len(results)) + + @mock.patch('presubmit_canned_checks._readDeps') + def testCollision(self, readDeps): + readDeps.return_value = '''deps = { + 'src/foo': {'url': 'bar', 'condition': 'non_git_source'}, + 'src/baz': {'url': 'baz'}, + }''' + + input_api = MockInputApi() + input_api.files = [ + MockAffectedFile('fo', 'content'), # no conflict + MockAffectedFile('foo', 'content'), # conflict + MockAffectedFile('foo/bar', 'content'), # conflict + MockAffectedFile('baz/qux', 'content'), # conflict, but ignored + ] + results = presubmit_canned_checks.CheckNoNewGitFilesAddedInDependencies( + input_api, MockOutputApi()) + + self.assertEqual(2, len(results)) + self.assertIn('File: foo', str(results)) + self.assertIn('File: foo/bar', str(results)) + + @mock.patch('presubmit_canned_checks._readDeps') + def testNoDeps(self, readDeps): + readDeps.return_value = '' # Empty deps + + input_api = MockInputApi() + input_api.files = [ + MockAffectedFile('fo', 'content'), # no conflict + MockAffectedFile('foo', 'content'), # conflict + MockAffectedFile('foo/bar', 'content'), # conflict + MockAffectedFile('baz/qux', 'content'), # conflict, but ignored + ] + results = presubmit_canned_checks.CheckNoNewGitFilesAddedInDependencies( + input_api, MockOutputApi()) + + self.assertEqual(0, len(results)) + + if __name__ == '__main__': unittest.main()