Reland: Add presubmit check for changes in 3pp

Reland of CL https://webrtc-review.googlesource.com/c/src/+/77421

Copied description:
--
Add presubmit check for changes in 3pp

Presubmit check will test will new changes be overriden by autoroll
or not. In more details presubmit will check:
1. Each dependency in third_party have to be specified in one of:
   a. THIRD_PARTY_CHROMIUM_DEPS.json
   b. THIRD_PARTY_WEBRTC_DEPS.json
2. Each dependency not specified in both files from #1
3. Changes won't be overriden by chromium third_party deps autoroll:
   a. Changes were made in WebRTC owned dependency
   b. Changes were addition of new Chromium owned dependency
--
Also if commit message contains tag NO_AUTOIMPORT_DEPS_CHECK equal
to True, than changes in chromium specific deps will be permitted.
It is required for autoroller to be able to commit its changes and
not to fail on presubmit check.

Bug: webrtc:8366
Change-Id: I545a4778445855cf3db7cf257ca0cb63753aac06
Reviewed-on: https://webrtc-review.googlesource.com/78042
Reviewed-by: Patrik Höglund <phoglund@webrtc.org>
Commit-Queue: Artem Titov <titovartem@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23348}
This commit is contained in:
Artem Titov 2018-05-22 10:21:27 +02:00 committed by Commit Bot
parent c7f09ad2e0
commit e92675b5c4
9 changed files with 388 additions and 11 deletions

View file

@ -13,7 +13,6 @@ import sys
from collections import defaultdict
from contextlib import contextmanager
# Files and directories that are *skipped* by cpplint in the presubmit script.
CPPLINT_BLACKLIST = [
'api/video_codecs/video_decoder.h',
@ -140,6 +139,7 @@ def VerifyNativeApiHeadersListIsValid(input_api, output_api):
non_existing_paths)]
return []
API_CHANGE_MSG = """
You seem to be changing native API header files. Please make sure that you:
1. Make compatible changes that don't break existing clients. Usually
@ -159,6 +159,7 @@ You seem to be changing native API header files. Please make sure that you:
Related files:
"""
def CheckNativeApiHeaderChanges(input_api, output_api):
"""Checks to remind proper changing of native APIs."""
files = []
@ -303,6 +304,7 @@ def CheckApprovedFilesLintClean(input_api, output_api,
return result
def CheckNoSourcesAbove(input_api, gn_files, output_api):
# Disallow referencing source files with paths above the GN file location.
source_pattern = input_api.re.compile(r' +sources \+?= \[(.*?)\]',
@ -331,11 +333,13 @@ def CheckNoSourcesAbove(input_api, gn_files, output_api):
items=violating_gn_files)]
return []
def CheckNoMixingSources(input_api, gn_files, output_api):
"""Disallow mixing C, C++ and Obj-C/Obj-C++ in the same target.
See bugs.webrtc.org/7743 for more context.
"""
def _MoreThanOneSourceUsed(*sources_lists):
sources_used = 0
for source_list in sources_lists:
@ -397,6 +401,7 @@ def CheckNoMixingSources(input_api, gn_files, output_api):
'\n'.join(errors.keys())))]
return []
def CheckNoPackageBoundaryViolations(input_api, gn_files, output_api):
cwd = input_api.PresubmitLocalPath()
with _AddToPath(input_api.os_path.join(
@ -456,6 +461,7 @@ def CheckNoStreamUsageIsAdded(input_api, output_api,
return [output_api.PresubmitError(error_msg, errors)]
return []
def CheckPublicDepsIsNotUsed(gn_files, input_api, output_api):
"""Checks that public_deps is not used without a good reason."""
result = []
@ -477,6 +483,7 @@ def CheckPublicDepsIsNotUsed(gn_files, input_api, output_api):
line_number)))
return result
def CheckCheckIncludesIsNotUsed(gn_files, output_api):
result = []
error_msg = ('check_includes overrides are not allowed since it can cause '
@ -494,6 +501,7 @@ def CheckCheckIncludesIsNotUsed(gn_files, output_api):
line_number)))
return result
def CheckGnChanges(input_api, output_api, source_file_filter):
file_filter = lambda x: (input_api.FilterSourceFile(
x, white_list=(r'.+\.(gn|gni)$',),
@ -514,6 +522,7 @@ def CheckGnChanges(input_api, output_api, source_file_filter):
result.extend(CheckCheckIncludesIsNotUsed(gn_files, output_api))
return result
def CheckGnGen(input_api, output_api):
"""Runs `gn gen --check` with default args to detect mismatches between
#includes and dependencies in the BUILD.gn files, as well as general build
@ -530,6 +539,7 @@ def CheckGnGen(input_api, output_api):
long_text='\n\n'.join(errors))]
return []
def CheckUnwantedDependencies(input_api, output_api, source_file_filter):
"""Runs checkdeps on #include statements added in this
change. Breaking - rules is an error, breaking ! rules is a
@ -589,6 +599,7 @@ def CheckUnwantedDependencies(input_api, output_api, source_file_filter):
warning_descriptions))
return results
def CheckCommitMessageBugEntry(input_api, output_api):
"""Check that bug entries are well-formed in commit message."""
bogus_bug_msg = (
@ -614,6 +625,7 @@ def CheckCommitMessageBugEntry(input_api, output_api):
results.append(bogus_bug_msg % bug)
return [output_api.PresubmitError(r) for r in results]
def CheckChangeHasBugField(input_api, output_api):
"""Requires that the changelist is associated with a bug.
@ -633,6 +645,7 @@ def CheckChangeHasBugField(input_api, output_api):
' * https://bugs.webrtc.org - reference it using Bug: webrtc:XXXX\n'
' * https://crbug.com - reference it using Bug: chromium:XXXXXX')]
def CheckJSONParseErrors(input_api, output_api, source_file_filter):
"""Check that JSON files do not contain syntax errors."""
@ -655,7 +668,8 @@ def CheckJSONParseErrors(input_api, output_api, source_file_filter):
affected_file.AbsoluteLocalPath())
if parse_error:
results.append(output_api.PresubmitError('%s could not be parsed: %s' %
(affected_file.LocalPath(), parse_error)))
(affected_file.LocalPath(),
parse_error)))
return results
@ -778,14 +792,14 @@ def CommonChecks(input_api, output_api):
third_party_filter_list)
hundred_char_sources = lambda x: input_api.FilterSourceFile(x,
white_list=objc_filter_list)
non_third_party_sources = lambda x: input_api.FilterSourceFile(x,
black_list=third_party_filter_list)
results.extend(input_api.canned_checks.CheckLongLines(
input_api, output_api, maxlen=80, source_file_filter=eighty_char_sources))
results.extend(input_api.canned_checks.CheckLongLines(
input_api, output_api, maxlen=100,
source_file_filter=hundred_char_sources))
non_third_party_sources = lambda x: input_api.FilterSourceFile(x,
black_list=third_party_filter_list)
results.extend(input_api.canned_checks.CheckChangeHasNoTabs(
input_api, output_api, source_file_filter=non_third_party_sources))
results.extend(input_api.canned_checks.CheckChangeHasNoStrayWhitespace(
@ -816,9 +830,17 @@ 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(CheckThirdPartyChanges(input_api, output_api))
return results
def CheckThirdPartyChanges(input_api, output_api):
with _AddToPath(input_api.os_path.join(
input_api.PresubmitLocalPath(), 'tools_webrtc', 'presubmit_checks_lib')):
from check_3pp import CheckThirdPartyDirectory
return CheckThirdPartyDirectory(input_api, output_api)
def CheckChangeOnUpload(input_api, output_api):
results = []
results.extend(CommonChecks(input_api, output_api))
@ -874,8 +896,7 @@ def CheckOrphanHeaders(input_api, output_api, source_file_filter):
return results
def CheckNewlineAtTheEndOfProtoFiles(input_api, output_api,
source_file_filter):
def CheckNewlineAtTheEndOfProtoFiles(input_api, output_api, source_file_filter):
"""Checks that all .proto files are terminated with a newline."""
error_msg = 'File {} must end with exactly one newline.'
results = []

View file

@ -5,5 +5,6 @@
"THIRD_PARTY_CHROMIUM_DEPS.json)."
],
"dependencies": [
"OWNERS"
]
}

View file

@ -9,6 +9,9 @@
# This file is inspired to [1].
# [1] - https://cs.chromium.org/chromium/src/PRESUBMIT_test_mocks.py
import os.path
import re
class MockInputApi(object):
"""Mock class for the InputApi class.
@ -20,11 +23,23 @@ class MockInputApi(object):
def __init__(self):
self.change = MockChange([], [])
self.files = []
self.presubmit_local_path = os.path.dirname(__file__)
def AffectedSourceFiles(self, file_filter=None):
return self.AffectedFiles(file_filter=file_filter)
def AffectedFiles(self, file_filter=None, include_deletes=False):
# pylint: disable=unused-argument
return self.files
@classmethod
def FilterSourceFile(cls, affected_file, white_list=(), black_list=()):
# pylint: disable=unused-argument
return True
def PresubmitLocalPath(self):
return self.presubmit_local_path
def ReadFile(self, affected_file, mode='rU'):
filename = affected_file.AbsoluteLocalPath()
for f in self.files:
@ -64,13 +79,20 @@ class MockChange(object):
current change.
"""
def __init__(self, changed_files, bugs_from_description):
def __init__(self, changed_files, bugs_from_description, tags=None):
self._changed_files = changed_files
self._bugs_from_description = bugs_from_description
self.tags = dict() if not tags else tags
def BugsFromDescription(self):
return self._bugs_from_description
def __getattr__(self, attr):
"""Return tags directly as attributes on the object."""
if not re.match(r"^[A-Z_]*$", attr):
raise AttributeError(self, attr)
return self.tags.get(attr)
class MockFile(object):
"""Mock class for the File class.
@ -79,11 +101,30 @@ class MockFile(object):
MockInputApi for presubmit unittests.
"""
def __init__(self, local_path):
def __init__(self, local_path, new_contents=None, old_contents=None,
action='A'):
if new_contents is None:
new_contents = ["Data"]
self._local_path = local_path
self._new_contents = new_contents
self._changed_contents = [(i + 1, l) for i, l in enumerate(new_contents)]
self._action = action
self._old_contents = old_contents
def Action(self):
return self._action
def ChangedContents(self):
return self._changed_contents
def NewContents(self):
return self._new_contents
def LocalPath(self):
return self._local_path
def AbsoluteLocalPath(self):
return self._local_path
def OldContents(self):
return self._old_contents

View file

@ -379,6 +379,7 @@ def GenerateCommitMessage(rev_update, current_commit_pos,
commit_msg.append('TBR=%s' % tbr_authors)
commit_msg.append('BUG=None')
commit_msg.append('CQ_INCLUDE_TRYBOTS=%s' % EXTRA_TRYBOTS)
commit_msg.append('NO_AUTOIMPORT_DEPS_CHECK=true')
return '\n'.join(commit_msg)

View file

@ -0,0 +1,175 @@
#!/usr/bin/env python
# Copyright (c) 2018 The WebRTC project authors. All Rights Reserved.
#
# Use of this source code is governed by a BSD-style license
# that can be found in the LICENSE file in the root of the source
# tree. An additional intellectual property rights grant can be found
# in the file PATENTS. All contributing project authors may
# be found in the AUTHORS file in the root of the source tree.
import json
import logging
import os.path
import subprocess
import sys
import re
def CheckThirdPartyDirectory(input_api, output_api):
# We have to put something in black_list here that won't blacklist
# third_party/* because otherwise default black list will be used. Default
# list contains third_party, so source set will become empty.
third_party_sources = lambda x: (
input_api.FilterSourceFile(x, white_list=(r'^third_party[\\\/].+',),
black_list=(r'^_',)))
webrtc_owned_deps_list_path = input_api.os_path.join(
input_api.PresubmitLocalPath(),
'THIRD_PARTY_WEBRTC_DEPS.json')
chromium_owned_deps_list_path = input_api.os_path.join(
input_api.PresubmitLocalPath(),
'THIRD_PARTY_CHROMIUM_DEPS.json')
webrtc_owned_deps = _LoadDepsList(webrtc_owned_deps_list_path)
chromium_owned_deps = _LoadDepsList(chromium_owned_deps_list_path)
chromium_added_deps = GetChromiumOwnedAddedDeps(input_api)
results = []
results.extend(CheckNoNotOwned3ppDeps(input_api, output_api,
webrtc_owned_deps, chromium_owned_deps))
results.extend(CheckNoBothOwned3ppDeps(output_api, webrtc_owned_deps,
chromium_owned_deps))
results.extend(CheckNoChangesInAutoImportedDeps(input_api, output_api,
webrtc_owned_deps,
chromium_owned_deps,
chromium_added_deps,
third_party_sources))
return results
def GetChromiumOwnedAddedDeps(input_api):
"""Return list of deps that were added into chromium owned deps list."""
chromium_owned_deps_list_source = lambda x: (
input_api.FilterSourceFile(x,
white_list=('THIRD_PARTY_CHROMIUM_DEPS.json',),
black_list=(r'^_',)))
chromium_owned_deps_list = input_api.AffectedFiles(
file_filter=chromium_owned_deps_list_source)
modified_deps_file = next(iter(chromium_owned_deps_list), None)
if not modified_deps_file:
return []
if modified_deps_file.Action() != 'M':
return []
prev_json = json.loads('\n'.join(modified_deps_file.OldContents()))
new_json = json.loads('\n'.join(modified_deps_file.NewContents()))
prev_deps_set = set(prev_json.get('dependencies', []))
new_deps_set = set(new_json.get('dependencies', []))
return list(new_deps_set.difference(prev_deps_set))
def CheckNoNotOwned3ppDeps(input_api, output_api,
webrtc_owned_deps, chromium_owned_deps):
"""Checks that there are no any not owned third_party deps."""
error_msg = ('Third party dependency [{}] have to be specified either in '
'THIRD_PARTY_WEBRTC_DEPS.json or in '
'THIRD_PARTY_CHROMIUM_DEPS.json.\n'
'If you want to add chromium-specific'
'dependency you can run this command (better in separate CL): \n'
'./tools_webrtc/autoroller/checkin_chromium_dep.py -d {}\n'
'If you want to add WebRTC-specific dependency just add it into '
'THIRD_PARTY_WEBRTC_DEPS.json manually')
third_party_dir = os.path.join(input_api.PresubmitLocalPath(), 'third_party')
os.listdir(third_party_dir)
stdout, _ = _RunCommand(['git', 'ls-tree', '--name-only', 'HEAD'],
working_dir=third_party_dir)
not_owned_deps = set()
results = []
for dep_name in stdout.split('\n'):
dep_name = dep_name.strip()
if len(dep_name) == 0:
continue
if dep_name == '.gitignore':
continue
if (dep_name not in webrtc_owned_deps
and dep_name not in chromium_owned_deps):
results.append(
output_api.PresubmitError(error_msg.format(dep_name, dep_name)))
not_owned_deps.add(dep_name)
return results
def CheckNoBothOwned3ppDeps(output_api, webrtc_owned_deps, chromium_owned_deps):
"""Checks that there are no any not owned third_party deps."""
error_msg = ('Third party dependencies {} can\'t be a WebRTC- and '
'Chromium-specific dependency at the same time. '
'Remove them from one of these files: '
'THIRD_PARTY_WEBRTC_DEPS.json or THIRD_PARTY_CHROMIUM_DEPS.json')
both_owned_deps = set(chromium_owned_deps).intersection(
set(webrtc_owned_deps))
results = []
if both_owned_deps:
results.append(output_api.PresubmitError(error_msg.format(
json.dumps(list(both_owned_deps)))))
return results
def CheckNoChangesInAutoImportedDeps(input_api, output_api,
webrtc_owned_deps, chromium_owned_deps, chromium_added_deps,
third_party_sources):
"""Checks that there are no changes in deps imported by autoroller."""
tag = input_api.change.NO_AUTOIMPORT_DEPS_CHECK
if tag is not None and tag.lower() == 'true':
# If there is a tag NO_AUTOIMPORT_DEPS_CHECK in the commit message, then
# permit any changes in chromium's specific deps.
return []
error_msg = ('Changes in [{}] will be overridden during chromium third_party '
'autoroll. If you really want to change this code you have to '
'do it upstream in Chromium\'s third_party.')
results = []
for f in input_api.AffectedFiles(file_filter=third_party_sources):
file_path = f.LocalPath()
split = re.split(r'[\\\/]', file_path)
dep_name = split[1]
if (dep_name not in webrtc_owned_deps
and dep_name in chromium_owned_deps
and dep_name not in chromium_added_deps):
results.append(output_api.PresubmitError(error_msg.format(file_path)))
return results
def _LoadDepsList(file_name):
with open(file_name, 'rb') as f:
content = json.load(f)
return content.get('dependencies', [])
def _RunCommand(command, working_dir):
"""Runs a command and returns the output from that command.
If the command fails (exit code != 0), the function will exit the process.
Returns:
A tuple containing the stdout and stderr outputs as strings.
"""
env = os.environ.copy()
p = subprocess.Popen(command,
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE, env=env,
cwd=working_dir, universal_newlines=True)
std_output, err_output = p.communicate()
p.stdout.close()
p.stderr.close()
if p.returncode != 0:
logging.error('Command failed: %s\n'
'stdout:\n%s\n'
'stderr:\n%s\n', ' '.join(command), std_output, err_output)
sys.exit(p.returncode)
return std_output, err_output

View file

@ -0,0 +1,138 @@
#!/usr/bin/env python
# Copyright (c) 2018 The WebRTC project authors. All Rights Reserved.
#
# Use of this source code is governed by a BSD-style license
# that can be found in the LICENSE file in the root of the source
# tree. An additional intellectual property rights grant can be found
# in the file PATENTS. All contributing project authors may
# be found in the AUTHORS file in the root of the source tree.
import os.path
import sys
import unittest
import check_3pp
SCRIPT_DIR = os.path.realpath(os.path.dirname(os.path.abspath(__file__)))
CHECKOUT_SRC_DIR = os.path.realpath(os.path.join(SCRIPT_DIR, os.pardir,
os.pardir))
TEST_DATA_DIR = os.path.join(SCRIPT_DIR, 'testdata',
'check_third_party_changes')
sys.path.append(CHECKOUT_SRC_DIR)
from presubmit_test_mocks import MockInputApi, MockOutputApi, MockFile
class CheckThirdPartyChangesTest(unittest.TestCase):
def setUp(self):
self._input_api = MockInputApi()
self._output_api = MockOutputApi()
def testGetChromiumOwnedAddedDeps(self):
self._input_api.files = [
MockFile('THIRD_PARTY_CHROMIUM_DEPS.json',
new_contents=[
"""
{
"dependencies": [
"foo",
"bar",
"buzz",
"xyz"
]
}
"""
],
old_contents=[
"""
{
"dependencies": [
"foo",
"buzz"
]
}
"""
],
action='M')
]
added_deps = check_3pp.GetChromiumOwnedAddedDeps(self._input_api)
self.assertEqual(len(added_deps), 2, added_deps)
self.assertIn('bar', added_deps)
self.assertIn('xyz', added_deps)
def testCheckNoNotOwned3ppDepsFire(self):
self._input_api.presubmit_local_path = os.path.join(TEST_DATA_DIR,
'not_owned_dep')
errors = check_3pp.CheckNoNotOwned3ppDeps(self._input_api, self._output_api,
['webrtc'], ['chromium'])
self.assertEqual(len(errors), 1)
self.assertIn('not_owned', errors[0].message)
def testCheckNoNotOwned3ppDepsNotFire(self):
self._input_api.presubmit_local_path = os.path.join(TEST_DATA_DIR,
'not_owned_dep')
errors = check_3pp.CheckNoNotOwned3ppDeps(self._input_api, self._output_api,
['webrtc', 'not_owned'],
['chromium'])
self.assertEqual(len(errors), 0, errors)
def testCheckNoBothOwned3ppDepsFire(self):
errors = check_3pp.CheckNoBothOwned3ppDeps(self._output_api, ['foo', 'bar'],
['buzz', 'bar'])
self.assertEqual(len(errors), 1)
self.assertIn('bar', errors[0].message)
def testCheckNoBothOwned3ppDepsNotFire(self):
errors = check_3pp.CheckNoBothOwned3ppDeps(self._output_api, ['foo', 'bar'],
['buzz'])
self.assertEqual(len(errors), 0, errors)
def testCheckNoChangesInAutoImportedDepsFire(self):
self._input_api.files = [
MockFile('third_party/chromium/source.js')
]
errors = check_3pp.CheckNoChangesInAutoImportedDeps(self._input_api,
self._output_api,
['webrtc'],
['chromium'], [],
None)
self.assertEqual(len(errors), 1)
self.assertIn('chromium', errors[0].message)
def testCheckNoChangesInAutoImportedDepsNotFire(self):
self._input_api.files = [
MockFile('third_party/webrtc/source.js')
]
errors = check_3pp.CheckNoChangesInAutoImportedDeps(self._input_api,
self._output_api,
['webrtc'],
['chromium'], [],
None)
self.assertEqual(len(errors), 0, errors)
def testCheckNoChangesInAutoImportedDepsNotFireOnNewlyAdded(self):
self._input_api.files = [
MockFile('third_party/chromium/source.js')
]
errors = check_3pp.CheckNoChangesInAutoImportedDeps(self._input_api,
self._output_api,
['webrtc'],
['chromium'],
['chromium'], None)
self.assertEqual(len(errors), 0, errors)
def testCheckNoChangesInAutoImportedDepsNotFireOnSpecialTag(self):
self._input_api.files = [
MockFile('third_party/chromium/source.js')
]
self._input_api.change.tags['NO_AUTOIMPORT_DEPS_CHECK'] = 'True'
errors = check_3pp.CheckNoChangesInAutoImportedDeps(self._input_api,
self._output_api,
['webrtc'],
['chromium'],
[], None)
self.assertEqual(len(errors), 0, errors)
if __name__ == '__main__':
unittest.main()