0

Revert "checkdeps: Teach it about new_usages_require_review"

This reverts commit 20ff3edd59.

Reason for revert: Breaking in V8
File "/usr/local/google/home/leszeks/v8/v8/buildtools/checkdeps/builddeps.py", line 373, in _IterSelfAndParentDirectories
    assert parent != current_dir_name, (
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: /usr/local/google/home/leszeks/third_party/inspector_protocol/crdtp, /usr/local/google/home/leszeks/v8/v8, /


Original change's description:
> checkdeps: Teach it about new_usages_require_review
>
> This will cause checkdeps to fail if an include is added to a directory
> that lists new_usages_require_review=True in its DEPS, but contains
> include_dirs entries only for ancestor directories.
>
> Bug: 365797506
> Change-Id: Ib6ccaf6d65bb869ff13f8f3c7722146fc7ad5317
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5953099
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Owners-Override: Andrew Grieve <agrieve@chromium.org>
> Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> Reviewed-by: David Benjamin <davidben@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1386478}

Bug: 365797506
Change-Id: I14e1f734359ad87927a6557a6ff2687acc0fe063
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6043338
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Andrew Grieve <agrieve@chromium.org>
Owners-Override: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1386859}
This commit is contained in:
Andrew Grieve
2024-11-22 16:27:10 +00:00
committed by Chromium LUCI CQ
parent a491af0aab
commit 142fc1efd6
22 changed files with 76 additions and 269 deletions
buildtools/checkdeps
README.mdbuilddeps.pycheckdeps_test.pyrules.py
testdata
requires_review
sub
DEPS
sub
DEPS
no_review
requires_review_users
DEPS
sub
includes_okay
includes_only_sub
usage.cc
content/browser/webauth
services/device
third_party/blink
common
interest_group
origin_trials
public
common
interest_group
renderer
core
inspector
modules
ad_auction
credentialmanagement
peerconnection

@ -99,20 +99,3 @@ directory and then take away permissions from sub-parts, or the reverse.
Note that all directory separators must be `/` slashes (Unix-style) and not
backslashes. All directories should be relative to the source root and use
only lowercase.
# Reviews
`DEPS` files can be used to require a review when someone adds an
`include_rules` entry on a directory. To do so, use:
```
new_usages_require_reviews = True
```
For example, if `//foo/bar/DEPS` sets `new_usages_require_reviews=True`, then:
1) `include_rules` for `//foo` will not allow includes for files in `//foo/bar`
2) A `PRESUBMIT.py` check will enforce that an `OWNER` of `//foo/bar` must +1
the change that adds the `include_rules` entry.
This behavior was the default until fall 2024, when it was switch to opt-in.

@ -12,12 +12,11 @@ See README.md for the format of the deps file.
import copy
import functools
import os.path
import posixpath
import subprocess
import rules
from rules import Rule, Rules
# Variable name used in the DEPS file to add or subtract include files from
@ -80,57 +79,6 @@ def _GitSourceDirectories(base_directory):
return git_source_directories
@functools.lru_cache(maxsize=None)
def _ParseDepsMemoize(dir_path, under_test, verbose):
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,
'Str': str,
}
# The second conditional here is to disregard the
# buildtools/checkdeps/DEPS file while running tests. This DEPS file
# has a skip_child_includes for 'testdata' which is necessary for
# running production tests, since there are intentional DEPS
# violations under the testdata directory. On the other hand when
# running tests, we absolutely need to verify the contents of that
# directory to trigger those intended violations and see that they
# are handled correctly.
deps_file_path = os.path.join(dir_path, 'DEPS')
if os.path.isfile(deps_file_path) and not (
under_test and
os.path.basename(dir_path) == 'checkdeps'):
try:
with open(deps_file_path) as file:
exec(file.read(), global_scope, local_scope)
except Exception as e:
print(' Error reading %s: %s' % (deps_file_path, str(e)))
raise
elif verbose:
print(' No deps file found in', dir_path)
return local_scope
class DepsBuilder(object):
"""Parses include_rules from DEPS files."""
@ -153,7 +101,7 @@ class DepsBuilder(object):
base_directory = (base_directory or
os.path.join(os.path.dirname(__file__),
os.path.pardir, os.path.pardir))
self.base_directory = os.path.normcase(os.path.abspath(base_directory))
self.base_directory = os.path.abspath(base_directory) # Local absolute path
self.extra_repos = extra_repos
self.verbose = verbose
self._under_test = being_tested
@ -161,12 +109,6 @@ class DepsBuilder(object):
self._ignore_specific_rules = ignore_specific_rules
self._git_source_directories = None
# Rules instances need a back reference in order to evaluate
# new_usages_require_review, but cannot hold a direct one because we
# deepcopy them.
self._instance_index = len(rules.deps_builders)
rules.deps_builders.append(self)
if os.getcwd().startswith('/google/cog/cloud'):
self.is_git = False
elif os.path.exists(os.path.join(base_directory, '.git')):
@ -180,8 +122,7 @@ class DepsBuilder(object):
# directories, or None for directories that should be skipped.
# Normalized is: absolute, lowercase, / for separator.
self.directory_rules = {}
self._ApplyDirectoryRulesAndSkipSubdirs(
rules.Rules(self._instance_index), self.base_directory)
self._ApplyDirectoryRulesAndSkipSubdirs(Rules(), self.base_directory)
def _ApplyRules(self, existing_rules, includes, specific_includes,
cur_dir_norm):
@ -198,7 +139,7 @@ class DepsBuilder(object):
Returns: A new set of rules combining the existing_rules with the other
arguments.
"""
new_rules = copy.deepcopy(existing_rules)
rules = copy.deepcopy(existing_rules)
# First apply the implicit "allow" rule for the current directory.
base_dir_norm = NormalizePath(self.base_directory)
@ -211,18 +152,18 @@ class DepsBuilder(object):
# Make the help string a little more meaningful.
source = relative_dir or 'top level'
new_rules.AddRule('+' + relative_dir,
rules.AddRule('+' + relative_dir,
relative_dir,
'Default rule for ' + source)
def ApplyOneRule(rule_str, dependee_regexp=None):
"""Deduces a sensible description for the rule being added, and
adds the rule with its description to |new_rules|.
adds the rule with its description to |rules|.
If we are ignoring temporary rules, this function does nothing
for rules beginning with the Rule.TEMP_ALLOW character.
"""
if self._ignore_temp_rules and rule_str.startswith(rules.Rule.TEMP_ALLOW):
if self._ignore_temp_rules and rule_str.startswith(Rule.TEMP_ALLOW):
return
rule_block_name = 'include_rules'
@ -232,8 +173,7 @@ class DepsBuilder(object):
rule_description = relative_dir + "'s %s" % rule_block_name
else:
rule_description = 'the top level %s' % rule_block_name
new_rules.AddRule(rule_str, relative_dir, rule_description,
dependee_regexp)
rules.AddRule(rule_str, relative_dir, rule_description, dependee_regexp)
# Apply the additional explicit rules.
for rule_str in includes:
@ -241,16 +181,13 @@ class DepsBuilder(object):
# Finally, apply the specific rules.
if self._ignore_specific_rules:
return new_rules
return rules
for regexp, specific_rules in specific_includes.items():
for rule_str in specific_rules:
ApplyOneRule(rule_str, regexp)
return new_rules
def _ParseDeps(self, dir_path):
return _ParseDepsMemoize(dir_path, self._under_test, self.verbose)
return rules
def _ApplyDirectoryRules(self, existing_rules, dir_path_local_abs):
"""Combines rules from the existing rules and the new directory.
@ -277,17 +214,61 @@ class DepsBuilder(object):
# Check the DEPS file in this directory.
if self.verbose:
print('Applying rules from', dir_path_local_abs)
deps_dict = self._ParseDeps(dir_path_local_abs)
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,
'Str': str,
}
deps_file_path = os.path.join(dir_path_local_abs, 'DEPS')
# The second conditional here is to disregard the
# buildtools/checkdeps/DEPS file while running tests. This DEPS file
# has a skip_child_includes for 'testdata' which is necessary for
# running production tests, since there are intentional DEPS
# violations under the testdata directory. On the other hand when
# running tests, we absolutely need to verify the contents of that
# directory to trigger those intended violations and see that they
# are handled correctly.
if os.path.isfile(deps_file_path) and not (
self._under_test and
os.path.basename(dir_path_local_abs) == 'checkdeps'):
try:
with open(deps_file_path) as file:
exec(file.read(), global_scope, local_scope)
except Exception as e:
print(' Error reading %s: %s' % (deps_file_path, str(e)))
raise
elif self.verbose:
print(' No deps file found in', dir_path_local_abs)
# Even if a DEPS file does not exist we still invoke ApplyRules
# to apply the implicit "allow" rule for the current directory
include_rules = deps_dict.get(INCLUDE_RULES_VAR_NAME, [])
specific_include_rules = deps_dict.get(SPECIFIC_INCLUDE_RULES_VAR_NAME,
include_rules = local_scope.get(INCLUDE_RULES_VAR_NAME, [])
specific_include_rules = local_scope.get(SPECIFIC_INCLUDE_RULES_VAR_NAME,
{})
skip_subdirs = deps_dict.get(SKIP_SUBDIRS_VAR_NAME, [])
noparent = deps_dict.get(NOPARENT_VAR_NAME, False)
skip_subdirs = local_scope.get(SKIP_SUBDIRS_VAR_NAME, [])
noparent = local_scope.get(NOPARENT_VAR_NAME, False)
if noparent:
parent_rules = rules.Rules(self._instance_index)
parent_rules = Rules()
else:
parent_rules = existing_rules
@ -364,34 +345,6 @@ class DepsBuilder(object):
yield (current_dir_rules, file_names)
def _IterSelfAndParentDirectories(self, dir_name):
dir_name = os.path.normcase(os.path.abspath(dir_name))
current_dir_name = dir_name
yield current_dir_name
while current_dir_name != self.base_directory:
parent = os.path.dirname(current_dir_name)
assert parent != current_dir_name, (
f'{dir_name}, {self.base_directory}, {current_dir_name}')
current_dir_name = parent
yield current_dir_name
def _DirectoryRequiresReview(self, dir_name):
deps_dict = self._ParseDeps(dir_name)
return deps_dict.get('new_usages_require_review')
def FindFirstAncestorThatRequiresReview(self, include_path):
if not os.path.isabs(include_path):
include_path = os.path.join(self.base_directory, include_path)
parent_dir = os.path.dirname(include_path)
for current_dir in self._IterSelfAndParentDirectories(parent_dir):
requires_review = self._DirectoryRequiresReview(current_dir)
if requires_review is not None:
if requires_review:
return NormalizePath(
os.path.relpath(current_dir, self.base_directory))
return None
return None
def GetDirectoryRules(self, dir_path_local):
"""Returns a Rules object to use for the given directory, or None
if the given directory should be skipped.

@ -30,6 +30,10 @@ class CheckDepsTest(unittest.TestCase):
'buildtools/checkdeps/testdata'))
problems = self.deps_checker.results_formatter.GetResults()
if skip_tests:
self.assertEqual(4, len(problems))
else:
self.assertEqual(5, len(problems))
def VerifySubstringsInProblems(key_path, substrings_in_sequence):
"""Finds the problem in |problems| that contains |key_path|,
@ -42,21 +46,14 @@ class CheckDepsTest(unittest.TestCase):
for problem in problems:
index = problem.find(key_path)
if index != -1:
if substrings_in_sequence is None:
self.fail(
f'Expected no problems for {key_path}, but found: {problem}')
for substring in substrings_in_sequence:
index = problem.find(substring, index + 1)
self.assertTrue(index != -1, '%s in %s' % (substring, problem))
found = True
break
if not found and substrings_in_sequence is not None:
if not found:
self.fail('Found no problem for file %s' % key_path)
def VerifyNoProblems(key_path):
VerifySubstringsInProblems(key_path, None)
if ignore_temp_rules:
VerifySubstringsInProblems('testdata/allowed/test.h',
['-buildtools/checkdeps/testdata/disallowed',
@ -80,23 +77,8 @@ class CheckDepsTest(unittest.TestCase):
VerifySubstringsInProblems('testdata/noparent/test.h',
['allowed/bad.h',
'Because of no rule applying'])
VerifySubstringsInProblems('testdata/requires_review_users/usage.cc', [
'testdata/requires_review/sub/foo.h"',
'requires_review/sub", which is marked'])
VerifyNoProblems('requires_review_users/sub/includes_okay/usage.cc')
VerifySubstringsInProblems(
'requires_review_users/sub/includes_only_sub/usage.cc',
[
'testdata/requires_review/sub/foo.h"',
'requires_review/sub", which is',
'testdata/requires_review/sub/inherited/foo.h"',
'testdata/requires_review/sub/sub/inherited/bar.h"',
'requires_review/sub/sub", which is',
])
if skip_tests:
VerifyNoProblems('allowed/not_a_test.cc')
else:
if not skip_tests:
VerifySubstringsInProblems('allowed/not_a_test.cc',
['-buildtools/checkdeps/testdata/disallowed'])
@ -121,10 +103,10 @@ class CheckDepsTest(unittest.TestCase):
return self.deps_checker.results_formatter.GetResults()
def testCountViolations(self):
self.assertEqual('15', self.CountViolations(False))
self.assertEqual('11', self.CountViolations(False))
def testCountViolationsIgnoringTempRules(self):
self.assertEqual('16', self.CountViolations(True))
self.assertEqual('12', self.CountViolations(True))
def testCountViolationsWithRelativePath(self):
self.deps_checker.results_formatter = results.CountViolationsFormatter()

@ -9,12 +9,6 @@ import os
import re
# Rules instances need a back reference in order to evaluate
# new_usages_require_review, but cannot hold a direct one because we
# deepcopy them. Use this list to enable indirect references.
deps_builders = []
class Rule(object):
"""Specifies a single rule for an include, which can be one of
ALLOW, DISALLOW and TEMP_ALLOW.
@ -101,7 +95,7 @@ class Rules(object):
set of rules per unique regular expression.
"""
def __init__(self, deps_builder_index):
def __init__(self):
"""Initializes the current rules with an empty rule list for all
files.
"""
@ -115,9 +109,6 @@ class Rules(object):
# their internal order is arbitrary.
self._specific_rules = {}
# Index of the DepsBuilder associated with this instance.
self._deps_builder_index = deps_builder_index
def __str__(self):
result = ['Rules = {\n (apply to all files): [\n%s\n ],' % '\n'.join(
' %s' % x for x in self._general_rules)]
@ -127,10 +118,6 @@ class Rules(object):
result.append(' }')
return '\n'.join(result)
def _FindFirstAncestorThatRequiresReview(self, include_path):
deps_builder = deps_builders[self._deps_builder_index]
return deps_builder.FindFirstAncestorThatRequiresReview(include_path)
def AsDependencyTuples(self, include_general_rules, include_specific_rules):
"""Returns a list of tuples (allow, dependent dir, dependee dir) for the
specified rules (general/specific). Currently only general rules are
@ -172,6 +159,10 @@ class Rules(object):
else:
rules_to_update = []
# Remove any existing rules or sub-rules that apply. For example, if we're
# passed "foo", we should remove "foo", "foo/bar", but not "foobar".
rules_to_update = [x for x in rules_to_update
if not x.ParentOrMatch(rule_dir)]
rules_to_update.insert(0, Rule(rule_type, rule_dir, dependent_dir, source))
if not dependee_regexp:
@ -183,27 +174,13 @@ class Rules(object):
"""Returns the rule that applies to |include_path| for a dependee
file located at |dependee_path|.
"""
review_parent = self._FindFirstAncestorThatRequiresReview(include_path)
almost_match = None
dependee_filename = os.path.basename(dependee_path)
for regexp, specific_rules in list(self._specific_rules.items()):
if re.match(regexp, dependee_filename):
for rule in specific_rules:
if rule.ChildOrMatch(include_path):
if review_parent is None:
return rule
if rule.ParentOrMatch(review_parent):
return rule
almost_match = rule
return rule
for rule in self._general_rules:
if rule.ChildOrMatch(include_path):
if review_parent is None:
return rule
if rule.ParentOrMatch(review_parent):
return rule
almost_match = rule
if almost_match:
return MessageRule(f'no rule applying for directory "{review_parent}", '
'which is marked as new_usages_require_review=True.')
return rule
return MessageRule('no rule applying.')

@ -1 +0,0 @@
new_usages_require_review = True

@ -1 +0,0 @@
new_usages_require_review = True

@ -1 +0,0 @@
new_usages_require_review = False

@ -1,3 +0,0 @@
include_rules = [
"+buildtools/checkdeps/testdata/requires_review",
]

@ -1,5 +0,0 @@
include_rules = [
"+buildtools/checkdeps/testdata/requires_review",
"+buildtools/checkdeps/testdata/requires_review/sub",
"+buildtools/checkdeps/testdata/requires_review/sub/sub",
]

@ -1,14 +0,0 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Okay since no review required.
#include "buildtools/checkdeps/testdata/requires_review/foo.h"
#include "buildtools/checkdeps/testdata/requires_review/inherited/foo.h"
// Okay since added to DEPS.
#include "buildtools/checkdeps/testdata/requires_review/sub/foo.h"
#include "buildtools/checkdeps/testdata/requires_review/sub/inherited/foo.h"
#include "buildtools/checkdeps/testdata/requires_review/sub/sub/foo.h"
#include "buildtools/checkdeps/testdata/requires_review/sub/sub/inherited/foo.h"
// Okay since no review required (and added to DEPS anyways).
#include "buildtools/checkdeps/testdata/requires_review/sub/sub/no_review/inherited/foo.h"

@ -1,4 +0,0 @@
include_rules = [
"+buildtools/checkdeps/testdata/requires_review",
"+buildtools/checkdeps/testdata/requires_review/sub/sub/foo.h",
]

@ -1,16 +0,0 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Okay since no review required.
#include "buildtools/checkdeps/testdata/requires_review/foo.h"
#include "buildtools/checkdeps/testdata/requires_review/inherited/foo.h"
// Not okay, since no DEPS entry for "sub":
#include "buildtools/checkdeps/testdata/requires_review/sub/foo.h"
#include "buildtools/checkdeps/testdata/requires_review/sub/inherited/foo.h"
// Okay since DEPS entry exits:
#include "buildtools/checkdeps/testdata/requires_review/sub/sub/foo.h"
// Not okay, since no DEPS is only for foo.h
#include "buildtools/checkdeps/testdata/requires_review/sub/sub/inherited/bar.h"
// Okay since no review required.
#include "buildtools/checkdeps/testdata/requires_review/sub/sub/no_review/inherited/foo.h"

@ -1,12 +0,0 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Okay since no review required.
#include "buildtools/checkdeps/testdata/requires_review/foo.h"
// Not okay since review required.
#include "buildtools/checkdeps/testdata/requires_review/sub/foo.h"
// Okay since no review required.
#include "buildtools/checkdeps/testdata/requires_review/sub/sub/no_review/foo.h"
// Okay since no review required.
#include "buildtools/checkdeps/testdata/requires_review/sub/sub/no_review/sub/foo.h"

@ -1,5 +1,5 @@
include_rules = [
"+device/fido",
"+device",
"+third_party/boringssl/src/pki",
]

@ -4,7 +4,6 @@ include_rules = [
"+components/system_cpu",
"+components/variations",
"+device",
"+device/fido",
"+net/base",
"+services/device/usb/jni_headers",
"+services/network/public/cpp",

@ -1,5 +0,0 @@
specific_include_rules = {
"interest_group.cc": [
"+third_party/boringssl/src/include",
],
}

@ -1,5 +0,0 @@
specific_include_rules = {
"trial_token.cc": [
"+third_party/boringssl/src/include",
],
}

@ -1,5 +0,0 @@
specific_include_rules = {
"interest_group.h": [
"+third_party/boringssl/src/include",
],
}

@ -43,7 +43,4 @@ specific_include_rules = {
"inspector_network_agent.h": [
"+net/filter/source_stream.h",
],
"inspector_network_agent.cc": [
"+third_party/boringssl/src/include",
],
}

@ -6,12 +6,6 @@ include_rules = [
]
specific_include_rules = {
"navigator_auction.cc": [
"+third_party/boringssl/src/include",
],
"validate_blink_interest_group.cc": [
"+third_party/boringssl/src/include",
],
"validate_blink_interest_group_test.cc": [
"+base",
"+url/gurl.h",

@ -10,9 +10,6 @@ include_rules = [
]
specific_include_rules = {
"credential_manager_type_converters.cc": [
"+third_party/boringssl/src/include",
],
".*test\.cc": [
"+components/ukm/test_ukm_recorder.h",
],

@ -36,9 +36,6 @@ include_rules = [
]
specific_include_rules = {
"rtc_dtls_transport.cc": [
"+third_party/boringssl/src/include",
],
".*test.*": [
# TODO(crbug.com/1294176): Remove when no longer needed.
"+base/cfi_buildflags.h",