Change Builder Names, MkIII

Change Builder Names, MkIII

Inspired by Ravi's comments on https://codereview.chromium.org/14475006/

- Use a dictionary of tuples to specify the builders in master_builders_cfg.
  This is much more readable and allows at-a-glance comparison.  The downside
  is that the line width is insanely long.
- Slightly restructure the builder names to use a full architecture spec rather
  than an arch width.  This allows us, for example, to differentiate between a
  32-bit ChromeOS compile bot targeting x86 and one targeting arm.

R=epoger@google.com, rmistry@google.com

Committed: https://code.google.com/p/skia/source/detail?r=8920

[borenet, 2013-04-26 15:28:28]

This is largely the same change as https://codereview.chromium.org/14475006,
with some major modifications to master_builders_cfg.

Note that I'll have to do a lot of merging as a result of
https://codereview.chromium.org/14081044/.

https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py
File master/master_builders_cfg.py (right):

https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py...
master/master_builders_cfg.py:82: builder_specs = {}
builder_specs is a dictionary whose keys are specifications for compile builders
and values are specifications for Test and Perf builders which will eventually
*depend* on those compile builders.

https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py...
master/master_builders_cfg.py:87: #
Don't fear the line lengths!  Set column width to 200!

https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py...
master/master_builders_cfg.py:243: defaults['category'] = '  Housekeeping'
Moved from utils.  Ideally, I'd make the housekeeper behave the same as the
above, but it's a bit of a special case.

https://codereview.chromium.org/14517004/diff/1/master/master_private_builder...
File master/master_private_builders_cfg.py (right):

https://codereview.chromium.org/14517004/diff/1/master/master_private_builder...
master/master_private_builders_cfg.py:43: ########## LIST ALL PRIVATELY VISIBLE
BUILDERS HERE ##########
Should update this to take the other format once this becomes relevant.

https://codereview.chromium.org/14517004/diff/1/master/skia_master_scripts/fa...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14517004/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:496: def Build(self, role=None,
clobber=None):
Combined the Build*() functions into one which switches on the role, since
MakeBuilderSet won't know which function to call.

https://codereview.chromium.org/14517004/diff/1/master/skia_master_scripts/ut...
File master/skia_master_scripts/utils.py (right):

https://codereview.chromium.org/14517004/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:214: def
AndroidModelToDevice(android_model):
Already added description locally:

Converts Android model names to device names which android_setup.sh will like.

  Examples:
    'NexusS' becomes 'nexus_s'
    'Nexus10' becomes 'nexus_10'

  android_model: string; model name for an Android device.

https://codereview.chromium.org/14517004/diff/1/master/slaves.cfg
File master/slaves.cfg (right):

https://codereview.chromium.org/14517004/diff/1/master/slaves.cfg#newcode233
master/slaves.cfg:233: 'serial': 'Rx86C801B5LH',
This and the others are "replace all" errors.  Already fixed locally.

[borenet, 2013-04-26 15:54:24]

On 2013/04/26 15:28:28, borenet wrote:
> This is largely the same change as https://codereview.chromium.org/14475006,
> with some major modifications to master_builders_cfg.
> 
> Note that I'll have to do a lot of merging as a result of
> https://codereview.chromium.org/14081044/.
> 
> https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py
> File master/master_builders_cfg.py (right):
> 
>
https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py...
> master/master_builders_cfg.py:82: builder_specs = {}
> builder_specs is a dictionary whose keys are specifications for compile
builders
> and values are specifications for Test and Perf builders which will eventually
> *depend* on those compile builders.
> 
>
https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py...
> master/master_builders_cfg.py:87: #
> Don't fear the line lengths!  Set column width to 200!
> 
>
https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py...
> master/master_builders_cfg.py:243: defaults['category'] = '  Housekeeping'
> Moved from utils.  Ideally, I'd make the housekeeper behave the same as the
> above, but it's a bit of a special case.
> 
>
https://codereview.chromium.org/14517004/diff/1/master/master_private_builder...
> File master/master_private_builders_cfg.py (right):
> 
>
https://codereview.chromium.org/14517004/diff/1/master/master_private_builder...
> master/master_private_builders_cfg.py:43: ########## LIST ALL PRIVATELY
VISIBLE
> BUILDERS HERE ##########
> Should update this to take the other format once this becomes relevant.
> 
>
https://codereview.chromium.org/14517004/diff/1/master/skia_master_scripts/fa...
> File master/skia_master_scripts/factory.py (right):
> 
>
https://codereview.chromium.org/14517004/diff/1/master/skia_master_scripts/fa...
> master/skia_master_scripts/factory.py:496: def Build(self, role=None,
> clobber=None):
> Combined the Build*() functions into one which switches on the role, since
> MakeBuilderSet won't know which function to call.
> 
>
https://codereview.chromium.org/14517004/diff/1/master/skia_master_scripts/ut...
> File master/skia_master_scripts/utils.py (right):
> 
>
https://codereview.chromium.org/14517004/diff/1/master/skia_master_scripts/ut...
> master/skia_master_scripts/utils.py:214: def
> AndroidModelToDevice(android_model):
> Already added description locally:
> 
> Converts Android model names to device names which android_setup.sh will like.
> 
>   Examples:
>     'NexusS' becomes 'nexus_s'
>     'Nexus10' becomes 'nexus_10'
> 
>   android_model: string; model name for an Android device.
> 
> https://codereview.chromium.org/14517004/diff/1/master/slaves.cfg
> File master/slaves.cfg (right):
> 
> https://codereview.chromium.org/14517004/diff/1/master/slaves.cfg#newcode233
> master/slaves.cfg:233: 'serial': 'Rx86C801B5LH',
> This and the others are "replace all" errors.  Already fixed locally.

Patch set 2 fixes some problems with the first patch set and fixes merge
conflicts.

[epoger, 2013-04-26 17:41:57]

LGTM if it LGTRavi

https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py
File master/master_builders_cfg.py (right):

https://codereview.chromium.org/14517004/diff/1/master/master_builders_cfg.py...
master/master_builders_cfg.py:82: builder_specs = {}
On 2013/04/26 15:28:28, borenet wrote:
> builder_specs is a dictionary whose keys are specifications for compile
builders
> and values are specifications for Test and Perf builders which will eventually
> *depend* on those compile builders.

Thanks. Please put this comment in the code...

[borenet, 2013-04-29 13:13:24]

Patch set 4 adds a couple of utility scripts for re-generating the baseline
files as well as updated baselines.

https://codereview.chromium.org/14517004/diff/23001/tools/tests/factory_confi...
File tools/tests/factory_configuration/expected/Build-Mac10.6-GCC-x86-Debug
(right):

https://codereview.chromium.org/14517004/diff/23001/tools/tests/factory_confi...
tools/tests/factory_configuration/expected/Build-Mac10.6-GCC-x86-Debug:1: {
Notes on these files:

All were created using the new rename_builders script, which runs "svn mv" on
the file and replaces all instances of the old builder name with the new name. 
Then I generated the actuals by running the buildbot and copied them over the
new files.

The major changes are:
- The names of the builders
- gm_image_subdir is removed from many builders who don't need it, eg. perf or
compile builders
- gyp defines - they have been re-ordered in many cases and unneeded defines are
removed.

Most of the differences are visible on the first page or so.  After that point,
they are repeated for each build step.

[borenet, 2013-04-29 16:11:09]

Patch set 5 adds files which will need to change as a result of changing the
builder names.  The estimated full set is located in this doc:
https://docs.google.com/a/google.com/document/d/1BfTUBPJcnc7DD-G3DlOr0kmQsz0b...

[rmistry, 2013-04-29 21:02:58]

LGTM

https://codereview.chromium.org/14517004/diff/38001/master/master_builders_cf...
File master/master_builders_cfg.py (right):

https://codereview.chromium.org/14517004/diff/38001/master/master_builders_cf...
master/master_builders_cfg.py:7: #pylint: disable=C0301
Looks like this works.. cool..

https://codereview.chromium.org/14517004/diff/38001/master/master_builders_cf...
master/master_builders_cfg.py:217: os=compile_builder[0],
Looks like you decided not to have helper methods to parse the tuple as we
initially discussed. That is ok with me, but wanted to add this comment incase
you forgot.

I also said I would look into a way to easily parse tuples, this looks like the
only option:
http://stackoverflow.com/questions/7839168/extracting-information-from-a-tupl...
Not the best option for us since it will blow up each line.

https://codereview.chromium.org/14517004/diff/38001/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14517004/diff/38001/master/skia_master_script...
master/skia_master_scripts/factory.py:124: else:
Instead of the else you could remove line 123 and do:
self._env_vars['GYP_DEFINES'] += ' skia_warnings_as_errors=%d' %
int(compile_warnings_as_errors)
since the same line is repeated in both blocks.

[borenet, 2013-04-30 11:43:50]

https://codereview.chromium.org/14517004/diff/38001/master/master_builders_cf...
File master/master_builders_cfg.py (right):

https://codereview.chromium.org/14517004/diff/38001/master/master_builders_cf...
master/master_builders_cfg.py:7: #pylint: disable=C0301
On 2013/04/29 21:02:58, rmistry wrote:
> Looks like this works.. cool..

Yep!

https://codereview.chromium.org/14517004/diff/38001/master/master_builders_cf...
master/master_builders_cfg.py:217: os=compile_builder[0],
On 2013/04/29 21:02:58, rmistry wrote:
> Looks like you decided not to have helper methods to parse the tuple as we
> initially discussed. That is ok with me, but wanted to add this comment incase
> you forgot.
> 
> I also said I would look into a way to easily parse tuples, this looks like
the
> only option:
>
http://stackoverflow.com/questions/7839168/extracting-information-from-a-tupl...
> Not the best option for us since it will blow up each line.

It looked like it was going to get much messier and less readable.  I opted for
clarity.

https://codereview.chromium.org/14517004/diff/38001/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14517004/diff/38001/master/skia_master_script...
master/skia_master_scripts/factory.py:124: else:
On 2013/04/29 21:02:58, rmistry wrote:
> Instead of the else you could remove line 123 and do:
> self._env_vars['GYP_DEFINES'] += ' skia_warnings_as_errors=%d' %
> int(compile_warnings_as_errors)
> since the same line is repeated in both blocks.

The problem is that GYP_DEFINES may not be in self._env_vars.  So using +=
unconditionally will fail if it is None.

[rmistry, 2013-04-30 11:59:23]

LGTM

https://codereview.chromium.org/14517004/diff/38001/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14517004/diff/38001/master/skia_master_script...
master/skia_master_scripts/factory.py:124: else:
On 2013/04/30 11:43:51, borenet wrote:
> On 2013/04/29 21:02:58, rmistry wrote:
> > Instead of the else you could remove line 123 and do:
> > self._env_vars['GYP_DEFINES'] += ' skia_warnings_as_errors=%d' %
> > int(compile_warnings_as_errors)
> > since the same line is repeated in both blocks.
> 
> The problem is that GYP_DEFINES may not be in self._env_vars.  So using +=
> unconditionally will fail if it is None.

self._env_vars['GYP_DEFINES'] = self._env_vars.get('GYP_DEFINES', '') + ...

Either way this is minor you can make the change later, lets get this CL in.

[borenet, 2013-04-30 12:01:48]

Committed patchset #5 manually as r8920.