Make download_from_google_storage.py print nothing in the happy case where it needs to do nothing.

The script prints a bunch of stuff behind `if verbose` checks, but then
"verbose" was turned on by default in
https://bugs.chromium.org/p/chromium/issues/detail?id=504452

I think the wrong lesson was learned from that bug – it sounds like the
problem was that an error message was printed only if verbose was set.
After this change, the script is silent when it does nothing, and prints
something if something happens. (Arguably, it still prints too much in the
case where it successfully downloads something.)

This is part of a few changes to make `gclient runhooks` less noisy.

Bug: 772741,504452
Change-Id: I5823c296abe97611ba4411bab2743673b10dca4b
Reviewed-on: https://chromium-review.googlesource.com/706915
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Ryan Tseng <hinoka@chromium.org>

download_from_google_storage.py

@@ -215,7 +215,7 @@ def _validate_tar_file(tar, prefix):
   return all(map(_validate, tar.getmembers()))
 
 def _downloader_worker_thread(thread_num, q, force, base_url,
-                              gsutil, out_q, ret_codes, verbose, extract,
+                              gsutil, out_q, ret_codes, extract,
                               delete=True):
   while True:
     input_sha1_sum, output_filename = q.get()
@@ -232,10 +232,6 @@ def _downloader_worker_thread(thread_num, q, force, base_url,
     if os.path.exists(output_filename) and not force:
       if not extract or os.path.exists(extract_dir):
         if get_sha1(output_filename) == input_sha1_sum:
-          if verbose:
-            out_q.put(
-                '%d> File %s exists and SHA1 matches. Skipping.' % (
-                    thread_num, output_filename))
           continue
     # Check if file exists.
     file_url = '%s/%s' % (base_url, input_sha1_sum)
@@ -326,18 +322,26 @@ def _downloader_worker_thread(thread_num, q, force, base_url,
         st = os.stat(output_filename)
         os.chmod(output_filename, st.st_mode | stat.S_IEXEC)
 
-def printer_worker(output_queue):
-  while True:
-    line = output_queue.get()
-    # Its plausible we want to print empty lines.
-    if line is None:
-      break
-    print line
+
+class PrinterThread(threading.Thread):
+  def __init__(self, output_queue):
+    super(PrinterThread, self).__init__()
+    self.output_queue = output_queue
+    self.did_print_anything = False
+
+  def run(self):
+    while True:
+      line = self.output_queue.get()
+      # It's plausible we want to print empty lines: Explicit `is None`.
+      if line is None:
+        break
+      self.did_print_anything = True
+      print line
 
 
 def download_from_google_storage(
     input_filename, base_url, gsutil, num_threads, directory, recursive,
-    force, output, ignore_errors, sha1_file, verbose, auto_platform, extract):
+    force, output, ignore_errors, sha1_file, auto_platform, extract):
   # Start up all the worker threads.
   all_threads = []
   download_start = time.time()
@@ -349,11 +353,11 @@ def download_from_google_storage(
     t = threading.Thread(
         target=_downloader_worker_thread,
         args=[thread_num, work_queue, force, base_url,
-              gsutil, stdout_queue, ret_codes, verbose, extract])
+              gsutil, stdout_queue, ret_codes, extract])
     t.daemon = True
     t.start()
     all_threads.append(t)
-  printer_thread = threading.Thread(target=printer_worker, args=[stdout_queue])
+  printer_thread = PrinterThread(stdout_queue)
   printer_thread.daemon = True
   printer_thread.start()
 
@@ -376,10 +380,9 @@ def download_from_google_storage(
     max_ret_code = max(ret_code, max_ret_code)
     if message:
       print >> sys.stderr, message
-  if verbose and not max_ret_code:
-    print 'Success!'
 
-  if verbose:
+  # Only print summary if any work was done.
+  if printer_thread.did_print_anything:
     print 'Downloading %d files took %1f second(s)' % (
         work_queue_size, time.time() - download_start)
   return max_ret_code
@@ -535,7 +538,7 @@ def main(args):
   return download_from_google_storage(
       input_filename, base_url, gsutil, options.num_threads, options.directory,
       options.recursive, options.force, options.output, options.ignore_errors,
-      options.sha1_file, options.verbose, options.auto_platform,
+      options.sha1_file, options.auto_platform,
       options.extract)
 
 

tests/download_from_google_storage_unittests.py

@@ -230,7 +230,7 @@ class DownloadTests(unittest.TestCase):
     stdout_queue = Queue.Queue()
     download_from_google_storage._downloader_worker_thread(
         0, self.queue, False, self.base_url, self.gsutil,
-        stdout_queue, self.ret_codes, True, False)
+        stdout_queue, self.ret_codes, True)
     expected_calls = [
         ('check_call',
             ('ls', input_filename)),
@@ -256,7 +256,7 @@ class DownloadTests(unittest.TestCase):
     stdout_queue = Queue.Queue()
     download_from_google_storage._downloader_worker_thread(
         0, self.queue, False, self.base_url, self.gsutil,
-        stdout_queue, self.ret_codes, True, False)
+        stdout_queue, self.ret_codes, True)
     expected_output = [
         '0> File %s exists and SHA1 matches. Skipping.' % output_filename
     ]
@@ -278,7 +278,7 @@ class DownloadTests(unittest.TestCase):
     stdout_queue = Queue.Queue()
     download_from_google_storage._downloader_worker_thread(
         0, self.queue, True, self.base_url, self.gsutil,
-        stdout_queue, self.ret_codes, True, True, delete=False)
+        stdout_queue, self.ret_codes, True, delete=False)
     expected_calls = [
         ('check_call',
             ('ls', input_filename)),
@@ -311,7 +311,7 @@ class DownloadTests(unittest.TestCase):
     self.gsutil.add_expected(1, '', '')  # Return error when 'ls' is called.
     download_from_google_storage._downloader_worker_thread(
         0, self.queue, False, self.base_url, self.gsutil,
-        stdout_queue, self.ret_codes, True, False)
+        stdout_queue, self.ret_codes, True)
     expected_output = [
         '0> Failed to fetch file %s for %s, skipping. [Err: ]' % (
             input_filename, output_filename),
@@ -345,7 +345,6 @@ class DownloadTests(unittest.TestCase):
         output=output_filename,
         ignore_errors=False,
         sha1_file=False,
-        verbose=True,
         auto_platform=False,
         extract=False)
     expected_calls = [
@@ -377,7 +376,7 @@ class DownloadTests(unittest.TestCase):
     self.gsutil.add_expected(0, '', '')
     self.gsutil.add_expected(0, '', '', _write_bad_file)
     download_from_google_storage._downloader_worker_thread(
-        1, q, True, self.base_url, self.gsutil, out_q, ret_codes, True, False)
+        1, q, True, self.base_url, self.gsutil, out_q, ret_codes, True)
     self.assertTrue(q.empty())
     msg = ('1> ERROR remote sha1 (%s) does not match expected sha1 (%s).' %
            ('8843d7f92416211de9ebb963ff4ce28125932878', sha1_hash))
@@ -403,7 +402,6 @@ class DownloadTests(unittest.TestCase):
         output=None,
         ignore_errors=False,
         sha1_file=False,
-        verbose=True,
         auto_platform=False,
         extract=False)
     expected_calls = [

upload_to_google_storage.py

@@ -18,7 +18,7 @@ import time
 
 from download_from_google_storage import get_sha1
 from download_from_google_storage import Gsutil
-from download_from_google_storage import printer_worker
+from download_from_google_storage import PrinterThread
 from download_from_google_storage import GSUTIL_DEFAULT_PATH
 
 USAGE_STRING = """%prog [options] target [target2 ...].
@@ -145,7 +145,7 @@ def upload_to_google_storage(
   upload_queue = Queue.Queue()
   upload_timer = time.time()
   stdout_queue = Queue.Queue()
-  printer_thread = threading.Thread(target=printer_worker, args=[stdout_queue])
+  printer_thread = PrinterThread(stdout_queue)
   printer_thread.daemon = True
   printer_thread.start()
   for thread_num in range(num_threads):