From c621b21ad4bfce59924ffc0116eff58bcd4b8bc0 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Wed, 21 Mar 2018 20:26:56 -0400 Subject: [PATCH] gclient sync: Add support to apply gerrit refs. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mimics bot_update's functionality to apply gerrit refs in gclient via --gerrit-ref flags. When the patch fails to apply, gclient sync will return exit code 2. The idea is to move this logic from bot_update to gclient sync to deal when patches for projects like ANGLE are tried on Chromium bots. This way the patch is applied before recursively parsing and syncing ANGLE’s DEPS.chromium file, which doesn't currently happen. Bug: 643346 Change-Id: I7e2018b3c393a5ac9852b8c3611f906977eeeb18 Reviewed-on: https://chromium-review.googlesource.com/961605 Reviewed-by: Aaron Gable Commit-Queue: Edward Lesmes --- gclient.py | 63 +++++++++++++++++++++++++++++++---- gclient_scm.py | 25 ++++++++++++++ testing_support/fake_repos.py | 8 +++++ tests/gclient_scm_test.py | 6 ++++ tests/gclient_smoketest.py | 25 +++++++++++++- tests/gclient_test.py | 4 +-- 6 files changed, 122 insertions(+), 9 deletions(-) diff --git a/gclient.py b/gclient.py index 8bd839d9f..c4bf01176 100755 --- a/gclient.py +++ b/gclient.py @@ -936,7 +936,8 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): # Arguments number differs from overridden method # pylint: disable=arguments-differ - def run(self, revision_overrides, command, args, work_queue, options): + def run(self, revision_overrides, command, args, work_queue, options, + patch_refs): """Runs |command| then parse the DEPS file.""" logging.info('Dependency(%s).run()' % self.name) assert self._file_list == [] @@ -963,6 +964,13 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): out_cb=work_queue.out_cb) self._got_revision = self._used_scm.RunCommand(command, options, args, file_list) + + patch_repo = parsed_url.split('@')[0] + patch_ref = patch_refs.pop(patch_repo, patch_refs.pop(self.name, None)) + if command == 'update' and patch_ref is not None: + self._used_scm.apply_patch_ref(patch_repo, patch_ref, options, + file_list) + if file_list: file_list = [os.path.join(self.name, f.strip()) for f in file_list] @@ -1596,6 +1604,20 @@ it or fix the checkout. index += 1 return revision_overrides + def _EnforcePatchRefs(self): + """Checks for patch refs.""" + patch_refs = {} + if not self._options.patch_refs: + return patch_refs + for given_patch_ref in self._options.patch_refs: + patch_repo, _, patch_ref = given_patch_ref.partition('@') + if not patch_repo or not patch_ref: + raise gclient_utils.Error( + 'Wrong revision format: %s should be of the form ' + 'patch_repo@patch_ref.' % given_patch_ref) + patch_refs[patch_repo] = patch_ref + return patch_refs + def RunOnDeps(self, command, args, ignore_requirements=False, progress=True): """Runs a command on each dependency in a client and its dependencies. @@ -1607,12 +1629,16 @@ it or fix the checkout. raise gclient_utils.Error('No solution specified') revision_overrides = {} + patch_refs = {} # It's unnecessary to check for revision overrides for 'recurse'. # Save a few seconds by not calling _EnforceRevisions() in that case. if command not in ('diff', 'recurse', 'runhooks', 'status', 'revert', 'validate'): self._CheckConfig() revision_overrides = self._EnforceRevisions() + + if command == 'update': + patch_refs = self._EnforcePatchRefs() # Disable progress for non-tty stdout. should_show_progress = ( setup_color.IS_TTY and not self._options.verbose and progress) @@ -1628,10 +1654,19 @@ it or fix the checkout. for s in self.dependencies: if s.should_process: work_queue.enqueue(s) - work_queue.flush(revision_overrides, command, args, options=self._options) + work_queue.flush(revision_overrides, command, args, options=self._options, + patch_refs=patch_refs) + if revision_overrides: print('Please fix your script, having invalid --revision flags will soon ' - 'considered an error.', file=sys.stderr) + 'be considered an error.', file=sys.stderr) + + if patch_refs: + raise gclient_utils.Error( + 'The following --patch-ref flags were not used. Please fix it:\n%s' % + ('\n'.join( + patch_repo + '@' + patch_ref + for patch_repo, patch_ref in patch_refs.iteritems()))) if self._cipd_root: self._cipd_root.run(command) @@ -1749,7 +1784,7 @@ it or fix the checkout. for s in self.dependencies: if s.should_process: work_queue.enqueue(s) - work_queue.flush({}, None, [], options=self._options) + work_queue.flush({}, None, [], options=self._options, patch_refs=None) def ShouldPrintRevision(path, rev): if not self._options.path and not self._options.url: @@ -1920,14 +1955,15 @@ class CipdDependency(Dependency): self._package_version = version #override - def run(self, revision_overrides, command, args, work_queue, options): + def run(self, revision_overrides, command, args, work_queue, options, + patch_refs): """Runs |command| then parse the DEPS file.""" logging.info('CipdDependency(%s).run()' % self.name) if not self.should_process: return self._CreatePackageIfNecessary() super(CipdDependency, self).run(revision_overrides, command, args, - work_queue, options) + work_queue, options, patch_refs) def _CreatePackageIfNecessary(self): # We lazily create the CIPD package to make sure that only packages @@ -2637,6 +2673,15 @@ def CMDsync(parser, args): 'takes precedence. -r can be used multiple times when ' '.gclient has multiple solutions configured, and will ' 'work even if the src@ part is skipped.') + parser.add_option('--patch-ref', action='append', + dest='patch_refs', metavar='GERRIT_REF', default=[], + help='Patches the given reference with the format dep@ref. ' + 'For dep, you can specify URLs as well as paths, with ' + 'URLs taking preference. The reference will be ' + 'applied to the necessary path, will be rebased on ' + 'top what the dep was synced to, and then will do a ' + 'soft reset. Use --no-rebase-patch-ref and ' + '--reset-patch-ref to disable this behavior.') parser.add_option('--with_branch_heads', action='store_true', help='Clone git "branch_heads" refspecs in addition to ' 'the default refspecs. This adds about 1/2GB to a ' @@ -2708,6 +2753,12 @@ def CMDsync(parser, args): parser.add_option('--disable-syntax-validation', action='store_false', dest='validate_syntax', help='Disable validation of .gclient and DEPS syntax.') + parser.add_option('--no-rebase-patch-ref', action='store_false', + dest='rebase_patch_ref', default=True, + help='Bypass rebase of the patch ref after checkout.') + parser.add_option('--no-reset-patch-ref', action='store_false', + dest='reset_patch_ref', default=True, + help='Bypass calling reset after patching the ref.') (options, args) = parser.parse_args(args) client = GClient.LoadCurrentConfig(options) diff --git a/gclient_scm.py b/gclient_scm.py index ce0ffd72f..a5dfbfceb 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -340,6 +340,31 @@ class GitWrapper(SCMWrapper): self.Print('FAILED to break lock: %s: %s' % (to_break, ex)) raise + def apply_patch_ref(self, patch_repo, patch_ref, options, file_list): + base_rev = self._Capture(['rev-parse', 'HEAD']) + self.Print('===Applying patch ref===') + self.Print('Repo is %r @ %r, ref is %r, root is %r' % ( + patch_repo, patch_ref, base_rev, self.checkout_path)) + self._Capture(['reset', '--hard']) + self._Capture(['fetch', patch_repo, patch_ref]) + file_list.extend(self._GetDiffFilenames('FETCH_HEAD')) + self._Capture(['checkout', 'FETCH_HEAD']) + + if options.rebase_patch_ref: + try: + # TODO(ehmaldonado): Look into cherry-picking to avoid an expensive + # checkout + rebase. + self._Capture(['rebase', base_rev]) + except subprocess2.CalledProcessError as e: + self._Capture(['rebase', '--abort']) + self.Print('Failed to apply %r @ %r to %r at %r' % ( + patch_repo, patch_ref, base_rev, self.checkout_path)) + self.Print('git returned non-zero exit status %s:\n%s' % ( + e.returncode, e.stderr)) + raise + if options.reset_patch_ref: + self._Capture(['reset', '--soft', base_rev]) + def update(self, options, args, file_list): """Runs git to update or transparently checkout the working copy. diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index d54463bb1..f0e574856 100755 --- a/testing_support/fake_repos.py +++ b/testing_support/fake_repos.py @@ -271,6 +271,10 @@ class FakeReposBase(object): self.git_dirty = False return True + def _git_rev_parse(self, path): + return subprocess2.check_output( + ['git', 'rev-parse', 'HEAD'], cwd=path).strip() + def _commit_git(self, repo, tree): repo_root = join(self.git_root, repo) self._genTree(repo_root, tree) @@ -862,6 +866,10 @@ class FakeReposTestBase(trial_dir.TestCase): """Sort-hand: returns the directory tree for a git 'revision'.""" return self.FAKE_REPOS.git_hashes[repo][int(rev)][1] + def gitrevparse(self, repo): + """Returns the actual revision for a given repo.""" + return self.FAKE_REPOS._git_rev_parse(repo) + def main(argv): fake = FakeRepos() diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index dd9fc6790..4742310c1 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -123,6 +123,9 @@ class BaseGitWrapperTestCase(GCBaseTestCase, StdoutCheck, TestCaseUtils, self.jobs = 1 self.break_repo_locks = False self.delete_unversioned_trees = False + self.patch_ref = None + self.patch_repo = None + self.rebase_patch_ref = True sample_git_import = """blob mark :1 @@ -561,6 +564,9 @@ class ManagedGitWrapperTestCaseMox(BaseTestCase): self.break_repo_locks = False # TODO(maruel): Test --jobs > 1. self.jobs = 1 + self.patch_ref = None + self.patch_repo = None + self.rebase_patch_ref = True def Options(self, *args, **kwargs): return self.OptionsObject(*args, **kwargs) diff --git a/tests/gclient_smoketest.py b/tests/gclient_smoketest.py index ca2d646a5..3a1f82ec1 100755 --- a/tests/gclient_smoketest.py +++ b/tests/gclient_smoketest.py @@ -389,7 +389,7 @@ class GClientSmokeGIT(GClientSmokeBase): '--revision', 'invalid@' + self.githash('repo_1', 1)], ['running', 'running', 'running'], 'Please fix your script, having invalid --revision flags ' - 'will soon considered an error.\n') + 'will soon be considered an error.\n') tree = self.mangle_git_tree(('repo_1@2', 'src'), ('repo_2@1', 'src/repo2'), ('repo_3@2', 'src/repo2/repo_renamed')) @@ -536,6 +536,29 @@ class GClientSmokeGIT(GClientSmokeBase): tree['src/git_hooked2'] = 'git_hooked2' self.assertTree(tree) + def testSyncGerritRef(self): + if not self.enabled: + return + self.gclient(['config', self.git_base + 'repo_1', '--name', 'src']) + self.gclient([ + 'sync', '-v', '-v', '-v', + '--revision', 'src/repo2@%s' % self.githash('repo_2', 1), + '--patch-ref', + '%srepo_2@%s' % (self.git_base, self.githash('repo_2', 2)), + ]) + # Assert that repo_2 files coincide with revision @2 (the patch ref) + tree = self.mangle_git_tree(('repo_1@2', 'src'), + ('repo_2@2', 'src/repo2'), + ('repo_3@2', 'src/repo2/repo_renamed')) + tree['src/git_hooked1'] = 'git_hooked1' + tree['src/git_hooked2'] = 'git_hooked2' + self.assertTree(tree) + # Assert that HEAD revision of repo_2 is @1 (the base we synced to) since we + # should have done a soft reset. + self.assertEqual( + self.githash('repo_2', 1), + self.gitrevparse(os.path.join(self.root_dir, 'src/repo2'))) + def testRunHooks(self): if not self.enabled: return diff --git a/tests/gclient_test.py b/tests/gclient_test.py index d53fbb25a..1360581a6 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -261,7 +261,7 @@ class GclientTest(trial_dir.TestCase): work_queue = gclient_utils.ExecutionQueue(options.jobs, None, False) for s in client.dependencies: work_queue.enqueue(s) - work_queue.flush({}, None, [], options=options) + work_queue.flush({}, None, [], options=options, patch_refs={}) self.assertEqual( [h.action for h in client.GetHooks(options)], [tuple(x['action']) for x in hooks]) @@ -312,7 +312,7 @@ class GclientTest(trial_dir.TestCase): work_queue = gclient_utils.ExecutionQueue(options.jobs, None, False) for s in client.dependencies: work_queue.enqueue(s) - work_queue.flush({}, None, [], options=options) + work_queue.flush({}, None, [], options=options, patch_refs={}) self.assertEqual( [h.action for h in client.GetHooks(options)], [tuple(x['action']) for x in hooks + extra_hooks + sub_hooks])