client.skia: separate builder and buildslave configs

client.skia: separate builder and buildslave configs

The way that recipes, build_configs, etc are specified in slaves.cfg is
not really compatible with how Skia's infrastructure thinks about
builders and buildslaves. In particular, any buildslave should be able
to run any number of buildslaves, regardless of build_config or recipe.

BUG=skia:761
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=284467

[iannucci, 2014-07-21 17:21:43]

I'm ok with this re-org, but I'm not certain that it's necessary. Recipes are
very un-opinionated about any of this stuff, the only thing that's needed is to
set a builder which has optional factory properties. how the slaves get attached
to that is up to the master.cfg

The GPU team did add a limited helper functionality which lets you put
everything all into slaves.cfg, which it looks like you were using before this
change.

If you're interested, we have an outstanding bug to formalize a recipes-only
builders.cfg format, which would replace basically all of master.cfg and
slaves.cfg. Builder as the primary entity is a much better way to organize.

Bug is here: crbug.com/356974

[borenet, 2014-07-21 17:28:34]

On 2014/07/21 17:21:43, iannucci wrote:
> I'm ok with this re-org, but I'm not certain that it's necessary. Recipes are
> very un-opinionated about any of this stuff, the only thing that's needed is
to
> set a builder which has optional factory properties. how the slaves get
attached
> to that is up to the master.cfg
> 
> The GPU team did add a limited helper functionality which lets you put
> everything all into slaves.cfg, which it looks like you were using before this
> change.
> 
> If you're interested, we have an outstanding bug to formalize a recipes-only
> builders.cfg format, which would replace basically all of master.cfg and
> slaves.cfg. Builder as the primary entity is a much better way to organize.
> 
> Bug is here: crbug.com/356974

Yeah, I guess we're using the helper functions, which I think is what we want
with just slightly different behavior.  Organizing by builder sounds right to me
as well.  For Skia, we have a *lot* of bots, the vast majority of which do
exactly the same series of steps but in slightly different ways.  On the master
side, I'd really like to end up with the right defaults set so that the builders
list can have one line per builder, which I think helps the readability a lot
when there are so many.

[iannucci, 2014-07-21 17:31:03]

overall lgtm

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
File masters/master.client.skia/master.cfg (right):

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
masters/master.client.skia/master.cfg:25: branch=None):
buildmaster_config and builders shouldn't default to None. This function would
break if that happened.

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
masters/master.client.skia/master.cfg:41: for maybe_parent in builders:
instead of looping a second time, I would add all of the
builders-which-will-be-parents to a set, and then check that potential_parents
>= set(builders.iterkeys()) at the end of the outer loop.

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
masters/master.client.skia/master.cfg:74: if 'recipe' in builder:
if it's not, that should be an error, right?

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
masters/master.client.skia/master.cfg:76: build_config = 'Release' if 'Release'
in name else 'Debug'
hm, sneaky. If we were making the names have semantic value, I'd prefer that the
builder names be /generated/ from a spec, instead of having the spec parsed from
the name. But as you like.

[borenet, 2014-07-21 18:04:31]

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
File masters/master.client.skia/master.cfg (right):

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
masters/master.client.skia/master.cfg:25: branch=None):
On 2014/07/21 17:31:03, iannucci wrote:
> buildmaster_config and builders shouldn't default to None. This function would
> break if that happened.

Yeah, and I don't know what happens when the scheduler name is None either, so
removed the default.

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
masters/master.client.skia/master.cfg:41: for maybe_parent in builders:
On 2014/07/21 17:31:03, iannucci wrote:
> instead of looping a second time, I would add all of the
> builders-which-will-be-parents to a set, and then check that potential_parents
> >= set(builders.iterkeys()) at the end of the outer loop.

Did something similar but used difference so I could provide an interesting
error message.

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
masters/master.client.skia/master.cfg:74: if 'recipe' in builder:
On 2014/07/21 17:31:03, iannucci wrote:
> if it's not, that should be an error, right?

In the next CL, I'll remove the 'if' and just default to the "skia" recipe.

https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
masters/master.client.skia/master.cfg:76: build_config = 'Release' if 'Release'
in name else 'Debug'
On 2014/07/21 17:31:03, iannucci wrote:
> hm, sneaky. If we were making the names have semantic value, I'd prefer that
the
> builder names be /generated/ from a spec, instead of having the spec parsed
from
> the name. But as you like.

Yeah, this is bad. Skia's bots currently do exactly that: the names are derived
from their configurations.  However, in this new world where all the interesting
stuff happens on the buildslave, I think the way to go is to have the list of
builder names on the master and parse the name to get the configs on the slave
side (and remove this build_config property here).  This is another thing I was
saving for the next CL; should I just lump the two together?

[iannucci, 2014-07-21 18:29:08]

On 2014/07/21 18:04:31, borenet wrote:
>
https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
> File masters/master.client.skia/master.cfg (right):
> 
>
https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
> masters/master.client.skia/master.cfg:25: branch=None):
> On 2014/07/21 17:31:03, iannucci wrote:
> > buildmaster_config and builders shouldn't default to None. This function
would
> > break if that happened.
> 
> Yeah, and I don't know what happens when the scheduler name is None either, so
> removed the default.
> 
>
https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
> masters/master.client.skia/master.cfg:41: for maybe_parent in builders:
> On 2014/07/21 17:31:03, iannucci wrote:
> > instead of looping a second time, I would add all of the
> > builders-which-will-be-parents to a set, and then check that
potential_parents
> > >= set(builders.iterkeys()) at the end of the outer loop.
> 
> Did something similar but used difference so I could provide an interesting
> error message.
> 
>
https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
> masters/master.client.skia/master.cfg:74: if 'recipe' in builder:
> On 2014/07/21 17:31:03, iannucci wrote:
> > if it's not, that should be an error, right?
> 
> In the next CL, I'll remove the 'if' and just default to the "skia" recipe.
> 
>
https://chromiumcodereview.appspot.com/393033005/diff/20001/masters/master.cl...
> masters/master.client.skia/master.cfg:76: build_config = 'Release' if
'Release'
> in name else 'Debug'
> On 2014/07/21 17:31:03, iannucci wrote:
> > hm, sneaky. If we were making the names have semantic value, I'd prefer that
> the
> > builder names be /generated/ from a spec, instead of having the spec parsed
> from
> > the name. But as you like.
> 
> Yeah, this is bad. Skia's bots currently do exactly that: the names are
derived
> from their configurations.  However, in this new world where all the
interesting
> stuff happens on the buildslave, I think the way to go is to have the list of
> builder names on the master and parse the name to get the configs on the slave
> side (and remove this build_config property here).  This is another thing I
was
> saving for the next CL; should I just lump the two together?

if you have another CL in the queue, no reason to glom it into this one :)

[iannucci, 2014-07-21 18:29:46]

still lgtm :)

[borenet, 2014-07-21 18:30:15]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-07-21 18:31:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/borenet@google.com/393033005/40001

[commit-bot: I haz the power, 2014-07-21 18:32:19]

Change committed as 284467