Skip to content

Commit

Permalink
Enforce LGTM from owners of depends-on paths in DEPS via presubmit.
Browse files Browse the repository at this point in the history
This presubmit check has been copied from Chromium's PRESUBMIT.py [1].

Example of the error message:

** Presubmit ERRORS **
You need LGTM from owners of depends-on paths in DEPS that were modified in this CL:
    '+third_party/protobuf/src/google/protobuf',

Suggested missing target path OWNERS:
...

[1] - https://cs.chromium.org/chromium/src/PRESUBMIT.py?l=1475-1550&rcl=57cc805bba436b3f26b86168628a343be8abe2a3

Bug: webrtc:9453
Change-Id: Icc028bcd1d48b83f2f31bb821c708289eebd8623
Reviewed-on: https://webrtc-review.googlesource.com/95885
Commit-Queue: Mirko Bonadei <[email protected]>
Reviewed-by: Patrik Höglund <[email protected]>
Cr-Commit-Position: refs/heads/master@{#24891}
  • Loading branch information
MirkoBonadei authored and Commit Bot committed Sep 28, 2018
1 parent 71a091e commit 7e4ee6e
Show file tree
Hide file tree
Showing 2 changed files with 160 additions and 0 deletions.
11 changes: 11 additions & 0 deletions ENG_REVIEW_OWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Current list of eng reviewers mostly for the purpose of reviewing
# dependencies on third-party directories (because //ENG_REVIEW_OWNERS is
# included by //third_party/OWNERS).

# People listed in this file will only have to coordinate with chromium's eng
# review owners to ensure that the added dependency was OK.

[email protected]
[email protected]
[email protected]
[email protected]
149 changes: 149 additions & 0 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,7 @@ def CommonChecks(input_api, output_api):
input_api, output_api, source_file_filter=non_third_party_sources))
results.extend(CheckNoStreamUsageIsAdded(
input_api, output_api, non_third_party_sources))
results.extend(CheckAddedDepsHaveTargetApprovals(input_api, output_api))
return results


Expand Down Expand Up @@ -930,3 +931,151 @@ def CheckNewlineAtTheEndOfProtoFiles(input_api, output_api, source_file_filter):
if len(lines) > 0 and not lines[-1].endswith('\n'):
results.append(output_api.PresubmitError(error_msg.format(file_path)))
return results


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 _, 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.
class VarImpl(object):

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 = {
'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.
For a directory (rather than a specific filename) we fake a path to
a specific filename by adding /DEPS. This is chosen as a file that
will seldom or never be subject to per-file include_rules.
"""
# We ignore deps entries on auto-generated directories.
auto_generated_dirs = ['grit', 'jni']

old_deps = _ExtractAddRulesFromParsedDeps(_ParseDeps(old_contents))
new_deps = _ExtractAddRulesFromParsedDeps(_ParseDeps(new_contents))

added_deps = new_deps.difference(old_deps)

results = set()
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


def CheckAddedDepsHaveTargetApprovals(input_api, output_api):
"""When a dependency prefixed with + is added to a DEPS file, we
want to make sure that the change is reviewed by an OWNER of the
target file or directory, to avoid layering violations from being
introduced. This check verifies that this happens.
"""
virtual_depended_on_files = set()

file_filter = lambda f: not input_api.re.match(
r"^third_party[\\\/](WebKit|blink)[\\\/].*", f.LocalPath())
for f in input_api.AffectedFiles(include_deletes=False,
file_filter=file_filter):
filename = input_api.os_path.basename(f.LocalPath())
if filename == 'DEPS':
virtual_depended_on_files.update(_CalculateAddedDeps(
input_api.os_path,
'\n'.join(f.OldContents()),
'\n'.join(f.NewContents())))

if not virtual_depended_on_files:
return []

if input_api.is_committing:
if input_api.tbr:
return [output_api.PresubmitNotifyResult(
'--tbr was specified, skipping OWNERS check for DEPS additions')]
if input_api.dry_run:
return [output_api.PresubmitNotifyResult(
'This is a dry run, skipping OWNERS check for DEPS additions')]
if not input_api.change.issue:
return [output_api.PresubmitError(
"DEPS approval by OWNERS check failed: this change has "
"no change number, so we can't check it for approvals.")]
output = output_api.PresubmitError
else:
output = output_api.PresubmitNotifyResult

owners_db = input_api.owners_db
owner_email, reviewers = (
input_api.canned_checks.GetCodereviewOwnerAndReviewers(
input_api,
owners_db.email_regexp,
approval_needed=input_api.is_committing))

owner_email = owner_email or input_api.change.author_email

reviewers_plus_owner = set(reviewers)
if owner_email:
reviewers_plus_owner.add(owner_email)
missing_files = owners_db.files_not_covered_by(virtual_depended_on_files,
reviewers_plus_owner)

# We strip the /DEPS part that was added by
# _FilesToCheckForIncomingDeps to fake a path to a file in a
# directory.
def StripDeps(path):
start_deps = path.rfind('/DEPS')
if start_deps != -1:
return path[:start_deps]
else:
return path
unapproved_dependencies = ["'+%s'," % StripDeps(path)
for path in missing_files]

if unapproved_dependencies:
output_list = [
output('You need LGTM from owners of depends-on paths in DEPS that were '
'modified in this CL:\n %s' %
'\n '.join(sorted(unapproved_dependencies)))]
suggested_owners = owners_db.reviewers_for(missing_files, owner_email)
output_list.append(output(
'Suggested missing target path OWNERS:\n %s' %
'\n '.join(suggested_owners or [])))
return output_list

return []

0 comments on commit 7e4ee6e

Please sign in to comment.