git cl try-results: add --wait-till-finished option.

git_cl.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.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld and Gerrit."""
 
 from __future__ import print_function
@@ skipping 136 matching lines @@
   return not code
 
 
 def time_sleep(seconds):
   # Use this so that it can be mocked in tests without interfering with python
   # system machinery.
   import time  # Local import to discourage others from importing time globally.
   return time.sleep(seconds)
 
 
+def time_time():
+  # Use this so that it can be mocked in tests without interfering with python
+  # system machinery.
+  import time  # Local import to discourage others from importing time globally.
+  return time.time()
+
 def ask_for_data(prompt):
   try:
     return raw_input(prompt)
   except KeyboardInterrupt:
     # Hide the exception.
     sys.exit(1)
 
 
 def _git_branch_config_key(branch, key):
   """Helper method to return Git config key for a branch."""
@@ skipping 284 matching lines @@
     content = _buildbucket_retry('fetching try jobs', http, url, 'GET')
     for build in content.get('builds', []):
       builds[build['id']] = build
     if 'next_cursor' in content:
       params['start_cursor'] = content['next_cursor']
     else:
       break
   return builds
 
 
+def _count_unfinished_tryjobs(jobs):
+  """Returns number of unfinished jobs from fetch_try_jobs result."""
+  return sum(b['status'] != 'COMPLETED' for b in jobs.itervalues())

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

perhaps 

len(b for b in jobs.itervalues() if b['status'] != 'COMPLETED')

is better than hiding implicit bool-to-int conversion here.

+
+
 def print_try_jobs(options, builds):
   """Prints nicely result of fetch_try_jobs."""
   if not builds:
     print('No try jobs scheduled')
     return
 
   # Make a copy, because we'll be modifying builds dictionary.
   builds = builds.copy()
   builder_names_cache = {}
 
@@ skipping 4424 matching lines @@
   group.add_option(
       '--print-master', action='store_true', help='print master name as well.')
   group.add_option(
       '--color', action='store_true', default=setup_color.IS_TTY,
       help='force color output, useful when piping output.')
   group.add_option(
       '--buildbucket-host', default='cr-buildbucket.appspot.com',
       help='Host of buildbucket. The default host is %default.')
   group.add_option(
       '--json', help='Path of JSON output file to write try job results to.')
+  group.add_option(
+      '-w', '--wait-till-finished', action='store_true', default=False,
+      help='Keep checking buildbucket until either all jobs finish. '

[Michael Achenbach, 2016/10/20 09:40:35]

nit: remove "either"?

+           'If after 1 hour jobs are still running, aborts and returns error '
+           'code 3.')
   parser.add_option_group(group)
   auth.add_auth_options(parser)
   options, args = parser.parse_args(args)
   if args:
     parser.error('Unrecognized args: %s' % ' '.join(args))
 
   auth_config = auth.extract_auth_config_from_options(options)
   cl = Changelist(auth_config=auth_config)
   if not cl.GetIssue():
     parser.error('Need to upload first')
 
   patchset = options.patchset
   if not patchset:
     patchset = cl.GetMostRecentPatchset()
     if not patchset:
       parser.error('Codereview doesn\'t know about issue %s. '
                    'No access to issue or wrong issue number?\n'
                    'Either upload first, or pass --patchset explicitely' %
                    cl.GetIssue())
 
     if patchset != cl.GetPatchset():
       print('Warning: Codereview server has newer patchsets (%s) than most '
             'recent upload from local checkout (%s). Did a previous upload '
             'fail?\n'
             'By default, git cl try-results uses the latest patchset from '
             'codereview, continuing to use patchset %s.\n' %
             (patchset, cl.GetPatchset(), patchset))
-  try:
-    jobs = fetch_try_jobs(auth_config, cl, options.buildbucket_host, patchset)
-  except BuildbucketResponseException as ex:
-    print('Buildbucket error: %s' % ex)
-    return 1
-  if options.json:
-    write_try_results_json(options.json, jobs)
-  else:
-    print_try_jobs(options, jobs)
+
+  start_time = time_time()
+  while True:
+    try:
+      jobs = fetch_try_jobs(auth_config, cl, options.buildbucket_host, patchset)
+    except BuildbucketResponseException as ex:
+      print('Buildbucket error: %s' % ex)
+      return 1
+    if options.json:
+      write_try_results_json(options.json, jobs)

[Michael Achenbach, 2016/10/20 09:40:35]

Does this need to write the json in the loop? How about just once in the end?

Also: This prints everything in a loop, maybe only the remaining ones should be
printed? Or maybe use carriage return to keep modifying the same lines?

+    else:
+      print_try_jobs(options, jobs)
+    if not options.wait_till_finished:
+      return 0

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

Please "break", so that if someone chooses to add something later in this func,
then it will be executed

+
+    # Only if --wait-till-finished is specified.
+    unfinished = _count_unfinished_tryjobs(jobs)
+    if not unfinished:
+      return 0

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

ditto

+    passed = time_time() - start_time
+    print('After %.0f seconds waiting, %d jobs still running\n\n' %

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

Juse use %d instead of %.0f. It will round it to the next it... it's not the
same because %.0f will take floor operation, but AFAIK this doesn't matter here
and %d is simpler to understand.

+          (passed, unfinished))
+    if passed > 3600:

[Michael Achenbach, 2016/10/20 09:40:35]

Do you need a limit at all? Is this tool supposed to be usable by automated
infrastructure? If it's just intended for users maybe stay without limit and be
robust to using Ctrl-C?

[Sergiy Byelozyorov, 2016/10/20 11:44:43]

IMHO, this is useful to prevent users forgetting that they are running their
command in some screen shell and having it run for days causing unneeded load on
buildbucket.

+      print('Ran for too long, aborting now.')
+      return 3
+    time_sleep(10)  # Limit unnecessary load on buildbucket.

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

If the option becomes widely popular, we may have a lot of developers running
this in parallel. A 1000 concurrently running commands with this option will
cause 100 requests per second. But OTH, IMHO paying for 0.1 requests per second
per developer is good value for the money, so this is probably reasonable. We
can probably revisit this later if it does indeed become that popular.

   return 0
 
 
 @subcommand.usage('[new upstream branch]')
 def CMDupstream(parser, args):
   """Prints or sets the name of the upstream branch, if any."""
   _, args = parser.parse_args(args)
   if len(args) > 1:
     parser.error('Unrecognized args: %s' % ' '.join(args))
 
@@ skipping 428 matching lines @@
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   setup_color.init()
   try:
     sys.exit(main(sys.argv[1:]))
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)