From fb09de29ea2e5eec09ff1daeb17ede8acabd68cb Mon Sep 17 00:00:00 2001 From: Stephen Martinis Date: Thu, 25 Feb 2021 03:41:13 +0000 Subject: [PATCH] Revert "Use GetCodeOwnersClient in presubmit_support" This reverts commit 7a67bca65a00c8cb34cf4d73d30fc36c450d5b72. Reason for revert: Probably broke chromium presubmit, see https://ci.chromium.org/ui/p/chromium/builders/try/chromium_presubmit/1152280/overview Original change's description: > Use GetCodeOwnersClient in presubmit_support > > This change also adds a target_ref flag to presubmit_support.py. > > Recipe-Nontrivial-Roll: build > Change-Id: I6de6bb87fc1482b88d9fbebe5e4ad1dbd8ce9748 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2702792 > Commit-Queue: Gavin Mak > Reviewed-by: Edward Lesmes > Reviewed-by: Josip Sokcevic Change-Id: I2cca469f14194f65306baab7928ddddd48033a3b No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2718691 Auto-Submit: Stephen Martinis Commit-Queue: Rubber Stamper Bot-Commit: Rubber Stamper --- git_cl.py | 4 -- owners_client.py | 2 +- presubmit_support.py | 51 +++++++++---------------- recipes/recipe_modules/presubmit/api.py | 1 - tests/git_cl_test.py | 7 ---- tests/owners_client_test.py | 7 ---- tests/presubmit_unittest.py | 12 ++---- 7 files changed, 22 insertions(+), 62 deletions(-) diff --git a/git_cl.py b/git_cl.py index d26302fb7..141364bc0 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1294,8 +1294,6 @@ class Changelist(object): gerrit_url = self.GetCodereviewServer() issue = self.GetIssue() patchset = self.GetPatchset() - remote, remote_branch = self.GetRemoteBranch() - target_ref = GetTargetRef(remote, remote_branch, None) if author: args.extend(['--author', author]) if gerrit_url: @@ -1304,8 +1302,6 @@ class Changelist(object): args.extend(['--issue', str(issue)]) if patchset: args.extend(['--patchset', str(patchset)]) - if target_ref: - args.extend(['--target_ref', target_ref]) return args diff --git a/owners_client.py b/owners_client.py index 915412e51..14c1bf5ef 100644 --- a/owners_client.py +++ b/owners_client.py @@ -228,6 +228,6 @@ def GetCodeOwnersClient(root, host, project, branch): Defaults to GerritClient, and falls back to DepotToolsClient if code-owners plugin is not available.""" - if host and gerrit_util.IsCodeOwnersEnabled(host): + if gerrit_util.IsCodeOwnersEnabled(host): return GerritClient(host, project, branch) return DepotToolsClient(root, branch) diff --git a/presubmit_support.py b/presubmit_support.py index 70b348948..7b0396276 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -376,10 +376,8 @@ class GerritAccessor(object): To avoid excessive Gerrit calls, caches the results. """ - def __init__(self, host, project=None, branch=None): + def __init__(self, host): self.host = host - self.project = project - self.branch = branch self.cache = {} def _FetchChangeDetail(self, issue): @@ -652,19 +650,10 @@ class InputApi(object): # Temporary files we must manually remove at the end of a run. self._named_temporary_files = [] - host = '' - project = '' - branch = '' - if gerrit_obj: - host = gerrit_obj.host or '' - project = gerrit_obj.project or '' - branch = gerrit_obj.branch or '' - - self.owners_client = owners_client.GetCodeOwnersClient( - root=change.RepositoryRoot(), - host=host, - project=project, - branch=branch) + # TODO(dpranke): figure out a list of all approved owners for a repo + # in order to be able to handle wildcard OWNERS files? + self.owners_client = owners_client.DepotToolsClient( + change.RepositoryRoot(), change.UpstreamBranch(), os_path=self.os_path) self.owners_db = owners_db.Database( change.RepositoryRoot(), fopen=open, os_path=self.os_path) self.owners_finder = owners_finder.OwnersFinder @@ -1518,7 +1507,7 @@ def DoPostUploadExecuter(change, class PresubmitExecuter(object): def __init__(self, change, committing, verbose, gerrit_obj, dry_run=None, - thread_pool=None, parallel=False): + thread_pool=None, parallel=False, gerrit_project=None): """ Args: change: The Change object. @@ -1536,6 +1525,7 @@ class PresubmitExecuter(object): self.more_cc = [] self.thread_pool = thread_pool self.parallel = parallel + self.gerrit_project = gerrit_project def ExecPresubmitScript(self, script_text, presubmit_path): """Executes a single presubmit script. @@ -1578,10 +1568,9 @@ class PresubmitExecuter(object): # Get the URL of git remote origin and use it to identify host and project host = '' - project = '' - if self.gerrit: - host = self.gerrit.host or '' - project = self.gerrit.project or '' + if self.gerrit and self.gerrit.host: + host = self.gerrit.host + project = self.gerrit_project or '' # Prefix for test names prefix = 'presubmit:%s/%s:%s/' % (host, project, rel_path) @@ -1680,7 +1669,8 @@ def DoPresubmitChecks(change, gerrit_obj, dry_run=None, parallel=False, - json_output=None): + json_output=None, + gerrit_project=None): """Runs all presubmit checks that apply to the files in the change. This finds all PRESUBMIT.py files in directories enclosing the files in the @@ -1723,7 +1713,7 @@ def DoPresubmitChecks(change, results = [] thread_pool = ThreadPool() executer = PresubmitExecuter(change, committing, verbose, gerrit_obj, - dry_run, thread_pool, parallel) + dry_run, thread_pool, parallel, gerrit_project) if default_presubmit: if verbose: sys.stdout.write('Running default presubmit script.\n') @@ -1880,15 +1870,11 @@ def _parse_gerrit_options(parser, options): parser: The parser used to parse the arguments from command line. options: The arguments parsed from command line. Returns: - A GerritAccessor object if options.gerrit_url or options.gerrit_project are - set, or None otherwise. + A GerritAccessor object if options.gerrit_url is set, or None otherwise. """ gerrit_obj = None - if options.gerrit_url or options.gerrit_project: - gerrit_obj = GerritAccessor( - urlparse.urlparse(options.gerrit_url or '').netloc, - options.gerrit_project, - options.target_ref) + if options.gerrit_url: + gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc) if not options.gerrit_fetch: return gerrit_obj @@ -1974,8 +1960,6 @@ def main(argv=None): 'executing presubmit or post-upload hooks. fnmatch ' 'wildcards can also be used.') parser.add_argument('--gerrit_project', help=argparse.SUPPRESS) - parser.add_argument('--target_ref', - help='The remote branch ref to use for the change.') options = parser.parse_args(argv) log_level = logging.ERROR @@ -2008,7 +1992,8 @@ def main(argv=None): gerrit_obj, options.dry_run, options.parallel, - options.json_output) + options.json_output, + options.gerrit_project) except PresubmitFailure as e: print(e, file=sys.stderr) print('Maybe your depot_tools is out of date?', file=sys.stderr) diff --git a/recipes/recipe_modules/presubmit/api.py b/recipes/recipe_modules/presubmit/api.py index 0093a258c..3fbdd2d59 100644 --- a/recipes/recipe_modules/presubmit/api.py +++ b/recipes/recipe_modules/presubmit/api.py @@ -98,7 +98,6 @@ class PresubmitApi(recipe_api.RecipeApi): '--issue', self.m.tryserver.gerrit_change.change, '--patchset', self.m.tryserver.gerrit_change.patchset, '--gerrit_url', 'https://%s' % self.m.tryserver.gerrit_change.host, - '--target_ref', self.m.tryserver.gerrit_change_target_ref, '--gerrit_fetch', ] if self.m.cq.state == self.m.cq.DRY: diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 730084e91..fdc884cad 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -2958,9 +2958,6 @@ class ChangelistTest(unittest.TestCase): mock.patch('git_cl.Changelist.GetAuthor', return_value='author').start() mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start() mock.patch('git_cl.Changelist.GetPatchset', return_value=7).start() - mock.patch('git_cl.Changelist.GetRemoteBranch', - return_value=('foo', 'bar')).start() - mock.patch('git_cl.GetTargetRef', return_value='ref').start() mock.patch('git_cl.PRESUBMIT_SUPPORT', 'PRESUBMIT_SUPPORT').start() mock.patch('git_cl.Settings.GetRoot', return_value='root').start() mock.patch('git_cl.time_time').start() @@ -3003,7 +3000,6 @@ class ChangelistTest(unittest.TestCase): '--gerrit_url', 'https://chromium-review.googlesource.com', '--issue', '123456', '--patchset', '7', - '--target_ref', 'ref', '--commit', '--may_prompt', '--parallel', @@ -3052,7 +3048,6 @@ class ChangelistTest(unittest.TestCase): 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', - '--target_ref', 'ref', '--upload', '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', @@ -3100,7 +3095,6 @@ class ChangelistTest(unittest.TestCase): 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', - '--target_ref', 'ref', '--upload', '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', @@ -3141,7 +3135,6 @@ class ChangelistTest(unittest.TestCase): '--gerrit_url', 'https://chromium-review.googlesource.com', '--issue', '123456', '--patchset', '7', - '--target_ref', 'ref', '--post_upload', '--description_file', '/tmp/fake-temp1', ]) diff --git a/tests/owners_client_test.py b/tests/owners_client_test.py index 6f6369f4c..7695f9ccd 100644 --- a/tests/owners_client_test.py +++ b/tests/owners_client_test.py @@ -275,13 +275,6 @@ class GetCodeOwnersClientTest(unittest.TestCase): mock.patch('gerrit_util.IsCodeOwnersEnabled').start() self.addCleanup(mock.patch.stopall) - def testGetCodeOwnersClient_NoHost(self): - owners_client.GetCodeOwnersClient('root', None, 'project', 'branch') - gerrit_util.IsCodeOwnersEnabled.assert_not_called() - - owners_client.GetCodeOwnersClient('root', '', 'project', 'branch') - gerrit_util.IsCodeOwnersEnabled.assert_not_called() - def testGetCodeOwnersClient_GerritClient(self): gerrit_util.IsCodeOwnersEnabled.return_value = True self.assertIsInstance( diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index dad2c4e0d..d745c7c40 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -184,7 +184,6 @@ index fe3de7b..54ae6e1 100755 mock.patch('os.path.abspath', lambda f: f).start() mock.patch('os.path.isfile').start() mock.patch('os.remove').start() - mock.patch('owners_client.GetCodeOwnersClient').start() mock.patch('presubmit_support._parse_files').start() mock.patch('presubmit_support.rdb_wrapper.client', return_value=self.rdb_client).start() @@ -1145,10 +1144,9 @@ def CheckChangeOnCommit(input_api, output_api): upstream=options.upstream) scm.GIT.GetAllFiles.assert_called_once_with(options.root) - def testParseGerritOptions_NoGerritUrlOrProject(self): + def testParseGerritOptions_NoGerritUrl(self): options = mock.Mock( gerrit_url=None, - gerrit_project=None, gerrit_fetch=False, author='author', description='description') @@ -1162,16 +1160,14 @@ def CheckChangeOnCommit(input_api, output_api): def testParseGerritOptions_NoGerritFetch(self): options = mock.Mock( gerrit_url='https://foo-review.googlesource.com/bar', - gerrit_project='baz/project', gerrit_fetch=False, - target_ref='refs/heads/foobar', author='author', description='description') gerrit_obj = presubmit._parse_gerrit_options(None, options) presubmit.GerritAccessor.assert_called_once_with( - 'foo-review.googlesource.com', 'baz/project', 'refs/heads/foobar') + 'foo-review.googlesource.com') self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj) self.assertEqual('author', options.author) self.assertEqual('description', options.description) @@ -1185,16 +1181,14 @@ def CheckChangeOnCommit(input_api, output_api): options = mock.Mock( gerrit_url='https://foo-review.googlesource.com/bar', - gerrit_project='baz/project', gerrit_fetch=True, - target_ref='refs/heads/foobar', issue=123, patchset=4) gerrit_obj = presubmit._parse_gerrit_options(None, options) presubmit.GerritAccessor.assert_called_once_with( - 'foo-review.googlesource.com', 'baz/project', 'refs/heads/foobar') + 'foo-review.googlesource.com') self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj) self.assertEqual('new owner', options.author) self.assertEqual('new description', options.description)