apply_issue: cleanup Rietveld patch download failure.

apply_issue.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Applies an issue from Rietveld.
 """
-
 import getpass
 import json
 import logging
 import optparse
 import os
 import subprocess
 import sys
 import urllib2
 
 import breakpad  # pylint: disable=W0611
 
 import annotated_gclient
 import auth
 import checkout
 import fix_encoding
 import gclient_utils
 import rietveld
 import scm
 
 BASE_DIR = os.path.dirname(os.path.abspath(__file__))
 
 
+RETURN_CODE_ARG_PARSER = 2     # default in python.
+RETURN_CODE_INFRA_FAILURE = 3  # considered as infra failure.
+RETURN_CODE_OTHERWISE = 1      # any other failure, likely patch apply one.

[pgervais, 2015/10/05 16:13:51]

Nit: RETURN_CODE_FAILURE_OTHER ?

'RETURN_CODE_OTHERWISE' in isolation doesn't help very much. Is it a failure or
a success?

Same for ARG_PARSER. I would add 'failure' somewhere in the name.

[tandrii(chromium), 2015/10/05 17:55:43]

Done.

+
+
 class Unbuffered(object):
   """Disable buffering on a file object."""
   def __init__(self, stream):
     self.stream = stream
 
   def write(self, data):
     self.stream.write(data)
     self.stream.flush()
 
   def __getattr__(self, attr):
     return getattr(self.stream, attr)
 
 
-def main():
-  # TODO(pgervais): This function is way too long. Split.
-  sys.stdout = Unbuffered(sys.stdout)
+def _get_arg_parser():
   parser = optparse.OptionParser(description=sys.modules[__name__].__doc__)
   parser.add_option(
       '-v', '--verbose', action='count', default=0,
       help='Prints debugging infos')
   parser.add_option(
       '-e', '--email',
       help='Email address to access rietveld.  If not specified, anonymous '
            'access will be used.')
   parser.add_option(
       '-E', '--email-file',
@@ skipping 28 matching lines @@
                          'is detected.')
   parser.add_option('-b', '--base_ref', help='DEPRECATED do not use.')
   parser.add_option('--whitelist', action='append', default=[],
                     help='Patch only specified file(s).')
   parser.add_option('--blacklist', action='append', default=[],
                     help='Don\'t patch specified file(s).')
   parser.add_option('-d', '--ignore_deps', action='store_true',
                     help='Don\'t run gclient sync on DEPS changes.')
 
   auth.add_auth_options(parser)
+  return parser
+
+
+def main():
+  # TODO(pgervais,tandrii): split this func, it's still too long.
+  sys.stdout = Unbuffered(sys.stdout)
+  parser = _get_arg_parser()
   options, args = parser.parse_args()
   auth_config = auth.extract_auth_config_from_options(options)
 
   if options.whitelist and options.blacklist:
     parser.error('Cannot specify both --whitelist and --blacklist')
 
   if options.email and options.email_file:
     parser.error('-e and -E options are incompatible')
 
   if (os.path.isfile(os.path.join(os.getcwd(), 'update.flag'))
       and not options.force):
     print 'update.flag file found: bot_update has run and checkout is already '
     print 'in a consistent state. No actions will be performed in this step.'
     return 0
+
   logging.basicConfig(
       format='%(levelname)5s %(module)11s(%(lineno)4d): %(message)s',
       level=[logging.WARNING, logging.INFO, logging.DEBUG][
           min(2, options.verbose)])
   if args:
     parser.error('Extra argument(s) "%s" not understood' % ' '.join(args))
   if not options.issue:
     parser.error('Require --issue')
   options.server = options.server.rstrip('/')
   if not options.server:
     parser.error('Require a valid server')
 
   options.revision_mapping = json.loads(options.revision_mapping)
 
   # read email if needed
   if options.email_file:
     if not os.path.exists(options.email_file):
       parser.error('file does not exist: %s' % options.email_file)
     with open(options.email_file, 'rb') as f:
       options.email = f.read().strip()
 
   print('Connecting to %s' % options.server)
   # Always try un-authenticated first, except for OAuth2
   if options.private_key_file:
     # OAuth2 authentication
-    obj = rietveld.JwtOAuth2Rietveld(options.server,
+    rietveld_obj = rietveld.JwtOAuth2Rietveld(options.server,
                                      options.email,
                                      options.private_key_file)
-    properties = obj.get_issue_properties(options.issue, False)
+    try:
+      properties = rietveld_obj.get_issue_properties(options.issue, False)
+    except urllib2.URLError:
+      logging.exception('failed to fetch issue properties')
+      sys.exit(RETURN_CODE_INFRA_FAILURE)
   else:
     # Passing None as auth_config disables authentication.
-    obj = rietveld.Rietveld(options.server, None)
+    rietveld_obj = rietveld.Rietveld(options.server, None)
     properties = None
     # Bad except clauses order (HTTPError is an ancestor class of
     # ClientLoginError)
     # pylint: disable=E0701
     try:
-      properties = obj.get_issue_properties(options.issue, False)
+      properties = rietveld_obj.get_issue_properties(options.issue, False)
     except urllib2.HTTPError as e:
       if e.getcode() != 302:
         raise
       if options.no_auth:
         exit('FAIL: Login detected -- is issue private?')
       # TODO(maruel): A few 'Invalid username or password.' are printed first,
       # we should get rid of those.
+    except urllib2.URLError:
+      logging.exception('failed to fetch issue properties')
+      return RETURN_CODE_INFRA_FAILURE
     except rietveld.upload.ClientLoginError as e:
       # Fine, we'll do proper authentication.
       pass
     if properties is None:
-      obj = rietveld.Rietveld(options.server, auth_config, options.email)
+      rietveld_obj = rietveld.Rietveld(options.server, auth_config,
+                                       options.email)
       try:
-        properties = obj.get_issue_properties(options.issue, False)
+        properties = rietveld_obj.get_issue_properties(options.issue, False)
       except rietveld.upload.ClientLoginError as e:
         print('Accessing the issue requires proper credentials.')
-        return 1
+        return RETURN_CODE_OTHERWISE
+      except urllib2.URLError:
+        logging.exception('failed to fetch issue properties')
+        return RETURN_CODE_INFRA_FAILURE
 
   if not options.patchset:
     options.patchset = properties['patchsets'][-1]
     print('No patchset specified. Using patchset %d' % options.patchset)
 
   issues_patchsets_to_apply = [(options.issue, options.patchset)]
-  depends_on_info = obj.get_depends_on_patchset(options.issue, options.patchset)
+  try:
+    depends_on_info = rietveld_obj.get_depends_on_patchset(
+        options.issue, options.patchset)
+  except urllib2.URLError:
+    logging.exception('failed to fetch depends_on_patchset')
+    return RETURN_CODE_INFRA_FAILURE
+
   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_info = rietveld_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
+    except urllib2.URLError:
+      logging.exception('failed to fetch dependency issue')
+      return RETURN_CODE_INFRA_FAILURE
 
   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)
+      patchset = rietveld_obj.get_patch(issue_to_apply, patchset_to_apply)
     except urllib2.HTTPError:
       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)
-      # Special code for bot_update to indicate that cause is network or
-      # Rietveld. Not 2, because 2 is returned on arg parsing failure.
-      return 3
+      # If we got this far, then this is likely missing patchset.
+      # Thus, it's not infra failure.
+      return RETURN_CODE_OTHERWISE
+    except urllib2.URLError:
+      logging.exception(
+          'Failed to fetch the patch for issue %d, patchset %d',
+          issue_to_apply, patchset_to_apply)
+      return RETURN_CODE_INFRA_FAILURE
     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)
@@ skipping 15 matching lines @@
       #    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
+      return RETURN_CODE_OTHERWISE
 
   if ('DEPS' in map(os.path.basename, patchset.filenames)
       and not options.ignore_deps):
     gclient_root = gclient_utils.FindGclientRoot(full_dir)
     if gclient_root and scm_type:
       print(
           'A DEPS file was updated inside a gclient checkout, running gclient '
           'sync.')
       gclient_path = os.path.join(BASE_DIR, 'gclient')
       if sys.platform == 'win32':
@@ skipping 17 matching lines @@
           annotated_gclient.emit_buildprops(revisions)
 
         return retcode
   return 0
 
 
 if __name__ == "__main__":
   fix_encoding.fix_encoding()
   try:
     sys.exit(main())
-  except urllib2.URLError:
-    # Weird flakiness of GAE, see http://crbug.com/537417
-    logging.exception('failed to fetch something from Rietveld')
-    sys.exit(3)
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
-    sys.exit(1)
+    sys.exit(RETURN_CODE_OTHERWISE)