gsutil: Decode output from subprocess.

Bug: 1007872, 1009819
Change-Id: Icb24badc0429012dc66912d7d35e03a456512787
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1894354Reviewed-by: Anthony Polito <apolito@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

PRESUBMIT.py

@@ -70,7 +70,6 @@ def CommonChecks(input_api, output_api, tests_to_black_list, run_on_python3):
     print('Warning: skipping most unit tests on Windows')
     tests_to_black_list = [
         r'.*auth_test\.py$',
-        r'.*download_from_google_storage_unittest\.py$',
         r'.*gclient_smoketest\.py$',
         r'.*git_cl_test\.py$',
         r'.*git_common_test\.py$',
@@ -82,7 +81,6 @@ def CommonChecks(input_api, output_api, tests_to_black_list, run_on_python3):
         r'.*roll_dep_test\.py$',
         r'.*scm_unittest\.py$',
         r'.*subprocess2_test\.py$',
-        r'.*upload_to_google_storage_unittest\.py$',
     ]
 
   # TODO(maruel): Make sure at least one file is modified first.

download_from_google_storage.py

@@ -114,14 +114,17 @@ class Gsutil(object):
         stderr=subprocess2.PIPE,
         env=self.get_sub_env())
 
+    out = out.decode('utf-8', 'replace')
+    err = err.decode('utf-8', 'replace')
+
     # Parse output.
-    status_code_match = re.search(b'status=([0-9]+)', err)
+    status_code_match = re.search('status=([0-9]+)', err)
     if status_code_match:
       return (int(status_code_match.group(1)), out, err)
-    if (b'You are attempting to access protected data with '
-        b'no configured credentials.' in err):
+    if ('You are attempting to access protected data with '
+        'no configured credentials.' in err):
       return (403, out, err)
-    if b'matched no objects' in err:
+    if 'matched no objects' in err:
       return (404, out, err)
     return (code, out, err)
 
@@ -267,9 +270,9 @@ def _downloader_worker_thread(thread_num, q, force, base_url,
       else:
         # Other error, probably auth related (bad ~/.boto, etc).
         out_q.put('%d> Failed to fetch file %s for %s, skipping. [Err: %s]' %
-                  (thread_num, file_url, output_filename, err.decode()))
+                  (thread_num, file_url, output_filename, err))
         ret_codes.put((1, 'Failed to fetch file %s for %s. [Err: %s]' %
-                       (file_url, output_filename, err.decode())))
+                       (file_url, output_filename, err)))
       continue
     # Fetch the file.
     out_q.put('%d> Downloading %s...' % (thread_num, output_filename))
@@ -282,8 +285,8 @@ def _downloader_worker_thread(thread_num, q, force, base_url,
             thread_num, output_filename))
     code, _, err = gsutil.check_call('cp', file_url, output_filename)
     if code != 0:
-      out_q.put('%d> %s' % (thread_num, err.decode()))
-      ret_codes.put((code, err.decode()))
+      out_q.put('%d> %s' % (thread_num, err))
+      ret_codes.put((code, err))
       continue
 
     remote_sha1 = get_sha1(output_filename)
@@ -336,11 +339,11 @@ def _downloader_worker_thread(thread_num, q, force, base_url,
     elif sys.platform != 'win32':
       # On non-Windows platforms, key off of the custom header
       # "x-goog-meta-executable".
-      code, out, _ = gsutil.check_call('stat', file_url)
+      code, out, err = gsutil.check_call('stat', file_url)
       if code != 0:
-        out_q.put('%d> %s' % (thread_num, err.decode()))
-        ret_codes.put((code, err.decode()))
-      elif re.search(r'executable:\s*1', out.decode()):
+        out_q.put('%d> %s' % (thread_num, err))
+        ret_codes.put((code, err))
+      elif re.search(r'executable:\s*1', out):
         st = os.stat(output_filename)
         os.chmod(output_filename, st.st_mode | stat.S_IEXEC)
 

gsutil.py

@@ -140,7 +140,8 @@ def run_gsutil(force_version, fallback, target, args, clean=False):
     # This script requires Windows Python, so invoke with depot_tools'
     # Python.
     def winpath(path):
-      return subprocess.check_output(['cygpath', '-w', path]).strip()
+      stdout = subprocess.check_output(['cygpath', '-w', path])
+      return stdout.strip().decode('utf-8', 'replace')
     cmd = ['python.bat', winpath(__file__)]
     cmd.extend(args)
     sys.exit(subprocess.call(cmd))

tests/download_from_google_storage_unittest.py

-#!/usr/bin/env python
+#!/usr/bin/env vpython3
 # 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.
@@ -71,7 +71,7 @@ class GsutilMock(object):
           fn()
         return code, out, err
       else:
-        return (0, b'', b'')
+        return (0, '', '')
 
   def check_call_with_retries(self, *args):
     return self.check_call(*args)
@@ -115,16 +115,19 @@ class GstoolsUnitTests(unittest.TestCase):
         self.assertTrue(
             download_from_google_storage._validate_tar_file(tar, tar_dir))
 
-      # Test no links.
-      tar_dir_link = 'for_tar_link'
-      os.makedirs(tar_dir_link)
-      link = os.path.join(tar_dir_link, 'link')
-      os.symlink(lorem_ipsum, link)
-      tar_with_links = 'with_links.tar.gz'
-      with tarfile.open(tar_with_links, 'w:gz') as tar:
-        tar.add(link)
-        self.assertFalse(
-            download_from_google_storage._validate_tar_file(tar, tar_dir_link))
+      # os.symlink doesn't exist on Windows.
+      if sys.platform != 'win32':
+        # Test no links.
+        tar_dir_link = 'for_tar_link'
+        os.makedirs(tar_dir_link)
+        link = os.path.join(tar_dir_link, 'link')
+        os.symlink(lorem_ipsum, link)
+        tar_with_links = 'with_links.tar.gz'
+        with tarfile.open(tar_with_links, 'w:gz') as tar:
+          tar.add(link)
+          self.assertFalse(
+              download_from_google_storage._validate_tar_file(
+                  tar, tar_dir_link))
 
       # Test not outside.
       tar_dir_outside = 'outside_tar'
@@ -158,8 +161,8 @@ class GstoolsUnitTests(unittest.TestCase):
     gsutil = download_from_google_storage.Gsutil(GSUTIL_DEFAULT_PATH, None)
     self.assertEqual(gsutil.path, GSUTIL_DEFAULT_PATH)
     code, _, err = gsutil.check_call()
-    self.assertEqual(code, 0)
-    self.assertEqual(err, b'')
+    self.assertEqual(code, 0, err)
+    self.assertEqual(err, '')
 
   def test_get_sha1(self):
     lorem_ipsum = os.path.join(self.base_path, 'lorem_ipsum.txt')
@@ -245,8 +248,8 @@ class DownloadTests(unittest.TestCase):
     sha1_hash = self.lorem_ipsum_sha1
     input_filename = '%s/%s' % (self.base_url, sha1_hash)
     output_filename = os.path.join(self.base_path, 'uploaded_lorem_ipsum.txt')
-    self.gsutil.add_expected(0, b'', b'')  # ls
-    self.gsutil.add_expected(0, b'', b'', lambda: shutil.copyfile(
+    self.gsutil.add_expected(0, '', '')  # ls
+    self.gsutil.add_expected(0, '', '', lambda: shutil.copyfile(
         self.lorem_ipsum, output_filename))  # cp
     self.queue.put((sha1_hash, output_filename))
     self.queue.put((None, None))
@@ -329,7 +332,7 @@ class DownloadTests(unittest.TestCase):
     self.queue.put((sha1_hash, output_filename))
     self.queue.put((None, None))
     stdout_queue = queue.Queue()
-    self.gsutil.add_expected(1, b'', b'')  # Return error when 'ls' is called.
+    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)
@@ -353,8 +356,8 @@ class DownloadTests(unittest.TestCase):
     sha1_hash = '7871c8e24da15bad8b0be2c36edc9dc77e37727f'
     input_filename = '%s/%s' % (self.base_url, sha1_hash)
     output_filename = os.path.join(self.base_path, 'uploaded_lorem_ipsum.txt')
-    self.gsutil.add_expected(0, b'', b'')  # ls
-    self.gsutil.add_expected(101, b'', b'Test error message.')
+    self.gsutil.add_expected(0, '', '')  # ls
+    self.gsutil.add_expected(101, '', 'Test error message.')
     code = download_from_google_storage.download_from_google_storage(
         input_filename=sha1_hash,
         base_url=self.base_url,

tests/upload_to_google_storage_unittest.py

-#!/usr/bin/env python
+#!/usr/bin/env vpython3
 # 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.
@@ -7,6 +7,7 @@
 
 import optparse
 import os
+import posixpath
 
 try:
   import Queue as queue
@@ -87,11 +88,12 @@ class UploadTests(unittest.TestCase):
       self.assertTrue(os.path.exists(tar_gz_file))
       with tarfile.open(tar_gz_file, 'r:gz') as tar:
         content = map(lambda x: x.name, tar.getmembers())
-        self.assertTrue(dirname in content)
-        self.assertTrue(os.path.join(dirname, 'subfolder_text.txt') in content)
-        self.assertTrue(
-            os.path.join(dirname, 'subfolder_text.txt.sha1') in content)
+        self.assertIn(dirname, content)
+        self.assertIn(posixpath.join(dirname, 'subfolder_text.txt'), content)
+        self.assertIn(
+            posixpath.join(dirname, 'subfolder_text.txt.sha1'), content)
 
+  @unittest.skipIf(sys.platform == 'win32', 'os.symlink does not exist on win')
   def test_validate_archive_dirs_fails(self):
     work_dir = os.path.join(self.base_path, 'download_test_data')
     with ChangedWorkingDirectory(work_dir):

upload_to_google_storage.py

@@ -84,7 +84,7 @@ def _upload_worker(
     if gsutil.check_call('ls', file_url)[0] == 0 and not force:
       # File exists, check MD5 hash.
       _, out, _ = gsutil.check_call_with_retries('ls', '-L', file_url)
-      etag_match = re.search(r'ETag:\s+([a-z0-9]{32})', out.decode())
+      etag_match = re.search(r'ETag:\s+([a-z0-9]{32})', out)
       if etag_match:
         remote_md5 = etag_match.group(1)
         # Calculate the MD5 checksum to match it to Google Storage's ETag.