From c2b9bd03e9c98ab318db2b153615c751d9299f9d Mon Sep 17 00:00:00 2001 From: "rmistry@google.com" Date: Mon, 22 Jun 2015 12:19:22 +0000 Subject: [PATCH] Find, upload and apply patchset dependencies. Here is an explanation of the changes in each module: * git_cl.py - IF a local branch is being tracked AND a CL has been uploaded there THEN use the CL's issue number and latest patchset as a dependency. * upload.py - Uploads the patchset dependency, if it exists, to Rietveld (Rietveld will be able to parse this when https://codereview.chromium.org/1155513002/ lands). * rietveld.py - Adds utility methods to get patchset dependencies from the new Rietveld endpoint (the endpoint will exist when https://codereview.chromium.org/1155513002/ lands). * apply_issue.py - If CL3 depends on CL2 which in turn depends on CL1 then apply_issue will gather a list of all issues and patchsets to apply (Eg: [CL1:PS1, CL2:PS1, CL3:PS2]). apply_issue will then loop over the list applying each dependency. Note: The apply_issue.py diff looks much worse than it is. Please see my comment in https://codereview.chromium.org/1149653002/diff/260001/apply_issue.py#oldcode169 Tested end-to-end using a test Git repository (https://skia.googlesource.com/skiabot-test/) and the following CLs created in my test Rietveld instance: * https://skia-codereview-staging.appspot.com/931002 ('Branch1 CL') * https://skia-codereview-staging.appspot.com/5001001 ('Branch2 CL') * https://skia-codereview-staging.appspot.com/9881001 ('Branch3 CL') * https://skia-codereview-staging.appspot.com/3951001 ('Branch3.1 CL') Opt into the new UI and observe the new 'Depends on Patchset' and 'Dependent Patchsets' sections in the above CLs. Design doc is here: https://docs.google.com/document/d/1KZGFKZpOPvco81sYVRCzwlnjGctup71RAzY0MSb0ntc/edit#heading=h.6r6lt4tsvssw BUG=502255 Review URL: https://codereview.chromium.org/1149653002 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@295778 0039d316-1c4b-4281-b951-d872f2087c98 --- apply_issue.py | 126 +++++++++++++++++++++++++++--------------- git_cl.py | 22 ++++++++ rietveld.py | 17 ++++++ third_party/upload.py | 6 ++ 4 files changed, 125 insertions(+), 46 deletions(-) diff --git a/apply_issue.py b/apply_issue.py index 41e133baa..ba389878f 100755 --- a/apply_issue.py +++ b/apply_issue.py @@ -166,52 +166,86 @@ def main(): options.patchset = properties['patchsets'][-1] print('No patchset specified. Using patchset %d' % options.patchset) - print('Downloading the patch.') - try: - patchset = obj.get_patch(options.issue, options.patchset) - except urllib2.HTTPError as e: - print( - 'Failed to fetch the patch for issue %d, patchset %d.\n' - 'Try visiting %s/%d') % ( - options.issue, options.patchset, - options.server, options.issue) - return 1 - if options.whitelist: - patchset.patches = [patch for patch in patchset.patches - if patch.filename in options.whitelist] - if options.blacklist: - patchset.patches = [patch for patch in patchset.patches - if patch.filename not in options.blacklist] - for patch in patchset.patches: - print(patch) - full_dir = os.path.abspath(options.root_dir) - scm_type = scm.determine_scm(full_dir) - if scm_type == 'svn': - scm_obj = checkout.SvnCheckout(full_dir, None, None, None, None) - elif scm_type == 'git': - scm_obj = checkout.GitCheckout(full_dir, None, None, None, None) - elif scm_type == None: - scm_obj = checkout.RawCheckout(full_dir, None, None) - else: - parser.error('Couldn\'t determine the scm') - - # TODO(maruel): HACK, remove me. - # When run a build slave, make sure buildbot knows that the checkout was - # modified. - if options.root_dir == 'src' and getpass.getuser() == 'chrome-bot': - # See sourcedirIsPatched() in: - # http://src.chromium.org/viewvc/chrome/trunk/tools/build/scripts/slave/ - # chromium_commands.py?view=markup - open('.buildbot-patched', 'w').close() - - print('\nApplying the patch.') - try: - scm_obj.apply_patch(patchset, verbose=True) - except checkout.PatchApplicationFailed as e: - print(str(e)) - print('CWD=%s' % os.getcwd()) - print('Checkout path=%s' % scm_obj.project_path) - return 1 + issues_patchsets_to_apply = [(options.issue, options.patchset)] + depends_on_info = obj.get_depends_on_patchset(options.issue, options.patchset) + while depends_on_info: + depends_on_issue = int(depends_on_info['issue']) + depends_on_patchset = int(depends_on_info['patchset']) + try: + depends_on_info = obj.get_depends_on_patchset(depends_on_issue, + depends_on_patchset) + issues_patchsets_to_apply.insert(0, (depends_on_issue, + depends_on_patchset)) + except urllib2.HTTPError: + print ('The patchset that was marked as a dependency no longer ' + 'exists: %s/%d/#ps%d' % ( + options.server, depends_on_issue, depends_on_patchset)) + print 'Therefore it is likely that this patch will not apply cleanly.' + print + depends_on_info = None + + num_issues_patchsets_to_apply = len(issues_patchsets_to_apply) + if num_issues_patchsets_to_apply > 1: + print + print 'apply_issue.py found %d dependent CLs.' % ( + num_issues_patchsets_to_apply - 1) + print 'They will be applied in the following order:' + num = 1 + for issue_to_apply, patchset_to_apply in issues_patchsets_to_apply: + print ' #%d %s/%d/#ps%d' % ( + num, options.server, issue_to_apply, patchset_to_apply) + num += 1 + print + + for issue_to_apply, patchset_to_apply in issues_patchsets_to_apply: + issue_url = '%s/%d/#ps%d' % (options.server, issue_to_apply, + patchset_to_apply) + print('Downloading patch from %s' % issue_url) + try: + patchset = obj.get_patch(issue_to_apply, patchset_to_apply) + except urllib2.HTTPError as e: + print( + 'Failed to fetch the patch for issue %d, patchset %d.\n' + 'Try visiting %s/%d') % ( + issue_to_apply, patchset_to_apply, + options.server, issue_to_apply) + return 1 + if options.whitelist: + patchset.patches = [patch for patch in patchset.patches + if patch.filename in options.whitelist] + if options.blacklist: + patchset.patches = [patch for patch in patchset.patches + if patch.filename not in options.blacklist] + for patch in patchset.patches: + print(patch) + full_dir = os.path.abspath(options.root_dir) + scm_type = scm.determine_scm(full_dir) + if scm_type == 'svn': + scm_obj = checkout.SvnCheckout(full_dir, None, None, None, None) + elif scm_type == 'git': + scm_obj = checkout.GitCheckout(full_dir, None, None, None, None) + elif scm_type == None: + scm_obj = checkout.RawCheckout(full_dir, None, None) + else: + parser.error('Couldn\'t determine the scm') + + # TODO(maruel): HACK, remove me. + # When run a build slave, make sure buildbot knows that the checkout was + # modified. + if options.root_dir == 'src' and getpass.getuser() == 'chrome-bot': + # See sourcedirIsPatched() in: + # http://src.chromium.org/viewvc/chrome/trunk/tools/build/scripts/slave/ + # chromium_commands.py?view=markup + open('.buildbot-patched', 'w').close() + + print('\nApplying the patch from %s' % issue_url) + try: + scm_obj.apply_patch(patchset, verbose=True) + except checkout.PatchApplicationFailed as e: + print(str(e)) + print('CWD=%s' % os.getcwd()) + print('Checkout path=%s' % scm_obj.project_path) + return 1 if ('DEPS' in map(os.path.basename, patchset.filenames) and not options.ignore_deps): diff --git a/git_cl.py b/git_cl.py index 60fd1ce71..50ba3432e 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2085,6 +2085,28 @@ def RietveldUpload(options, args, cl, change): if target_ref: upload_args.extend(['--target_ref', target_ref]) + # Look for dependent patchsets. See crbug.com/480453 for more details. + remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch()) + upstream_branch = ShortBranchName(upstream_branch) + if remote is '.': + # A local branch is being tracked. + local_branch = ShortBranchName(upstream_branch) + auth_config = auth.extract_auth_config_from_options(options) + branch_cl = Changelist(branchref=local_branch, auth_config=auth_config) + branch_cl_issue_url = branch_cl.GetIssueURL() + branch_cl_issue = branch_cl.GetIssue() + branch_cl_patchset = branch_cl.GetPatchset() + if branch_cl_issue_url and branch_cl_issue and branch_cl_patchset: + upload_args.extend( + ['--depends_on_patchset', '%s:%s' % ( + branch_cl_issue, branch_cl_patchset)]) + print + print ('The current branch (%s) is tracking a local branch (%s) with ' + 'an open CL.') % (cl.GetBranch(), local_branch) + print 'Adding %s/#ps%s as a dependency patchset.' % ( + branch_cl_issue_url, branch_cl_patchset) + print + project = settings.GetProject() if project: upload_args.extend(['--project', project]) diff --git a/rietveld.py b/rietveld.py index eddf77eef..68988ab73 100644 --- a/rietveld.py +++ b/rietveld.py @@ -84,6 +84,20 @@ class Rietveld(object): data['description'] = '\n'.join(data['description'].strip().splitlines()) return data + def get_depends_on_patchset(self, issue, patchset): + """Returns the patchset this patchset depends on if it exists.""" + url = '/%d/patchset/%d/get_depends_on_patchset' % (issue, patchset) + resp = None + try: + resp = json.loads(self.get(url)) + except urllib2.HTTPError: + # The get_depends_on_patchset endpoint does not exist on this Rietveld + # instance yet. Ignore the error and proceed. + # TODO(rmistry): Make this an error when all Rietveld instances have + # this endpoint. + pass + return resp + def get_patchset_properties(self, issue, patchset): """Returns the patchset properties.""" url = '/api/%d/%d' % (issue, patchset) @@ -677,6 +691,9 @@ class ReadOnlyRietveld(object): def get_patchset_properties(self, issue, patchset): return self._rietveld.get_patchset_properties(issue, patchset) + def get_depends_on_patchset(self, issue, patchset): + return self._rietveld.get_depends_on_patchset(issue, patchset) + def get_patch(self, issue, patchset): return self._rietveld.get_patch(issue, patchset) diff --git a/third_party/upload.py b/third_party/upload.py index eac2a3d09..002a0d66a 100755 --- a/third_party/upload.py +++ b/third_party/upload.py @@ -635,6 +635,10 @@ group.add_option("--target_ref", action="store", dest="target_ref", parser.add_option("--cq_dry_run", action="store_true", help="Send the patchset to do a CQ dry run right after " "upload.") +parser.add_option("--depends_on_patchset", action="store", + dest="depends_on_patchset", + help="The uploaded patchset this patchset depends on. The " + "value will be in this format- issue_num:patchset_num") group.add_option("--download_base", action="store_true", dest="download_base", default=False, help="Base files will be downloaded by the server " @@ -2436,6 +2440,8 @@ def RealMain(argv, data=None): if options.cq_dry_run: form_fields.append(("cq_dry_run", "1")) form_fields.append(("commit", "1")) + if options.depends_on_patchset: + form_fields.append(("depends_on_patchset", options.depends_on_patchset)) # Process --message, --title and --file. message = options.message or ""