From 4dd9f7205a32b0b8d22c8d8a8ee1eccd5dd8bac4 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Mon, 1 Oct 2012 16:23:03 +0000 Subject: [PATCH] Make apply_issue.py much more verbose about what it's doing. It's to help users figuring out what is happening. TBR=rogerta@chromium.org BUG=153284 Review URL: https://chromiumcodereview.appspot.com/11028002 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@159504 0039d316-1c4b-4281-b951-d872f2087c98 --- apply_issue.py | 4 +- checkout.py | 181 ++++++++++++++++++++++++++++------------- patch.py | 11 +++ tests/checkout_test.py | 23 +++--- 4 files changed, 149 insertions(+), 70 deletions(-) diff --git a/apply_issue.py b/apply_issue.py index 3eb4d61f3..da8d54211 100755 --- a/apply_issue.py +++ b/apply_issue.py @@ -132,9 +132,9 @@ def main(): # chromium_commands.py?view=markup open('.buildbot-patched', 'w').close() - # Apply the patch. + print('\nApplying the patch.') try: - scm_obj.apply_patch(patchset) + scm_obj.apply_patch(patchset, verbose=True) except checkout.PatchApplicationFailed, e: print >> sys.stderr, str(e) return 1 diff --git a/checkout.py b/checkout.py index 03e28f68c..5fd236b4a 100644 --- a/checkout.py +++ b/checkout.py @@ -47,6 +47,17 @@ def get_code_review_setting(path, key, return settings.get(key, None) +def align_stdout(stdout): + """Returns the aligned output of multiple stdouts.""" + output = '' + for item in stdout: + item = item.strip() + if not item: + continue + output += ''.join(' %s\n' % line for line in item.splitlines()) + return output + + class PatchApplicationFailed(Exception): """Patch failed to be applied.""" def __init__(self, p, status): @@ -65,6 +76,7 @@ class PatchApplicationFailed(Exception): out.append('Failed to apply patch for %s:' % self.filename) if self.status: out.append(self.status) + out.append('Patch: %s' % self.patch.dump()) return '\n'.join(out) @@ -105,7 +117,7 @@ class CheckoutBase(object): """ raise NotImplementedError() - def apply_patch(self, patches, post_processors=None): + def apply_patch(self, patches, post_processors=None, verbose=False): """Applies a patch and returns the list of modified files. This function should throw patch.UnsupportedPatchFormat or @@ -139,26 +151,28 @@ class RawCheckout(CheckoutBase): """Stubbed out.""" pass - def apply_patch(self, patches, post_processors=None): + def apply_patch(self, patches, post_processors=None, verbose=False): """Ignores svn properties.""" post_processors = post_processors or self.post_processors or [] for p in patches: - logging.debug('Applying %s' % p.filename) + stdout = [] try: - stdout = '' - filename = os.path.join(self.project_path, p.filename) + filepath = os.path.join(self.project_path, p.filename) if p.is_delete: - os.remove(filename) + os.remove(filepath) + stdout.append('Deleted.') else: dirname = os.path.dirname(p.filename) full_dir = os.path.join(self.project_path, dirname) if dirname and not os.path.isdir(full_dir): os.makedirs(full_dir) + stdout.append('Created missing directory %s.' % dirname) - filepath = os.path.join(self.project_path, p.filename) if p.is_binary: + content = p.get() with open(filepath, 'wb') as f: - f.write(p.get()) + f.write(content) + stdout.append('Added binary file %d bytes.' % len(content)) else: if p.source_filename: if not p.is_new: @@ -171,22 +185,35 @@ class RawCheckout(CheckoutBase): p, 'File exist but was about to be overwriten') shutil.copy2( os.path.join(self.project_path, p.source_filename), filepath) + stdout.append('Copied %s -> %s' % (p.source_filename, p.filename)) if p.diff_hunks: - stdout = subprocess2.check_output( - ['patch', '-u', '--binary', '-p%s' % p.patchlevel], - stdin=p.get(False), - stderr=subprocess2.STDOUT, - cwd=self.project_path) + cmd = ['patch', '-u', '--binary', '-p%s' % p.patchlevel] + if verbose: + cmd.append('--verbose') + stdout.append( + subprocess2.check_output( + cmd, + stdin=p.get(False), + stderr=subprocess2.STDOUT, + cwd=self.project_path)) elif p.is_new and not os.path.exists(filepath): # There is only a header. Just create the file. open(filepath, 'w').close() + stdout.append('Created an empty file.') for post in post_processors: post(self, p) + if verbose: + print p.filename + print align_stdout(stdout) except OSError, e: - raise PatchApplicationFailed(p, '%s%s' % (stdout, e)) + raise PatchApplicationFailed(p, '%s%s' % (align_stdout(stdout), e)) except subprocess.CalledProcessError, e: raise PatchApplicationFailed( - p, '%s%s' % (stdout, getattr(e, 'stdout', None))) + p, + 'While running %s;\n%s%s' % ( + ' '.join(e.cmd), + align_stdout(stdout), + align_stdout([getattr(e, 'stdout', '')]))) def commit(self, commit_message, user): """Stubbed out.""" @@ -299,18 +326,19 @@ class SvnCheckout(CheckoutBase, SvnMixIn): (self.project_name, self.project_path)) return self._revert(revision) - def apply_patch(self, patches, post_processors=None): + def apply_patch(self, patches, post_processors=None, verbose=False): post_processors = post_processors or self.post_processors or [] for p in patches: - logging.debug('Applying %s' % p.filename) + stdout = [] try: + filepath = os.path.join(self.project_path, p.filename) # It is important to use credentials=False otherwise credentials could # leak in the error message. Credentials are not necessary here for the # following commands anyway. - stdout = '' if p.is_delete: - stdout += self._check_output_svn( - ['delete', p.filename, '--force'], credentials=False) + stdout.append(self._check_output_svn( + ['delete', p.filename, '--force'], credentials=False)) + stdout.append('Deleted.') else: # svn add while creating directories otherwise svn add on the # contained files will silently fail. @@ -323,13 +351,16 @@ class SvnCheckout(CheckoutBase, SvnMixIn): dirname = os.path.dirname(dirname) for dir_to_create in reversed(dirs_to_create): os.mkdir(os.path.join(self.project_path, dir_to_create)) - stdout += self._check_output_svn( - ['add', dir_to_create, '--force'], credentials=False) + stdout.append( + self._check_output_svn( + ['add', dir_to_create, '--force'], credentials=False)) + stdout.append('Created missing directory %s.' % dir_to_create) - filepath = os.path.join(self.project_path, p.filename) if p.is_binary: + content = p.get() with open(filepath, 'wb') as f: - f.write(p.get()) + f.write(content) + stdout.append('Added binary file %d bytes.' % len(content)) else: if p.source_filename: if not p.is_new: @@ -340,8 +371,10 @@ class SvnCheckout(CheckoutBase, SvnMixIn): if os.path.isfile(filepath): raise PatchApplicationFailed( p, 'File exist but was about to be overwriten') - self._check_output_svn( - ['copy', p.source_filename, p.filename]) + stdout.append( + self._check_output_svn( + ['copy', p.source_filename, p.filename])) + stdout.append('Copied %s -> %s' % (p.source_filename, p.filename)) if p.diff_hunks: cmd = [ 'patch', @@ -350,24 +383,32 @@ class SvnCheckout(CheckoutBase, SvnMixIn): '--force', '--no-backup-if-mismatch', ] - stdout += subprocess2.check_output( - cmd, stdin=p.get(False), cwd=self.project_path) + stdout.append( + subprocess2.check_output( + cmd, stdin=p.get(False), cwd=self.project_path)) elif p.is_new and not os.path.exists(filepath): # There is only a header. Just create the file if it doesn't # exist. open(filepath, 'w').close() + stdout.append('Created an empty file.') if p.is_new and not p.source_filename: # Do not run it if p.source_filename is defined, since svn copy was # using above. - stdout += self._check_output_svn( - ['add', p.filename, '--force'], credentials=False) + stdout.append( + self._check_output_svn( + ['add', p.filename, '--force'], credentials=False)) for name, value in p.svn_properties: if value is None: - stdout += self._check_output_svn( - ['propdel', '--quiet', name, p.filename], credentials=False) + stdout.append( + self._check_output_svn( + ['propdel', '--quiet', name, p.filename], + credentials=False)) + stdout.append('Property %s deleted.' % name) else: - stdout += self._check_output_svn( - ['propset', name, value, p.filename], credentials=False) + stdout.append( + self._check_output_svn( + ['propset', name, value, p.filename], credentials=False)) + stdout.append('Property %s=%s' % (name, value)) for prop, values in self.svn_config.auto_props.iteritems(): if fnmatch.fnmatch(p.filename, prop): for value in values.split(';'): @@ -378,17 +419,24 @@ class SvnCheckout(CheckoutBase, SvnMixIn): if params[1] == '*': # Works around crbug.com/150960 on Windows. params[1] = '.' - stdout += self._check_output_svn( - ['propset'] + params + [p.filename], credentials=False) + stdout.append( + self._check_output_svn( + ['propset'] + params + [p.filename], credentials=False)) + stdout.append('Property (auto) %s' % '='.join(params)) for post in post_processors: post(self, p) + if verbose: + print p.filename + print align_stdout(stdout) except OSError, e: - raise PatchApplicationFailed(p, '%s%s' % (stdout, e)) + raise PatchApplicationFailed(p, '%s%s' % (align_stdout(stdout), e)) except subprocess.CalledProcessError, e: raise PatchApplicationFailed( p, 'While running %s;\n%s%s' % ( - ' '.join(e.cmd), stdout, getattr(e, 'stdout', ''))) + ' '.join(e.cmd), + align_stdout(stdout), + align_stdout([getattr(e, 'stdout', '')]))) def commit(self, commit_message, user): logging.info('Committing patch for %s' % user) @@ -499,7 +547,7 @@ class GitCheckoutBase(CheckoutBase): if self.working_branch in branches: self._call_git(['branch', '-D', self.working_branch]) - def apply_patch(self, patches, post_processors=None): + def apply_patch(self, patches, post_processors=None, verbose=False): """Applies a patch on 'working_branch' and switch to it. Also commits the changes on the local branch. @@ -514,38 +562,47 @@ class GitCheckoutBase(CheckoutBase): ['checkout', '-b', self.working_branch, '%s/%s' % (self.remote, self.remote_branch), '--quiet']) for index, p in enumerate(patches): - logging.debug('Applying %s' % p.filename) + stdout = [] try: - stdout = '' + filepath = os.path.join(self.project_path, p.filename) if p.is_delete: - if (not os.path.exists(p.filename) and + if (not os.path.exists(filepath) and any(p1.source_filename == p.filename for p1 in patches[0:index])): - # The file could already be deleted if a prior patch with file - # rename was already processed. To be sure, look at all the previous - # patches to see if they were a file rename. + # The file was already deleted if a prior patch with file rename + # was already processed because 'git apply' did it for us. pass else: - stdout += self._check_output_git(['rm', p.filename]) + stdout.append(self._check_output_git(['rm', p.filename])) + stdout.append('Deleted.') else: dirname = os.path.dirname(p.filename) full_dir = os.path.join(self.project_path, dirname) if dirname and not os.path.isdir(full_dir): os.makedirs(full_dir) + stdout.append('Created missing directory %s.' % dirname) if p.is_binary: - with open(os.path.join(self.project_path, p.filename), 'wb') as f: - f.write(p.get()) - stdout += self._check_output_git(['add', p.filename]) + content = p.get() + with open(filepath, 'wb') as f: + f.write(content) + stdout.append('Added binary file %d bytes' % len(content)) + cmd = ['add', p.filename] + if verbose: + cmd.append('--verbose') + stdout.append(self._check_output_git(cmd)) else: # No need to do anything special with p.is_new or if not # p.diff_hunks. git apply manages all that already. - stdout += self._check_output_git( - ['apply', '--index', '-p%s' % p.patchlevel], stdin=p.get(True)) - for name, _ in p.svn_properties: + cmd = ['apply', '--index', '-p%s' % p.patchlevel] + if verbose: + cmd.append('--verbose') + stdout.append(self._check_output_git(cmd, stdin=p.get(True))) + for name, value in p.svn_properties: # Ignore some known auto-props flags through .subversion/config, # bails out on the other ones. # TODO(maruel): Read ~/.subversion/config and detect the rules that # applies here to figure out if the property will be correctly # handled. + stdout.append('Property %s=%s' % (name, value)) if not name in ( 'svn:eol-style', 'svn:executable', 'svn:mime-type'): raise patch.UnsupportedPatchFormat( @@ -554,14 +611,24 @@ class GitCheckoutBase(CheckoutBase): name, p.filename)) for post in post_processors: post(self, p) + if verbose: + print p.filename + print align_stdout(stdout) except OSError, e: - raise PatchApplicationFailed(p, '%s%s' % (stdout, e)) + raise PatchApplicationFailed(p, '%s%s' % (align_stdout(stdout), e)) except subprocess.CalledProcessError, e: raise PatchApplicationFailed( - p, '%s%s' % (stdout, getattr(e, 'stdout', None))) + p, + 'While running %s;\n%s%s' % ( + ' '.join(e.cmd), + align_stdout(stdout), + align_stdout([getattr(e, 'stdout', '')]))) # Once all the patches are processed and added to the index, commit the # index. - self._check_call_git(['commit', '-m', 'Committed patch']) + cmd = ['commit', '-m', 'Committed patch'] + if verbose: + cmd.append('--verbose') + self._check_call_git(cmd) # TODO(maruel): Weirdly enough they don't match, need to investigate. #found_files = self._check_output_git( # ['diff', 'master', '--name-only']).splitlines(False) @@ -643,9 +710,9 @@ class ReadOnlyCheckout(object): def get_settings(self, key): return self.checkout.get_settings(key) - def apply_patch(self, patches, post_processors=None): + def apply_patch(self, patches, post_processors=None, verbose=False): return self.checkout.apply_patch( - patches, post_processors or self.post_processors) + patches, post_processors or self.post_processors, verbose) def commit(self, message, user): # pylint: disable=R0201 logging.info('Would have committed for %s with message: %s' % ( diff --git a/patch.py b/patch.py index 9083094e8..cd594f8e4 100644 --- a/patch.py +++ b/patch.py @@ -100,6 +100,10 @@ class FilePatchBase(object): out += '%s->' % self.source_filename_utf8 return out + self.filename_utf8 + def dump(self): + """Dumps itself in a verbose way to help diagnosing.""" + return str(self) + class FilePatchDelete(FilePatchBase): """Deletes a file.""" @@ -123,6 +127,9 @@ class FilePatchBinary(FilePatchBase): def get(self): return self.data + def __str__(self): + return str(super(FilePatchBinary, self)) + ' %d bytes' % len(self.data) + class Hunk(object): """Parsed hunk data container.""" @@ -491,6 +498,10 @@ class FilePatchDiff(FilePatchBase): # We're done. return + def dump(self): + """Dumps itself in a verbose way to help diagnosing.""" + return str(self) + '\n' + self.get(True) + class PatchSet(object): """A list of FilePatch* objects.""" diff --git a/tests/checkout_test.py b/tests/checkout_test.py index 1c4d7fdd5..5486fac80 100755 --- a/tests/checkout_test.py +++ b/tests/checkout_test.py @@ -308,9 +308,9 @@ class SvnCheckout(SvnBaseTest): self._check_exception( self._get_co(None), 'While running patch -p1 --forward --force --no-backup-if-mismatch;\n' - 'patching file chrome/file.cc\n' - 'Hunk #1 FAILED at 3.\n' - '1 out of 1 hunk FAILED -- saving rejects to file ' + ' patching file chrome/file.cc\n' + ' Hunk #1 FAILED at 3.\n' + ' 1 out of 1 hunk FAILED -- saving rejects to file ' 'chrome/file.cc.rej\n') def testSvnProps(self): @@ -328,8 +328,8 @@ class SvnCheckout(SvnBaseTest): e.status, 'While running svn propset svn:ignore foo chrome/file.cc ' '--non-interactive;\n' - 'patching file chrome/file.cc\n' - 'svn: Cannot set \'svn:ignore\' on a file (\'chrome/file.cc\')\n') + ' patching file chrome/file.cc\n' + ' svn: Cannot set \'svn:ignore\' on a file (\'chrome/file.cc\')\n') co.prepare(None) svn_props = [('svn:eol-style', 'LF'), ('foo', 'bar')] co.apply_patch( @@ -472,9 +472,10 @@ class RawCheckout(SvnBaseTest): def testException(self): self._check_exception( self._get_co(None), - 'patching file chrome/file.cc\n' - 'Hunk #1 FAILED at 3.\n' - '1 out of 1 hunk FAILED -- saving rejects to file ' + 'While running patch -u --binary -p1;\n' + ' patching file chrome/file.cc\n' + ' Hunk #1 FAILED at 3.\n' + ' 1 out of 1 hunk FAILED -- saving rejects to file ' 'chrome/file.cc.rej\n') def testProcess(self): @@ -514,9 +515,9 @@ class ReadOnlyCheckout(SvnBaseTest): self._check_exception( self._get_co(None), 'While running patch -p1 --forward --force --no-backup-if-mismatch;\n' - 'patching file chrome/file.cc\n' - 'Hunk #1 FAILED at 3.\n' - '1 out of 1 hunk FAILED -- saving rejects to file ' + ' patching file chrome/file.cc\n' + ' Hunk #1 FAILED at 3.\n' + ' 1 out of 1 hunk FAILED -- saving rejects to file ' 'chrome/file.cc.rej\n') def testProcess(self):