Bug 89215 – master.cfg cleanup: Pass CheckOutSource instance instead of class to BuildStep.addStep

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

because it is deprecated and will be dropped in buildbot 0.8.7

[Csaba Osztrogonác]

Created attachment 147822 [details]
Patch

Tested on a local test buildmaster (same as build.webkit.org)

[Tony Chang]

Comment on attachment 147822 [details]
Patch

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

> Tools/ChangeLog:6
> +
> +        Reviewed by NOBODY (OOPS!).

Nit: Can you include the 'why' (what you wrote in comment #0) in the changelog?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:117
> +    def __init__(self, **kwargs):

Nit: Do we need **kwargs here? I only see one caller for CheckOutSource and it doesn't pass any flags.

[Csaba Osztrogonác]

Comment on attachment 147822 [details]
Patch

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

>> Tools/ChangeLog:6
>> +        Reviewed by NOBODY (OOPS!).
> 
> Nit: Can you include the 'why' (what you wrote in comment #0) in the changelog?

Sure, will do.

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:117
>> +    def __init__(self, **kwargs):
> 
> Nit: Do we need **kwargs here? I only see one caller for CheckOutSource and it doesn't pass any flags.

Yes, we need. Buildbot will instantiate CheckOutSource too,
we should pass through kwargs to the parent class always.

"Keep a **kwargs argument on the end of your options, and pass that up to the parent class's constructor."
http://buildbot.net/buildbot/docs/0.8.6p1/manual/customization.html#writing-buildstep-constructors

[Csaba Osztrogonác]

Comment on attachment 147822 [details]
Patch

Landed in http://trac.webkit.org/changeset/121263 with additional comment.