Bug 89001 - master.cfg cleanup, pass BuildStep instances instead of BuildStep subclasses
Summary: master.cfg cleanup, pass BuildStep instances instead of BuildStep subclasses
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Szilard Ledan
URL:
Keywords:
Depends on: 89120 89215 89564
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-13 08:15 PDT by Csaba Osztrogonác
Modified: 2012-07-03 08:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.70 KB, patch)
2012-06-13 08:18 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
patch (10.64 KB, patch)
2012-07-03 02:23 PDT, Szilard Ledan
no flags Details | Formatted Diff | Diff
patch (10.65 KB, patch)
2012-07-03 03:23 PDT, Szilard Ledan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-06-13 08:15:40 PDT
because it is deprecated and will be dropped in buildbot 0.8.7:
"./mastercfg_unittest.py:....: DeprecationWarning: Passing a BuildStep subclass to factory.addStep is deprecated.  Please pass a BuildStep instance instead.  Support will be dropped in v0.8.7."
Comment 1 Csaba Osztrogonác 2012-06-13 08:18:30 PDT
Created attachment 147319 [details]
Patch
Comment 2 Lucas Forschler 2012-06-14 12:18:40 PDT
Looks good to me... finally, those deprecation warnings will be gone :)
Comment 3 Csaba Osztrogonác 2012-06-14 13:04:01 PDT
Landed in http://trac.webkit.org/changeset/120352
Comment 4 WebKit Review Bot 2012-06-14 13:21:08 PDT
Re-opened since this is blocked by 89120
Comment 5 Csaba Osztrogonác 2012-06-15 07:36:15 PDT
It caused problems, because buildstep's constructors aren't in a good shape now,
they must be refactored before we commit this change.

See http://buildbot.net/buildbot/docs/0.8.6p1/manual/customization.html#writing-buildstep-constructors for details.

I'm going to check and fix them one by one soon.
Comment 6 Csaba Osztrogonác 2012-06-15 08:18:58 PDT
I'm going to add smaller patches to "depends on" list, here is the first one - https://bugs.webkit.org/show_bug.cgi?id=89215

Unfortunately I can't test all of them, because I can't run locally for example Apple, Chromium, ... buildslaves. And unfortunately this kind of change isn't unittestable simple ... But I'm going to thinking how is it possible ...

Or Péter, have you got any idea, how can we can make 
this kind of changes unittestable?
Comment 7 Csaba Osztrogonác 2012-06-26 08:04:28 PDT
Unit test helping this fix added in https://bugs.webkit.org/show_bug.cgi?id=89564
Comment 8 Szilard Ledan 2012-07-03 02:23:22 PDT
Created attachment 150564 [details]
patch
Comment 9 Peter Gal 2012-07-03 02:28:45 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=150564&action=review

Just some minor things:

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:222
> +        kwargs["slavesrc"]=self.slavesrc
> +        kwargs["masterdest"]=self.masterdest
> +        kwargs["mode"]=0644

put spaces before and after the '=' in this cases.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:642
> +        kwargs["slavesrc"]=self.slavesrc
> +        kwargs["masterdest"]=self.masterdest
> +        kwargs["mode"]=0644

ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:652
> +        kwargs["command"]=""

ditto
Comment 10 Csaba Osztrogonác 2012-07-03 02:33:44 PDT
Comment on attachment 150564 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150564&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:220
> +        kwargs["slavesrc"]=self.slavesrc

One more thing: Please use ' instead of " similar to other parts of master.cfg.
Comment 11 Szilard Ledan 2012-07-03 03:23:46 PDT
Created attachment 150572 [details]
patch

Thanks for your advice!
Comment 12 Csaba Osztrogonác 2012-07-03 05:52:44 PDT
Comment on attachment 150572 [details]
patch

LGTM, r=me. But cq-, because I'd like to land it manually. If something goes wrong, I'll roll it out immediately.
Comment 13 Csaba Osztrogonác 2012-07-03 06:00:58 PDT
Comment on attachment 150572 [details]
patch

Clearing flags on attachment: 150572

Committed r121760: <http://trac.webkit.org/changeset/121760>
Comment 14 Csaba Osztrogonác 2012-07-03 06:01:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Csaba Osztrogonác 2012-07-03 06:05:01 PDT
(In reply to comment #13)
> (From update of attachment 150572 [details])
> Clearing flags on attachment: 150572
> 
> Committed r121760: <http://trac.webkit.org/changeset/121760>

It seems master haven't restarted after this patch. Lucas, could you restart
the master please? (Feel free to revert the patch if something doesn't work.)
Comment 16 Lucas Forschler 2012-07-03 08:00:25 PDT
I just logged in to restart the master, but since we've moved to new hardware, my account hasn't been given permission.  I've emailed the right people for 2 issues:
1.  buildbot isnt' restarting after check-in
2.  giving me permissions to manually restart.
Comment 17 Lucas Forschler 2012-07-03 08:14:57 PDT
I've restarted the master.
Comment 18 Csaba Osztrogonác 2012-07-03 08:56:21 PDT
(In reply to comment #17)
> I've restarted the master.
Many thanks, we have better master.cfg day by day. :)