Adding trybots for Periodic and PerCommit Housekeeper

Adding trybots for Periodic and PerCommit Housekeeper

Created to fix the feature request https://code.google.com/p/skia/issues/detail?id=1139: support tries on the housekeeping bot

(SkipBuildbotRuns)
Committed: https://code.google.com/p/skia/source/detail?r=7950

[borenet, 2013-03-01 20:53:00]

https://codereview.chromium.org/12380071/diff/4001/master/master_builders_cfg.py
File master/master_builders_cfg.py (right):

https://codereview.chromium.org/12380071/diff/4001/master/master_builders_cfg...
master/master_builders_cfg.py:9: from skia_master_scripts.utils import
MakeBuilderSet, MakeAndroidBuilderSet, \
Nit: since the rest have their own line, maybe move MakeAndroidBuilderSet to the
next line?

https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
File master/skia_master_scripts/housekeeping_periodic_factory.py (right):

https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
master/skia_master_scripts/housekeeping_periodic_factory.py:33: if not
self._do_patch_step:  # Do not run the sanitizer if it is a try job.
I think it's clearer from a user perspective to perform this check in the build
step script itself so that the step can throw a BuildStepWarning indicating that
the step doesn't get run on trybots.  This is what we currently do with
UploadGMResults.

[rmistry, 2013-03-01 21:05:06]

https://codereview.chromium.org/12380071/diff/4001/master/master_builders_cfg.py
File master/master_builders_cfg.py (right):

https://codereview.chromium.org/12380071/diff/4001/master/master_builders_cfg...
master/master_builders_cfg.py:9: from skia_master_scripts.utils import
MakeBuilderSet, MakeAndroidBuilderSet, \
On 2013/03/01 20:53:00, borenet wrote:
> Nit: since the rest have their own line, maybe move MakeAndroidBuilderSet to
the
> next line?

Done.

https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
File master/skia_master_scripts/housekeeping_periodic_factory.py (right):

https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
master/skia_master_scripts/housekeeping_periodic_factory.py:33: if not
self._do_patch_step:  # Do not run the sanitizer if it is a try job.
On 2013/03/01 20:53:00, borenet wrote:
> I think it's clearer from a user perspective to perform this check in the
build
> step script itself so that the step can throw a BuildStepWarning indicating
that
> the step doesn't get run on trybots.  This is what we currently do with
> UploadGMResults.

Yes this makes sense in the python scripts, but some build steps here (and in
percommit factory) are bash scripts and some are just bash commands. So to make
it consistent I used the same conditional stmt.
I think if we depend on one behavior for one case and another for other cases it
will be harder for us to maintain.

[borenet, 2013-03-01 21:18:49]

https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
File master/skia_master_scripts/housekeeping_periodic_factory.py (right):

https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
master/skia_master_scripts/housekeeping_periodic_factory.py:33: if not
self._do_patch_step:  # Do not run the sanitizer if it is a try job.
On 2013/03/01 21:05:07, rmistry wrote:
> On 2013/03/01 20:53:00, borenet wrote:
> > I think it's clearer from a user perspective to perform this check in the
> build
> > step script itself so that the step can throw a BuildStepWarning indicating
> that
> > the step doesn't get run on trybots.  This is what we currently do with
> > UploadGMResults.
> 
> Yes this makes sense in the python scripts, but some build steps here (and in
> percommit factory) are bash scripts and some are just bash commands. So to
make
> it consistent I used the same conditional stmt.
> I think if we depend on one behavior for one case and another for other cases
it
> will be harder for us to maintain.

Fair enough.  Actually, now that I think about it, the best way to do it is with
doStepIf, which is a function pointer passed into factory.addStep.  By default,
all of our build steps are instances of SkiaBuildStep and doStepIf is set to
ShouldDoStep (see
https://code.google.com/p/skia/source/browse/buildbot/master/skia_master_scri...).

Maybe we should add a field to SkiaBuildStep which is "skip_on_trybots" or some
such, and add logic in ShouldDoStep to determine if the build is a try, and
return False if skip_on_trybots is True and the build is a try.  The nice thing
about "skipped" steps is that they still show up, but they are colored blue and
are clearly marked "skipped."  This would work no matter what the slave-side
script looks like, and would be consistent across the board.  Thoughts?

[rmistry, 2013-03-04 12:16:25]

On 2013/03/01 21:18:49, borenet wrote:
>
https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
> File master/skia_master_scripts/housekeeping_periodic_factory.py (right):
> 
>
https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
> master/skia_master_scripts/housekeeping_periodic_factory.py:33: if not
> self._do_patch_step:  # Do not run the sanitizer if it is a try job.
> On 2013/03/01 21:05:07, rmistry wrote:
> > On 2013/03/01 20:53:00, borenet wrote:
> > > I think it's clearer from a user perspective to perform this check in the
> > build
> > > step script itself so that the step can throw a BuildStepWarning
indicating
> > that
> > > the step doesn't get run on trybots.  This is what we currently do with
> > > UploadGMResults.
> > 
> > Yes this makes sense in the python scripts, but some build steps here (and
in
> > percommit factory) are bash scripts and some are just bash commands. So to
> make
> > it consistent I used the same conditional stmt.
> > I think if we depend on one behavior for one case and another for other
cases
> it
> > will be harder for us to maintain.
> 
> Fair enough.  Actually, now that I think about it, the best way to do it is
with
> doStepIf, which is a function pointer passed into factory.addStep.  By
default,
> all of our build steps are instances of SkiaBuildStep and doStepIf is set to
> ShouldDoStep (see
>
https://code.google.com/p/skia/source/browse/buildbot/master/skia_master_scri...).
> 
> Maybe we should add a field to SkiaBuildStep which is "skip_on_trybots" or
some
> such, and add logic in ShouldDoStep to determine if the build is a try, and
> return False if skip_on_trybots is True and the build is a try.  The nice
thing
> about "skipped" steps is that they still show up, but they are colored blue
and
> are clearly marked "skipped."  This would work no matter what the slave-side
> script looks like, and would be consistent across the board.  Thoughts?


This means in places like commands.AddRunCommand will have to take in a
'skip_on_trybots' or maybe just a 'skip' parameter. The factory will still have
to pass in the appropriate boolean to determine if the step should be skipped or
not, and that is probably better than the conditionals everywhere. The major
benefit with your proposal is that they will be marked skipped. so SGTM.

That is going to be a separate effort from this CL. Is this CL ready to commit?

[borenet, 2013-03-04 12:22:21]

On 2013/03/04 12:16:25, rmistry wrote:
> On 2013/03/01 21:18:49, borenet wrote:
> >
>
https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
> > File master/skia_master_scripts/housekeeping_periodic_factory.py (right):
> > 
> >
>
https://codereview.chromium.org/12380071/diff/4001/master/skia_master_scripts...
> > master/skia_master_scripts/housekeeping_periodic_factory.py:33: if not
> > self._do_patch_step:  # Do not run the sanitizer if it is a try job.
> > On 2013/03/01 21:05:07, rmistry wrote:
> > > On 2013/03/01 20:53:00, borenet wrote:
> > > > I think it's clearer from a user perspective to perform this check in
the
> > > build
> > > > step script itself so that the step can throw a BuildStepWarning
> indicating
> > > that
> > > > the step doesn't get run on trybots.  This is what we currently do with
> > > > UploadGMResults.
> > > 
> > > Yes this makes sense in the python scripts, but some build steps here (and
> in
> > > percommit factory) are bash scripts and some are just bash commands. So to
> > make
> > > it consistent I used the same conditional stmt.
> > > I think if we depend on one behavior for one case and another for other
> cases
> > it
> > > will be harder for us to maintain.
> > 
> > Fair enough.  Actually, now that I think about it, the best way to do it is
> with
> > doStepIf, which is a function pointer passed into factory.addStep.  By
> default,
> > all of our build steps are instances of SkiaBuildStep and doStepIf is set to
> > ShouldDoStep (see
> >
>
https://code.google.com/p/skia/source/browse/buildbot/master/skia_master_scri...).
> > 
> > Maybe we should add a field to SkiaBuildStep which is "skip_on_trybots" or
> some
> > such, and add logic in ShouldDoStep to determine if the build is a try, and
> > return False if skip_on_trybots is True and the build is a try.  The nice
> thing
> > about "skipped" steps is that they still show up, but they are colored blue
> and
> > are clearly marked "skipped."  This would work no matter what the slave-side
> > script looks like, and would be consistent across the board.  Thoughts?
> 
> 
> This means in places like commands.AddRunCommand will have to take in a
> 'skip_on_trybots' or maybe just a 'skip' parameter. The factory will still
have
> to pass in the appropriate boolean to determine if the step should be skipped
or
> not, and that is probably better than the conditionals everywhere. The major
> benefit with your proposal is that they will be marked skipped. so SGTM.
> 
> That is going to be a separate effort from this CL. Is this CL ready to
commit?

LGTM.  Let's make a note to add a skip_on_trybots parameter.

[rmistry, 2013-03-04 12:36:26]

Committed patchset #4 manually as r7950 (presubmit successful).