Use bench_pictures.cfg to configure BenchPictures build step

Use bench_pictures.cfg to configure BenchPictures build step
Committed: https://code.google.com/p/skia/source/detail?r=6810

[EricB, 2012-12-11 20:10:13]

Work in progress.

https://codereview.appspot.com/6927050/diff/1/bench_pictures.cfg
File bench_pictures.cfg (right):

https://codereview.appspot.com/6927050/diff/1/bench_pictures.cfg#newcode1
bench_pictures.cfg:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
This file will live somewhere in trunk, rather than in buildbot.

[EricB, 2012-12-11 21:30:55]

Uploaded patch set 2, which introduces helper functions and does away with most
of the loops.

[epoger, 2012-12-12 19:26:27]

https://codereview.appspot.com/6927050/diff/1003/bench_pictures.cfg
File bench_pictures.cfg (right):

https://codereview.appspot.com/6927050/diff/1003/bench_pictures.cfg#newcode9
bench_pictures.cfg:9: bench_pictures_cfg dictionary. Everything else in this
file is for convenience.
maybe "Everything else in this file is for convenience." -> "Everything else in
this file is just used to construct that dictionary."

https://codereview.appspot.com/6927050/diff/1003/bench_pictures.cfg#newcode11
bench_pictures.cfg:11: This code is executed directly on the buildbot so that
convenient things like
I think it might be better to just define a static dictionary, for several
reasons:

1. you don't need to eval() on the buildbot, so we don't have to worry about
running "nasty" code on the build slaves

2. it will be easier to see exactly what configurations are running, just by
looking at this file; this will make it clearer that we are running a heck of a
lot of combinations!

3. it will be easier to run something slightly different in a trybot (svn diff
of this file will show you EXACTLY what will run differently)


We could still use this code to generate the static dictionary file...

[epoger, 2012-12-12 19:38:16]

https://codereview.appspot.com/6927050/diff/1003/bench_pictures.cfg
File bench_pictures.cfg (right):

https://codereview.appspot.com/6927050/diff/1003/bench_pictures.cfg#newcode11
bench_pictures.cfg:11: This code is executed directly on the buildbot so that
convenient things like
On 2012/12/12 19:26:27, epoger wrote:
> I think it might be better to just define a static dictionary, for several
> reasons:
> 
> 1. you don't need to eval() on the buildbot, so we don't have to worry about
> running "nasty" code on the build slaves
> 
> 2. it will be easier to see exactly what configurations are running, just by
> looking at this file; this will make it clearer that we are running a heck of
a
> lot of combinations!
> 
> 3. it will be easier to run something slightly different in a trybot (svn diff
> of this file will show you EXACTLY what will run differently)
> 
> 
> We could still use this code to generate the static dictionary file...

Eric has convinced me of the error of my ways.  I retract this comment.

[EricB, 2012-12-12 19:42:25]

Uploaded https://codereview.appspot.com/6873073/ for "real" review.

[EricB, 2012-12-12 22:08:17]

Patch set 5 adapts this for bench_pictures.cfg's location in trunk/tools.  The
bench_pictures_cfg name is defined per-builder as a parameter to SkiaFactory and
is set to 'default' unless otherwise defined.  AndroidFactory provides this as
the device name, and the Linux NoGPU bot and ChromeOS bot both use 'no_gpu'.

Ready for "real" review.

[epoger, 2012-12-13 15:51:33]

LGTM with one optional suggestion

https://codereview.appspot.com/6927050/diff/8005/slave/skia_slave_scripts/ben...
File slave/skia_slave_scripts/bench_pictures.py (right):

https://codereview.appspot.com/6927050/diff/8005/slave/skia_slave_scripts/ben...
slave/skia_slave_scripts/bench_pictures.py:54: cfg_name =
self._args['bench_pictures_cfg']
maybe extract lines 54-64 into a separate method?
my_configs = self._GetMyConfigs()

Either that, or use newlines/comments to delineate between "figuring out which
configs I use" and "assembling args based on those configs"

[EricB, 2012-12-13 16:19:57]

Patch set 6.

https://codereview.appspot.com/6927050/diff/8005/slave/skia_slave_scripts/ben...
File slave/skia_slave_scripts/bench_pictures.py (right):

https://codereview.appspot.com/6927050/diff/8005/slave/skia_slave_scripts/ben...
slave/skia_slave_scripts/bench_pictures.py:54: cfg_name =
self._args['bench_pictures_cfg']
On 2012/12/13 15:51:33, epoger wrote:
> maybe extract lines 54-64 into a separate method?
> my_configs = self._GetMyConfigs()
> 
> Either that, or use newlines/comments to delineate between "figuring out which
> configs I use" and "assembling args based on those configs"

I added comments.  I think the code for determining which configs to run is
small enough, and the function as a whole is small enough, that splitting into a
different function might be harder to read.

https://codereview.appspot.com/6927050/diff/14001/slave/skia_slave_scripts/be...
File slave/skia_slave_scripts/bench_pictures.py (right):

https://codereview.appspot.com/6927050/diff/14001/slave/skia_slave_scripts/be...
slave/skia_slave_scripts/bench_pictures.py:83: raise errors[0]
Added delayed failure so that all configs are attempted even if one or more
fails.

[epoger, 2012-12-13 16:43:37]

LGTM