upload SKP renderings that did not match expectations

upload SKP renderings that did not match expectations

BUG=skia:1942
Committed: https://skia.googlesource.com/buildbot/+/45fa087c24e3f6bab61bd951b4cb6280c0f4f2c6

[epoger, 2014-05-19 19:35:40]

Eric- PTAL at the change as a whole.

Ravi- PTAL at the specific permissions stuff (as I have called out with a
comment), and anything else that strikes you.

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/fla...
File slave/skia_slave_scripts/flavor_utils/default_build_step_utils.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/fla...
slave/skia_slave_scripts/flavor_utils/default_build_step_utils.py:122:
TODO(epoger): Clarify the description a bit: this method does not expect
Made this as a note to myself while working on the CL.  Let me know if you can
think of a good way this could be worded, and I can perhaps clarify behavior
with some comments here...

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/pos...
File slave/skia_slave_scripts/postrender.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/pos...
slave/skia_slave_scripts/postrender.py:31: def _Run(self):
It was more of an issue in prerender.py than here, but in both cases I found it
easier to follow if we were clearer about which step we were cleaning up after,
for each operation.

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/run...
File slave/skia_slave_scripts/run_gm.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/run...
slave/skia_slave_scripts/run_gm.py:23: # TODO(epoger): I think some of these
os.path.join() calls need to be
Am I right about this?  If so, I can make the change now if you want...

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/upl...
File slave/skia_slave_scripts/upload_rendered_skps.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/upl...
slave/skia_slave_scripts/upload_rendered_skps.py:17: # EPOGER: Before
committing, find out from Ravi how GS_* should be set.
Ravi- please weigh in on this.

We are uploading images of SKP test pages to Google Storage, and JSON summaries
(lists of image checksums) to skia-autogen.

My guess is that the images need to be invite-only, but it's OK for the
summaries to be world-readable.  Does that seem right?  And if so, have I set
the permissions properly here?

[borenet, 2014-05-19 21:04:38]

Looks fine overall, though I like "playback" much less than "skp".

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/bui...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/bui...
slave/skia_slave_scripts/build_step.py:206:
self._local_playback_dirs.PlaybackActualImagesDir())
So I never understood this - what's the deal with "playback"?  Why not name
these as "skp"-something, since that's what they are?

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/fla...
File slave/skia_slave_scripts/flavor_utils/android_build_step_utils.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/fla...
slave/skia_slave_scripts/flavor_utils/android_build_step_utils.py:179: prefix +
'playback_expected_summaries'),
Same question about naming.

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/pos...
File slave/skia_slave_scripts/postrender.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/pos...
slave/skia_slave_scripts/postrender.py:31: def _Run(self):
On 2014/05/19 19:35:40, epoger wrote:
> It was more of an issue in prerender.py than here, but in both cases I found
it
> easier to follow if we were clearer about which step we were cleaning up
after,
> for each operation.

It's possible that we should just have optional pre/post for each build step?

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/ren...
File slave/skia_slave_scripts/render_skps.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/ren...
slave/skia_slave_scripts/render_skps.py:82: }
It's possible that these belong in a config file in skia repo, like
bench_pictures.cfg.

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/run...
File slave/skia_slave_scripts/run_gm.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/run...
slave/skia_slave_scripts/run_gm.py:23: # TODO(epoger): I think some of these
os.path.join() calls need to be
On 2014/05/19 19:35:40, epoger wrote:
> Am I right about this?  If so, I can make the change now if you want...

Yeah, I think that's true.  I think we get lucky because we the only time they
*would* differ would be if we attached Android or ChromeOS devices to Windows
machines, which we don't do.

[epoger, 2014-05-19 22:07:23]

Thanks, Eric...

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/bui...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/bui...
slave/skia_slave_scripts/build_step.py:206:
self._local_playback_dirs.PlaybackActualImagesDir())
On 2014/05/19 21:04:38, borenet wrote:
> So I never understood this - what's the deal with "playback"?  Why not name
> these as "skp"-something, since that's what they are?

It was bugging me too as I wrote the CL, although by the time I'd been "soaking
in it" for a couple days I got over it.

Looks like that has been the case since these late-2012 CLs:
https://codereview.appspot.com/6750057/ ('Script to archive or replay webpages,
generate skp files and store both in Google Storage')
https://codereview.appspot.com/6817062 ('Building on the SkPicture replay
framework')

At any rate, how about I do the following AFTER this CL is committed?  (I can
add them as TODOs now)
1. Rename the build_step fields from self.playback_* to self.skp_*
2. Rename playback_dirs.py as skp_dirs.py, and change names within to follow

It gets a little tricky, because it affects not only render_skps.py but also
bench_pictures.py .  (Maybe we should pick one of "skps", "pictures", or
"replay" and stick with it?)

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/pos...
File slave/skia_slave_scripts/postrender.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/pos...
slave/skia_slave_scripts/postrender.py:31: def _Run(self):
On 2014/05/19 21:04:38, borenet wrote:
> On 2014/05/19 19:35:40, epoger wrote:
> > It was more of an issue in prerender.py than here, but in both cases I found
> it
> > easier to follow if we were clearer about which step we were cleaning up
> after,
> > for each operation.
> 
> It's possible that we should just have optional pre/post for each build step?

Do you mean...
1. Copying GM expectations to the device as part of the GenerateGMs step,
instead of as part of a separate PreRender step?  OR
2. Creating a PreGenerateGMs step that only prepares for GenerateGMs, and not
RenderSKPs/RunDecodingTests?

Downside of #1: if the copy fails, it looks like we failed while running gm
Downside of #2: ever more steps, slave script files

I prefer it as it is here: a limited number of slave scripts, but clarity as to
what step we are preparing for.

But I'm quite open to suggestion!  Feel free to grab me for live discussion
anytime on Tuesday.

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/ren...
File slave/skia_slave_scripts/render_skps.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/ren...
slave/skia_slave_scripts/render_skps.py:82: }
On 2014/05/19 21:04:38, borenet wrote:
> It's possible that these belong in a config file in skia repo, like
> bench_pictures.cfg.

Yeah, maybe, I'm not sure.  And maybe we should even share the configs with
bench_pictures?

Added a TODO.  Do you think it's something we should do as part of this same CL?

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/run...
File slave/skia_slave_scripts/run_gm.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/run...
slave/skia_slave_scripts/run_gm.py:23: # TODO(epoger): I think some of these
os.path.join() calls need to be
On 2014/05/19 21:04:38, borenet wrote:
> On 2014/05/19 19:35:40, epoger wrote:
> > Am I right about this?  If so, I can make the change now if you want...
> 
> Yeah, I think that's true.  I think we get lucky because we the only time they
> *would* differ would be if we attached Android or ChromeOS devices to Windows
> machines, which we don't do.

Done.

[rmistry, 2014-05-20 11:11:35]

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/upl...
File slave/skia_slave_scripts/upload_rendered_skps.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/upl...
slave/skia_slave_scripts/upload_rendered_skps.py:17: # EPOGER: Before
committing, find out from Ravi how GS_* should be set.
On 2014/05/19 19:35:40, epoger wrote:
> Ravi- please weigh in on this.
> 
> We are uploading images of SKP test pages to Google Storage, and JSON
summaries
> (lists of image checksums) to skia-autogen.
> 
> My guess is that the images need to be invite-only, but it's OK for the
> summaries to be world-readable.  Does that seem right?  And if so, have I set
> the permissions properly here?

Yes this looks correct. Stop by when you have a chance today, there is something
related to permissions here I would like to discuss.

[borenet, 2014-05-20 17:52:20]

LGTM

https://codereview.chromium.org/295753002/diff/40001/slave/skia_slave_scripts...
File slave/skia_slave_scripts/flavor_utils/default_build_step_utils.py (right):

https://codereview.chromium.org/295753002/diff/40001/slave/skia_slave_scripts...
slave/skia_slave_scripts/flavor_utils/default_build_step_utils.py:67: """Holds
SKP files that are consumed by RenderSKPs and BenchPictures."""
Why document this one but no others?

https://codereview.chromium.org/295753002/diff/40001/slave/skia_slave_scripts...
slave/skia_slave_scripts/flavor_utils/default_build_step_utils.py:232: In this
case, host_dirs and device_dirs are the same, which is why
"In the case of DefaultBuildStepUtils, ..."

[epoger, 2014-05-20 19:02:47]

Thanks, Eric.  Addressed your remaining comments in patchset 4.  Barring
objection, I will now work on committing the change.

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/bui...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.chromium.org/295753002/diff/1/slave/skia_slave_scripts/bui...
slave/skia_slave_scripts/build_step.py:206:
self._local_playback_dirs.PlaybackActualImagesDir())
On 2014/05/19 22:07:24, epoger wrote:
> On 2014/05/19 21:04:38, borenet wrote:
> > So I never understood this - what's the deal with "playback"?  Why not name
> > these as "skp"-something, since that's what they are?
> 
> It was bugging me too as I wrote the CL, although by the time I'd been
"soaking
> in it" for a couple days I got over it.
> 
> Looks like that has been the case since these late-2012 CLs:
> https://codereview.appspot.com/6750057/ ('Script to archive or replay
webpages,
> generate skp files and store both in Google Storage')
> https://codereview.appspot.com/6817062 ('Building on the SkPicture replay
> framework')
> 
> At any rate, how about I do the following AFTER this CL is committed?  (I can
> add them as TODOs now)
> 1. Rename the build_step fields from self.playback_* to self.skp_*
> 2. Rename playback_dirs.py as skp_dirs.py, and change names within to follow
> 
> It gets a little tricky, because it affects not only render_skps.py but also
> bench_pictures.py .  (Maybe we should pick one of "skps", "pictures", or
> "replay" and stick with it?)

Added a TODO above.

https://codereview.chromium.org/295753002/diff/40001/slave/skia_slave_scripts...
File slave/skia_slave_scripts/flavor_utils/default_build_step_utils.py (right):

https://codereview.chromium.org/295753002/diff/40001/slave/skia_slave_scripts...
slave/skia_slave_scripts/flavor_utils/default_build_step_utils.py:67: """Holds
SKP files that are consumed by RenderSKPs and BenchPictures."""
On 2014/05/20 17:52:20, borenet wrote:
> Why document this one but no others?

Documented the ones that I know well.

https://codereview.chromium.org/295753002/diff/40001/slave/skia_slave_scripts...
slave/skia_slave_scripts/flavor_utils/default_build_step_utils.py:232: In this
case, host_dirs and device_dirs are the same, which is why
On 2014/05/20 17:52:20, borenet wrote:
> "In the case of DefaultBuildStepUtils, ..."

Done.

[epoger, 2014-05-21 13:57:04]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-05-21 13:57:55]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/epoger@google.com/295753002/100001

[commit-bot: I haz the power, 2014-05-21 13:58:05]

Change committed as 45fa087c24e3f6bab61bd951b4cb6280c0f4f2c6

[epoger, 2014-05-21 14:11:06]

A revert of this CL has been created in
https://codereview.chromium.org/296003008/ by epoger@google.com.

The reason for reverting is: http://skbug.com/2582 ('PreRender and
UploadRenderedSKPs steps failing on some builders after my buildbot change').