From 4103b383505575b23222f77fd04116d2f6c10273 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Fri, 18 May 2018 13:33:51 +0200 Subject: [PATCH] Add presubmit check for changes in 3pp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Bug: webrtc:8366 Change-Id: Ic5db24289e7fa461e0959f75cfbe81ecc65af4b5 Reviewed-on: https://webrtc-review.googlesource.com/77421 Reviewed-by: Karl Wiberg Reviewed-by: Patrik Höglund Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/master@{#23301} --- PRESUBMIT.py | 37 +++- THIRD_PARTY_WEBRTC_DEPS.json | 3 +- presubmit_test_mocks.py | 56 +++++- .../presubmit_checks_lib/check_3pp.py | 169 ++++++++++++++++++ .../presubmit_checks_lib/check_3pp_test.py | 126 +++++++++++++ .../third_party/chromium/source.js | 0 .../third_party/not_owned/source.js | 0 .../third_party/webrtc/source.js | 0 8 files changed, 379 insertions(+), 12 deletions(-) create mode 100644 tools_webrtc/presubmit_checks_lib/check_3pp.py create mode 100755 tools_webrtc/presubmit_checks_lib/check_3pp_test.py create mode 100644 tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/chromium/source.js create mode 100644 tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/not_owned/source.js create mode 100644 tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/webrtc/source.js diff --git a/PRESUBMIT.py b/PRESUBMIT.py index d4b7796288..37948dbdf8 100755 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -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 = [] @@ -288,7 +289,7 @@ def CheckApprovedFilesLintClean(input_api, output_api, for f in input_api.AffectedSourceFiles(source_file_filter): # Note that moved/renamed files also count as added. if f.Action() == 'A' or not IsLintBlacklisted(blacklist_paths, - f.LocalPath()): + f.LocalPath()): files.append(f.AbsoluteLocalPath()) for file_name in 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 = [] diff --git a/THIRD_PARTY_WEBRTC_DEPS.json b/THIRD_PARTY_WEBRTC_DEPS.json index 2f2190f910..53d45ee913 100644 --- a/THIRD_PARTY_WEBRTC_DEPS.json +++ b/THIRD_PARTY_WEBRTC_DEPS.json @@ -5,5 +5,6 @@ "THIRD_PARTY_CHROMIUM_DEPS.json)." ], "dependencies": [ + "OWNERS" ] -} \ No newline at end of file +} diff --git a/presubmit_test_mocks.py b/presubmit_test_mocks.py index 4b2b3c7522..6c0b0c48a1 100644 --- a/presubmit_test_mocks.py +++ b/presubmit_test_mocks.py @@ -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,10 +23,38 @@ 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): - # pylint: disable=unused-argument - return self.files + return self.AffectedFiles(file_filter=file_filter) + + def AffectedFiles(self, file_filter=None, include_deletes=False): + for f in self.files: + if file_filter and not file_filter(f): + continue + if not include_deletes and f.Action() == 'D': + continue + yield f + + @classmethod + def FilterSourceFile(cls, affected_file, white_list=(), black_list=()): + local_path = affected_file.LocalPath() + found_in_white_list = not white_list + if white_list: + for pattern in white_list: + compiled_pattern = re.compile(pattern) + if compiled_pattern.search(local_path): + found_in_white_list = True + break + if black_list: + for pattern in black_list: + compiled_pattern = re.compile(pattern) + if compiled_pattern.search(local_path): + return False + return found_in_white_list + + def PresubmitLocalPath(self): + return self.presubmit_local_path def ReadFile(self, affected_file, mode='rU'): filename = affected_file.AbsoluteLocalPath() @@ -79,11 +110,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 diff --git a/tools_webrtc/presubmit_checks_lib/check_3pp.py b/tools_webrtc/presubmit_checks_lib/check_3pp.py new file mode 100644 index 0000000000..5ef70b5eff --- /dev/null +++ b/tools_webrtc/presubmit_checks_lib/check_3pp.py @@ -0,0 +1,169 @@ +#!/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.""" + + 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 diff --git a/tools_webrtc/presubmit_checks_lib/check_3pp_test.py b/tools_webrtc/presubmit_checks_lib/check_3pp_test.py new file mode 100755 index 0000000000..1a685ca5a9 --- /dev/null +++ b/tools_webrtc/presubmit_checks_lib/check_3pp_test.py @@ -0,0 +1,126 @@ +#!/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) + + +if __name__ == '__main__': + unittest.main() diff --git a/tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/chromium/source.js b/tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/chromium/source.js new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/not_owned/source.js b/tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/not_owned/source.js new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/webrtc/source.js b/tools_webrtc/presubmit_checks_lib/testdata/check_third_party_changes/not_owned_dep/third_party/webrtc/source.js new file mode 100644 index 0000000000..e69de29bb2