add ACL-setting code to upload_rendered_skps.py

slave/skia_slave_scripts/upload_rendered_skps.py

Index: slave/skia_slave_scripts/upload_rendered_skps.py
diff --git a/slave/skia_slave_scripts/upload_rendered_skps.py b/slave/skia_slave_scripts/upload_rendered_skps.py
index 36aec879325380433bf4fcf522cec11f366a8ee5..1e812133243f4ccae73e453f4f1cffe97287eb96 100644
--- a/slave/skia_slave_scripts/upload_rendered_skps.py
+++ b/slave/skia_slave_scripts/upload_rendered_skps.py
@@ -9,10 +9,13 @@ import os
 import posixpath
 import sys
 
-from build_step import BuildStep, PLAYBACK_CANNED_ACL
-from utils import old_gs_utils as gs_utils
+from build_step import BuildStep, BOTO_CREDENTIALS_FILE
+from build_step import PLAYBACK_CANNED_ACL, PLAYBACK_FINEGRAINED_ACL_LIST
+from py.utils import gs_utils
+from utils import old_gs_utils
 import skia_vars
 
+GS_PREFIX = 'gs://'

[borenet, 2014/07/21 14:07:47]

This could go into gs_utils as it may be useful elsewhere.

 GS_SUMMARIES_BUCKET = 'gs://chromium-skia-skp-summaries'
 SUBDIR_NAME = 'rendered-skps'
 
@@ -30,20 +33,21 @@ class UploadRenderedSKPs(BuildStep):
     # and use it here so we don't re-upload image files we already have
     # in Google Storage.
     #
-    # TODO(epoger): Change ACLs of files uploaded to Google Storage to
-    # google.com:READ .  See _SetGoogleReadACLs() in
-    # https://skia.googlesource.com/buildbot/+/master/slave/skia_slave_scripts/
-    #         webpages_playback.py
+    gs_bucket = skia_vars.GetGlobalVariable('googlestorage_bucket')

[rmistry, 2014/07/21 13:56:52]

In global_variables, gm_summaries_bucket is "chromium-skia-gm-summaries" and
googlestorage_bucket is "gs://chromium-skia-gm", to me it makes sense to have
the values have only the names of the bucket and the prefix should not have to
be specified. This will simplify callers who will assume that the bucket names
will not contain prefixes.

If you do change that then you will have to update all references that call
googlestorage_bucket and this may be be out of the scope of this CL, leaving the
decision up to you.

[epoger, 2014/07/21 20:01:32]

Yes, I agree that it would be better for those bucket names in global_variables
to be consistent.

But I think that change will be a bit tricky to stage (due to version skew
between repos in production), so I'd like to tackle that in a separate CL.

For now, I have created the with_gs_prefix() and without_gs_prefix() methods in
gs_utils.py so that we can safely use these global variables whether they
contain the prefix or not.

+    if gs_bucket.startswith(GS_PREFIX):
+      gs_bucket = gs_bucket[len(GS_PREFIX):]
+    gs = gs_utils.GSUtils(BOTO_CREDENTIALS_FILE)
+
     src_dir = os.path.abspath(self.playback_actual_images_dir)
     if os.listdir(src_dir):
-      dest_dir = posixpath.join(
-          skia_vars.GetGlobalVariable('googlestorage_bucket'), SUBDIR_NAME)
-      print 'Uploading image files from %s to %s.' % (src_dir, dest_dir)
-      gs_utils.upload_dir_contents(
-          local_src_dir=src_dir, remote_dest_dir=dest_dir,
-          gs_acl=PLAYBACK_CANNED_ACL)
+      print 'Uploading image files from %s to bucket=%s, dir=%s' % (
+          src_dir, gs_bucket, SUBDIR_NAME)
+      gs.upload_dir_contents(
+          source_dir=src_dir, dest_bucket=gs_bucket, dest_dir=SUBDIR_NAME,
+          predefined_acl=PLAYBACK_CANNED_ACL,
+          fine_grained_acl_list=PLAYBACK_FINEGRAINED_ACL_LIST)
     else:
-      print ('No image files in %s, so skipping upload to Google Storage.' %
+      print ('Skipping upload to Google Storage, because no image files in %s' %
              src_dir)
 
     # Upload image summaries (checksums) to Google Storage.
@@ -56,12 +60,20 @@ class UploadRenderedSKPs(BuildStep):
       for filename in filenames:
         src_path = os.path.join(src_dir, filename)
         dest_path = posixpath.join(dest_dir, filename)
-        gs_utils.upload_file(
+        # It's important to only upload the summary file when it has changed,
+        # because we use the history of the file in Google Storage to tell us
+        # when any of the results changed.
+        #
+        # TODO(epoger): Once gs_utils.upload_file() supports only_if_modified
+        # parameter, start using it, so we can set fine_grained_acl_list like
+        # we do above... we'll need that for google.com users to be able to
+        # download the summary files.
+        old_gs_utils.upload_file(
             local_src_path=src_path, remote_dest_path=dest_path,
             gs_acl=PLAYBACK_CANNED_ACL, only_if_modified=True)
     else:
-      print ('No image summaries in %s, so skipping upload to Google Storage.' %
-             src_dir)
+      print ('Skipping upload to Google Storage, because no image summaries '
+             'in %s' % src_dir)
 
 if '__main__' == __name__:
   sys.exit(BuildStep.RunBuildStep(UploadRenderedSKPs))