add ACL-setting code to upload_rendered_skps.py

slave/skia_slave_scripts/build_step.py

 # Copyright (c) 2013 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.
 
 """Base class for all slave-side build steps. """
 
 import config
 # pylint: disable=W0611
 import flavor_utils
 import imp
@@ skipping 15 matching lines @@
 # Add important directories to the PYTHONPATH
 sys.path.append(os.path.join(BUILDBOT_PATH))
 sys.path.append(os.path.join(BUILDBOT_PATH, 'site_config'))
 sys.path.append(os.path.join(BUILDBOT_PATH, 'master'))
 sys.path.insert(0, os.path.join(BUILDBOT_PATH, 'common'))
 
 import builder_name_schema
 import slave_hosts_cfg
 import slaves_cfg
 
+from py.utils import gs_utils
 from py.utils import misc
 
 
 DEFAULT_TIMEOUT = 4800
 DEFAULT_NO_OUTPUT_TIMEOUT = 3600
 DEFAULT_NUM_CORES = 2
 
 
 GM_EXPECTATIONS_FILENAME = 'expected-results.json'
 GM_IGNORE_FAILURES_FILE = 'ignored-tests.txt'
 
 
 # multiprocessing.Value doesn't accept boolean types, so we have to use an int.
 INT_TRUE = 1
 INT_FALSE = 0
 build_step_stdout_has_written = multiprocessing.Value('i', INT_FALSE)
 
 
-# The canned acl to use while copying playback files to Google Storage.
-PLAYBACK_CANNED_ACL = 'private'
+# The canned acl to use while copying playback (SKP) files to Google Storage.
+# They should not be world-readable!
+PLAYBACK_CANNED_ACL = gs_utils.PREDEFINED_ACL_PRIVATE
+PLAYBACK_FINEGRAINED_ACL_LIST = [
+    (gs_utils.ID_TYPE_GROUP_BY_DOMAIN, 'google.com', gs_utils.PERMISSION_READ),
+]

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

I would strongly prefer that these go into gs_utils instead of build_step.

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

Given that the buildbots write SKP files into Google Storage, but individual
developers (i.e. users of the Skia repo) never do, it seems like these
configurations should live within the buildbot repo, not common.

So I can think of 3 options.  I will happily do any of them; you tell me which
one you want:
1. Move these constants into gs_utils.py in the common repo.
2. Define them within upload_rendered_skps.py, and any other build steps that
need them import them from there.
3. Define them within a new shared module in the buildbot repo
("skp_playback.py"?)
4. Something else you come up with

Also, would you like me to leave BOTO_CREDENTIALS_FILE defined here or move it
elsewhere?  (Again, I think it's buildbot-specific, not appropriate for the
common repo)

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

I like #1 and #3 (though I really don't like the "playback" term; it confuses me
for some reason), and possibly another option would be to create a new
gs_utils.py in skia-buildbot which just imports * from gs_utils in common repo
and adds whatever buildbot-specific stuff we want.

And really, it's not a crime to have variables shared by multiple build steps in
this file.  I just get nervous every time this file gets touched because of the
potential for disaster.

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

Cool idea.  I went with it... in order to do it without "import * from gs_utils"
(which is considered dangerous) I had to modify the structure of common's
gs_utils.py in https://codereview.chromium.org/405083005/ ('use better mechanism
for enums in gs_utils.py')

+BOTO_CREDENTIALS_FILE = os.path.join(
+    BUILDBOT_PATH, 'third_party', 'chromium_buildbot', 'site_config', '.boto')

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

This is where we already put the .boto file on the bots... we can change this
path if we want, once old_gs_utils.py is gone.  (Unless we need the
chromium_buildbot code to use these same credentials???)

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

If it's okay with you, let's leave it as-is, since it'll need to be there
eventually when we're in sync with upstream code.

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

Now, users of gs_utils.py within buildbot automatically get the credentials from
this filepath, unless they ask for something different.

 
 
 class BuildStepWarning(Exception):
   pass
 
 
 class BuildStepFailure(Exception):
   pass
 
 
@@ skipping 326 matching lines @@
         else:
           raise BuildStepFailure('Build step failed.')
       except Exception:
         print traceback.format_exc()
         if attempt + 1 >= step.attempts:
           raise
       # pylint: disable=W0212
       step._WaitFunc(attempt)
       attempt += 1
       print '**** %s, attempt %d ****' % (StepType.__name__, attempt + 1)