Bug 89001 – master.cfg cleanup, pass BuildStep instances instead of BuildStep subclasses

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

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."

[Csaba Osztrogonác]

Created attachment 147319 [details]
Patch

[Lucas Forschler]

Looks good to me... finally, those deprecation warnings will be gone :)

[Csaba Osztrogonác]

Landed in http://trac.webkit.org/changeset/120352

[WebKit Review Bot]

Re-opened since this is blocked by 89120

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

Unit test helping this fix added in https://bugs.webkit.org/show_bug.cgi?id=89564

[Szilard Ledan]

Created attachment 150564 [details]
patch

[Peter Gal]

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

[Csaba Osztrogonác]

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.

[Szilard Ledan]

Created attachment 150572 [details]
patch

Thanks for your advice!

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Comment on attachment 150572 [details]
patch

Clearing flags on attachment: 150572

Committed r121760: <http://trac.webkit.org/changeset/121760>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.)

[Lucas Forschler]

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.

[Lucas Forschler]

I've restarted the master.

[Csaba Osztrogonác]

(In reply to comment #17)
> I've restarted the master.
Many thanks, we have better master.cfg day by day. :)