Building on the SkPicture replay framework

* Improved the webpages_playback.py script. Made it simplier, more modular and moved it up a level.

* Created the following new BuildSteps:
  ** RenderWebpagePictures: It downloads skp files from Google Storage, runs render_pictures and stores images back to Google Storage.
  ** CompareWebpageGMs.
  ** BenchWebpagePictures.
  ** UploadWebpagePictureBenchResults.
  ** GenerateWebpagePictureBenchGraphs.
  ** UploadWebpagePictureBenchGraphs.

* Added utility modules- file_utils and gs_utils.

* Added run_unittests that runs all unit tests below the slaves directory. Added unit tests for file_utils and gs_utils.

* If gm-actual does not exist for a builder then images from gm-expected are copied there.

* If there are not enough revisions to generate graphs with, then the exception is caught and the empty graphs are not uploaded.
Committed: https://code.google.com/p/skia/source/detail?r=6366

[EricB, 2012-11-06 20:25:33]

This seems to be duplicating a lot of the slave-side steps.  I'm sort of
confused on why we need to do this, since all of our skp's come from webpages.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ben...
File slave/skia_slave_scripts/bench_webpage_pictures.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ben...
slave/skia_slave_scripts/bench_webpage_pictures.py:6: """Runs the bench_pictures
executable on skp files from archived webpages.
I'm a little confused.  Isn't this what we always want to do with
bench_pictures?

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/bui...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/bui...
slave/skia_slave_scripts/build_step.py:33: RELATIVE_PLAYBACK_SKP_DIR =
os.path.join(ROOT_PLAYBACK_DIR_NAME, 'skpictures')
Does Google Storage use posix-style paths?  If so, will this cause problems on
Windows?

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/bui...
slave/skia_slave_scripts/build_step.py:151: local_playback_base_dir,
self._relative_playback_gm_expected_dir)
It may be worth looking at how we organize directories for Android in
android_build_step.py.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/com...
File slave/skia_slave_scripts/compare_webpage_gms.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/com...
slave/skia_slave_scripts/compare_webpage_gms.py:6: """Compares the GM images
from archived webpages to the baselines.
Again, I not sure that we need a separate step for this.  I believe that skdiff
will work recursively, so it *should* work as it stands even if we put the skp
output in a subdirectory.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/gen...
File slave/skia_slave_scripts/generate_webpage_picture_bench_graphs.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/gen...
slave/skia_slave_scripts/generate_webpage_picture_bench_graphs.py:6: """Generate
performance graphs from bench output of archived webpages.
Again, this seems like a duplicate to me.  Maybe we should meet to discuss why
we need to do this?

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/uti...
File slave/skia_slave_scripts/utils/gs_utils.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/uti...
slave/skia_slave_scripts/utils/gs_utils.py:15: Returns True if deleted
successfully else returns False
Most of our slave-side scripts use the convention of returning nothing but
throwing an exception on failure. It's probably a good thing to be consistent
throughout.

[rmistry, 2012-11-06 20:40:20]

Addressed only the high level design comments in this round.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ben...
File slave/skia_slave_scripts/bench_webpage_pictures.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ben...
slave/skia_slave_scripts/bench_webpage_pictures.py:6: """Runs the bench_pictures
executable on skp files from archived webpages.
On 2012/11/06 20:25:33, EricB wrote:
> I'm a little confused.  Isn't this what we always want to do with
> bench_pictures?

Yes, this (and the other 'webpage' BuildSteps) will replace the existing
BuildSteps after we know everything is working as expected (I plan to turn it on
different Builders slowly maybe starting with Linux and then Windows). I am
currently subclassing the existing steps to reuse code so I do not have to
duplicate everything.  I wanted to leave the current framework intact and
untouched especially because of the current situation.

Perhaps my naming of the new BuildSteps is confusing, if you have any
suggestions please let me know.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/com...
File slave/skia_slave_scripts/compare_webpage_gms.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/com...
slave/skia_slave_scripts/compare_webpage_gms.py:6: """Compares the GM images
from archived webpages to the baselines.
On 2012/11/06 20:25:33, EricB wrote:
> Again, I not sure that we need a separate step for this.  I believe that
skdiff
> will work recursively, so it *should* work as it stands even if we put the skp
> output in a subdirectory.

The skp output is in a completely different directory in this BuildStep, to
leave the current framework unchanged.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/gen...
File slave/skia_slave_scripts/generate_webpage_picture_bench_graphs.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/gen...
slave/skia_slave_scripts/generate_webpage_picture_bench_graphs.py:6: """Generate
performance graphs from bench output of archived webpages.
On 2012/11/06 20:25:33, EricB wrote:
> Again, this seems like a duplicate to me.  Maybe we should meet to discuss why
> we need to do this?

See above comments.

[EricB, 2012-11-06 21:54:22]

I think this looks good for the most part.  I guess we'll switch to the new
scripts one builder/platform at time and go from there.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ben...
File slave/skia_slave_scripts/bench_webpage_pictures.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ben...
slave/skia_slave_scripts/bench_webpage_pictures.py:6: """Runs the bench_pictures
executable on skp files from archived webpages.
On 2012/11/06 20:40:20, rmistry wrote:
> On 2012/11/06 20:25:33, EricB wrote:
> > I'm a little confused.  Isn't this what we always want to do with
> > bench_pictures?
> 
> Yes, this (and the other 'webpage' BuildSteps) will replace the existing
> BuildSteps after we know everything is working as expected (I plan to turn it
on
> different Builders slowly maybe starting with Linux and then Windows). I am
> currently subclassing the existing steps to reuse code so I do not have to
> duplicate everything.  I wanted to leave the current framework intact and
> untouched especially because of the current situation.
> 
> Perhaps my naming of the new BuildSteps is confusing, if you have any
> suggestions please let me know.

Gotcha.  Without knowing that plan, I assumed that you intended to have both
sets of build steps running indefinitely.  My only concern here is that, once
committed, this will nearly double the buildbot cycle time until we merge the
steps again.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/com...
File slave/skia_slave_scripts/compare_webpage_gms.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/com...
slave/skia_slave_scripts/compare_webpage_gms.py:20: --dest_gsbase ""
Thanks for including this. It's entirely unclear how to run these buildsteps
otherwise.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/gen...
File slave/skia_slave_scripts/generate_bench_graphs.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/gen...
slave/skia_slave_scripts/generate_bench_graphs.py:76: print 'Not enough
revisions created yet to generate graphs with!'
Can we verify that this is the reason for failure?  Also, we should throw
BuildStepWarning (from module build_step).

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ren...
File slave/skia_slave_scripts/render_webpage_pictures.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ren...
slave/skia_slave_scripts/render_webpage_pictures.py:116:
file_utils.CreateCleanLocalDir(self._local_playback_skp_dir)
This new utility should be used everywhere applicable.  Doesn't need to be in
this change, but we should update the other slave scripts to use it.

[rmistry, 2012-11-08 13:04:03]

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/bui...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/bui...
slave/skia_slave_scripts/build_step.py:33: RELATIVE_PLAYBACK_SKP_DIR =
os.path.join(ROOT_PLAYBACK_DIR_NAME, 'skpictures')
On 2012/11/06 20:25:33, EricB wrote:
> Does Google Storage use posix-style paths?  If so, will this cause problems on
> Windows?

Good catch, yes it will. Fixed this. Tested it locally on linux (but not on
windows).

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/bui...
slave/skia_slave_scripts/build_step.py:151: local_playback_base_dir,
self._relative_playback_gm_expected_dir)
On 2012/11/06 20:25:33, EricB wrote:
> It may be worth looking at how we organize directories for Android in
> android_build_step.py.

Hmm, that is an interesting pattern. Trying it out for all the new playback
directories.
Also, looks like I will need separate subclasses for android and chromeos but I
will do that when I get there.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/gen...
File slave/skia_slave_scripts/generate_bench_graphs.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/gen...
slave/skia_slave_scripts/generate_bench_graphs.py:76: print 'Not enough
revisions created yet to generate graphs with!'
On 2012/11/06 21:54:22, EricB wrote:
> Can we verify that this is the reason for failure?  Also, we should throw
> BuildStepWarning (from module build_step).

Yea I tried to verify but it just throws a generic Exception which is why I am
assuming that it will only fail when there are not enough revisions created yet.
Done BuildStepWarning.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ren...
File slave/skia_slave_scripts/render_webpage_pictures.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/ren...
slave/skia_slave_scripts/render_webpage_pictures.py:116:
file_utils.CreateCleanLocalDir(self._local_playback_skp_dir)
On 2012/11/06 21:54:22, EricB wrote:
> This new utility should be used everywhere applicable.  Doesn't need to be in
> this change, but we should update the other slave scripts to use it.

Acknowledged.

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/uti...
File slave/skia_slave_scripts/utils/gs_utils.py (right):

https://codereview.appspot.com/6817062/diff/9008/slave/skia_slave_scripts/uti...
slave/skia_slave_scripts/utils/gs_utils.py:15: Returns True if deleted
successfully else returns False
On 2012/11/06 20:25:33, EricB wrote:
> Most of our slave-side scripts use the convention of returning nothing but
> throwing an exception on failure. It's probably a good thing to be consistent
> throughout.

Done. Except for DoesStorageObjectExist which needs to return a boolean.

[epoger, 2012-11-08 15:26:26]

I've looked through the CL and I don't have any comments to add.  When it looks
good to Eric, it looks good to me.

[rmistry, 2012-11-08 19:35:03]

Added unit tests in file_utils_test.py and gs_utils_test.py.

Also added a script in skia_slave_scripts/run_unittests that recursively goes
through all child directories and constructs test suites to execute.

Please review at Patch Set 16.

[epoger, 2012-11-08 19:43:29]

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/run...
File slave/skia_slave_scripts/run_unittests (right):

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/run...
slave/skia_slave_scripts/run_unittests:6: """Runs all unit tests under this base
directory."""
Here are the 3 changes between patchsets 14 and 16:

https://codereview.appspot.com/6817062/diff2/4007:5011/slave/skia_slave_scrip...

https://codereview.appspot.com/6817062/diff2/4007:5011/slave/skia_slave_scrip...

https://codereview.appspot.com/6817062/diff2/4007:5011/slave/skia_slave_scrip...

Given the size of this CL already, I think it would have been better to add
those in a separate CL.  But they look good.

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/uti...
File slave/skia_slave_scripts/utils/file_utils_test.py (right):

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/uti...
slave/skia_slave_scripts/utils/file_utils_test.py:32: os.path.exists =
_MockPathExists
<keanu> Whoa. </keanu>

[EricB, 2012-11-08 20:11:54]

Looks mostly good, but I have a few concerns.

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/pla...
File slave/skia_slave_scripts/playback_dirs.py (right):

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/pla...
slave/skia_slave_scripts/playback_dirs.py:33: #################################
The reason we used this model for Android was that we had a different view of
the filesystem when we were inside the app from when we were using ADB.  That
code is gone now, but we created two instances of the same object with different
basedirs.  Instead of having {Local|Storage}PlaybackGmActualDir(), you just have
PlaybackGmActualDir(), but the two instances return different values.  So,
instead of:

d1 = dirs.LocalPlaybackGmActualDir()
d2 = dirs.StoragePlaybackGmActualDir()

You have:

d1 = local_dirs.PlaybackGmActualDir()
d2 = storage_dirs.PlaybackGmActualDir()

Which I think is cleaner and a bit better OO practice.

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/upl...
File slave/skia_slave_scripts/upload_bench_graphs.py (right):

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/upl...
slave/skia_slave_scripts/upload_bench_graphs.py:41: print '\n\n%s does not
exist! Skipping graph upload!' % graph_filepath
Raise a BuildStepWarning (or failure)? Or, return False and raise a
BuildStepWarning at the end of the loop in _Run if anything failed?

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/uti...
File slave/skia_slave_scripts/utils/file_utils_test.py (right):

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/uti...
slave/skia_slave_scripts/utils/file_utils_test.py:32: os.path.exists =
_MockPathExists
On 2012/11/08 19:43:29, epoger wrote:
> <keanu> Whoa. </keanu>

+1.  I guess each test runs in a separate process?  I want to be sure that
things we clobber like this can't affect other tests.

[rmistry, 2012-11-09 14:41:17]

Also added page_sets/skia_set.json (list of websites to use for replaying)
because I figure we will almost definitely need a different list than
http://src.chromium.org/viewvc/chrome/trunk/src/tools/perf/page_sets/top_25.j...

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/pla...
File slave/skia_slave_scripts/playback_dirs.py (right):

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/pla...
slave/skia_slave_scripts/playback_dirs.py:33: #################################
On 2012/11/08 20:11:54, EricB wrote:
> The reason we used this model for Android was that we had a different view of
> the filesystem when we were inside the app from when we were using ADB.  That
> code is gone now, but we created two instances of the same object with
different
> basedirs.  Instead of having {Local|Storage}PlaybackGmActualDir(), you just
have
> PlaybackGmActualDir(), but the two instances return different values.  So,
> instead of:
> 
> d1 = dirs.LocalPlaybackGmActualDir()
> d2 = dirs.StoragePlaybackGmActualDir()
> 
> You have:
> 
> d1 = local_dirs.PlaybackGmActualDir()
> d2 = storage_dirs.PlaybackGmActualDir()
> 
> Which I think is cleaner and a bit better OO practice.

Good idea. Done. Kept all classes in one module instead of creating too many
files.

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/upl...
File slave/skia_slave_scripts/upload_bench_graphs.py (right):

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/upl...
slave/skia_slave_scripts/upload_bench_graphs.py:41: print '\n\n%s does not
exist! Skipping graph upload!' % graph_filepath
On 2012/11/08 20:11:54, EricB wrote:
> Raise a BuildStepWarning (or failure)? Or, return False and raise a
> BuildStepWarning at the end of the loop in _Run if anything failed?

Raised BuildStepWarning here.

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/uti...
File slave/skia_slave_scripts/utils/file_utils_test.py (right):

https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/uti...
slave/skia_slave_scripts/utils/file_utils_test.py:32: os.path.exists =
_MockPathExists
On 2012/11/08 20:11:54, EricB wrote:
> On 2012/11/08 19:43:29, epoger wrote:
> > <keanu> Whoa. </keanu>
> 
> +1.  I guess each test runs in a separate process?  I want to be sure that
> things we clobber like this can't affect other tests.

Hmm, in google3 this would work but since I am not sure here, I am setting back
the originals, which is a good process to follow anyway.

[EricB, 2012-11-09 14:46:16]

On 2012/11/09 14:41:17, rmistry wrote:
> Also added page_sets/skia_set.json (list of websites to use for replaying)
> because I figure we will almost definitely need a different list than
>
http://src.chromium.org/viewvc/chrome/trunk/src/tools/perf/page_sets/top_25.j...
> 
>
https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/pla...
> File slave/skia_slave_scripts/playback_dirs.py (right):
> 
>
https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/pla...
> slave/skia_slave_scripts/playback_dirs.py:33:
#################################
> On 2012/11/08 20:11:54, EricB wrote:
> > The reason we used this model for Android was that we had a different view
of
> > the filesystem when we were inside the app from when we were using ADB. 
That
> > code is gone now, but we created two instances of the same object with
> different
> > basedirs.  Instead of having {Local|Storage}PlaybackGmActualDir(), you just
> have
> > PlaybackGmActualDir(), but the two instances return different values.  So,
> > instead of:
> > 
> > d1 = dirs.LocalPlaybackGmActualDir()
> > d2 = dirs.StoragePlaybackGmActualDir()
> > 
> > You have:
> > 
> > d1 = local_dirs.PlaybackGmActualDir()
> > d2 = storage_dirs.PlaybackGmActualDir()
> > 
> > Which I think is cleaner and a bit better OO practice.
> 
> Good idea. Done. Kept all classes in one module instead of creating too many
> files.
> 
>
https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/upl...
> File slave/skia_slave_scripts/upload_bench_graphs.py (right):
> 
>
https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/upl...
> slave/skia_slave_scripts/upload_bench_graphs.py:41: print '\n\n%s does not
> exist! Skipping graph upload!' % graph_filepath
> On 2012/11/08 20:11:54, EricB wrote:
> > Raise a BuildStepWarning (or failure)? Or, return False and raise a
> > BuildStepWarning at the end of the loop in _Run if anything failed?
> 
> Raised BuildStepWarning here.
> 
>
https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/uti...
> File slave/skia_slave_scripts/utils/file_utils_test.py (right):
> 
>
https://codereview.appspot.com/6817062/diff/5011/slave/skia_slave_scripts/uti...
> slave/skia_slave_scripts/utils/file_utils_test.py:32: os.path.exists =
> _MockPathExists
> On 2012/11/08 20:11:54, EricB wrote:
> > On 2012/11/08 19:43:29, epoger wrote:
> > > <keanu> Whoa. </keanu>
> > 
> > +1.  I guess each test runs in a separate process?  I want to be sure that
> > things we clobber like this can't affect other tests.
> 
> Hmm, in google3 this would work but since I am not sure here, I am setting
back
> the originals, which is a good process to follow anyway.

LGTM at patch set 18.