From 87677e9830fec827c48d57d5dc764e0320282585 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 9 Oct 2017 15:42:44 -0400 Subject: [PATCH] Make download_from_google_storage.py print nothing in the happy case where it needs to do nothing. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Dirk Pranke Reviewed-by: Ryan Tseng --- download_from_google_storage.py | 41 ++++++++++--------- .../download_from_google_storage_unittests.py | 12 +++--- upload_to_google_storage.py | 4 +- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/download_from_google_storage.py b/download_from_google_storage.py index c9f5f676c..5c4f677d6 100755 --- a/download_from_google_storage.py +++ b/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) diff --git a/tests/download_from_google_storage_unittests.py b/tests/download_from_google_storage_unittests.py index d2ed8a8e1..27c0f7f6c 100755 --- a/tests/download_from_google_storage_unittests.py +++ b/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 = [ diff --git a/upload_to_google_storage.py b/upload_to_google_storage.py index e53cef026..031214d6a 100755 --- a/upload_to_google_storage.py +++ b/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):