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..e3e301ccefc5d3482b23312e80e4792cca694a35 100644
--- a/slave/skia_slave_scripts/upload_rendered_skps.py
+++ b/slave/skia_slave_scripts/upload_rendered_skps.py
@@ -9,8 +9,10 @@ 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_SUMMARIES_BUCKET = 'gs://chromium-skia-skp-summaries'
@@ -30,20 +32,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')

[epoger, 2014/07/18 21:10:25]

Actually, I guess now (before we start uploading these rendered SKP images) is a
good time to ask: Should we create a new bucket specifically for them, rather
than uploading them into gs://chromium-skia-gm/rendered-skps ?

I think a separate bucket is better.  (It allows us to set bucket
permissions/configs that are appropriate for each use.)  What do you think?

Shall I ask to have a new "chromium-skia-skp-images" bucket created for this
purpose?  (Consistent with the "'chromium-skia-skp-summaries" bucket we had
created for the summary files.)

[borenet, 2014/07/18 22:11:12]

Sounds good to me, although I don't know why we need the chromium prefix.

[epoger, 2014/07/21 13:42:51]

I don't know either, but hinoka@ asked us to in http://crbug.com/372905 ('please
create 2 new Google Storage buckets for Skia team (or can I just do it
myself?)').

Filed http://crbug.com/395615 ('please create 2 new Google Storage buckets for
Skia team') to get this new bucket created.

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

Since these buckets are created under the c-i-t umbrella and likely billed
through them, it makes sense that they have the chromium prefix.

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

Rules is rules I guess.

+    if gs_bucket.startswith('gs://'):
+      gs_bucket = gs_bucket[5:]

[borenet, 2014/07/18 22:11:12]

GS_PREFIX = 'gs://'
if gs_bucket.startswith(GS_PREFIX):
  gs_bucket = gs_bucket[len(GS_PREFIX):]

[epoger, 2014/07/21 13:42:51]

Done.

+    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 +59,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))