mirror of
https://chromium.googlesource.com/chromium/tools/depot_tools.git
synced 2026-01-11 18:51:29 +00:00
Revert "[presubmit checks] Check if files are written to a dep dir"
This reverts commit 561772c448.
Reason for revert: `KeyError: 'deps'` breaking some builds
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 <gavinmak@google.com>
> Commit-Queue: Josip Sokcevic <sokcevic@chromium.org>
Bug: 343199633
Change-Id: I26b8e72320260e394a72aee1f7e67dfc2b3e2987
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5641258
Auto-Submit: Gavin Mak <gavinmak@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
This commit is contained in:
@@ -1766,9 +1766,6 @@ 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(
|
||||
@@ -2139,46 +2136,6 @@ 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['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."""
|
||||
|
||||
@@ -7,13 +7,14 @@ 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, MockAffectedFile, MockInputApi, MockOutputApi, MockChange)
|
||||
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
|
||||
|
||||
import presubmit_canned_checks
|
||||
|
||||
@@ -440,49 +441,5 @@ 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))
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user