Bug 185484 – Add Buildbot configuration for Opensource EWS

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

[Aakash Jain]

We are planning to use Buildbot for OpenSource EWS (See https://bugs.webkit.org/show_bug.cgi?id=179506). We need to write corresponding Buildbot configuration.

[Aakash Jain]

Created attachment 340011 [details]
Proposed patch

Patch for basic buildbot configuration. Doesn't have factories, steps etc yet. Tested locally.

[EWS Watchlist]

Attachment 340011 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/ews-build/loadConfig.py:5:  No name 'worker' in module 'buildbot'  [pylint/E0611] [5]
ERROR: Tools/BuildSlaveSupport/ews-build/loadConfig.py:6:  No name 'identifiers' in module 'buildbot.util'  [pylint/E0611] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Johnson]

Comment on attachment 340011 [details]
Proposed patch

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

Looks mostly good; can you add tests for the loadSafariConfig functions? At least for: getValidTags, getTagsForBuilder, checkValidWorker, checkValidBuilder? IMO we don't need to check each message that's used, just make sure each potential issue in config.json is adequately caught.

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:32
> +        kls = globals()[scheduler.pop('type')]

kls? Maybe we should name this `factoryName`?

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:33
> +        scheduler = dict(map(lambda key_value_pair: (str(key_value_pair[0]), key_value_pair[1]), scheduler.items()))

This is a bit dense, but still readable. When can key_value_pair[0] ever be a non-string type?

It also may read better as follows:
scheduler = {str(k): v for k, v in scheduler.items()}  # Python >= 2.7 iirc.

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:79
> +            if not worker:

Nit: I'd recommend writing this like follows, since you're really checking if the value is None as opposed to if it's Truth-y/False-y.

`if worker is None`

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:82
> +            if worker['platform'] != builder['platform']:

Do we need to define worker['platform']? Can't it be inferred from builder['platform'] instead? It would simplify the config greatly.

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:103
> +    keywords = filter(None, re.split("[, \-_:()]+", str(builder['name'])))

Is the `str` cast necessary here?

[Aakash Jain]

(In reply to Dean Johnson from comment #3)
> Looks mostly good; can you add tests for the loadSafariConfig functions? At
> least for: getValidTags, getTagsForBuilder, checkValidWorker,
> checkValidBuilder? IMO we don't need to check each message that's used, just
> make sure each potential issue in config.json is adequately caught.
Added unit-tests.
 
> > Tools/BuildSlaveSupport/ews-build/loadConfig.py:32
> > +        kls = globals()[scheduler.pop('type')]
> 
> kls? Maybe we should name this `factoryName`?
Renamed to schedulerType.

> > Tools/BuildSlaveSupport/ews-build/loadConfig.py:79
> > +            if not worker:
> 
> Nit: I'd recommend writing this like follows, since you're really checking
> if the value is None as opposed to if it's Truth-y/False-y.
> 
> `if worker is None`
Done
 
> > Tools/BuildSlaveSupport/ews-build/loadConfig.py:82
> > +            if worker['platform'] != builder['platform']:
> 
> Do we need to define worker['platform']? Can't it be inferred from
> builder['platform'] instead? It would simplify the config greatly.
This is for verification and preventing manual mistakes like accidentally connecting worker to wrong builder. This is similar to all other buildbot configurations we have currently.

> > Tools/BuildSlaveSupport/ews-build/loadConfig.py:103
> > +    keywords = filter(None, re.split("[, \-_:()]+", str(builder['name'])))
> 
> Is the `str` cast necessary here?
Yeah, this is in order to convert from unicode to str. Otherwise tags are returned as Unicode and buildbot gives an error:
builder 'iOS-11-EWS': tags list contains something that is not a string
Also added unit-test for this case.

[Aakash Jain]

Created attachment 341011 [details]
Updated patch

Updated patch with unit-tests and review comments incorporated.

[EWS Watchlist]

Attachment 341011 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/ews-build/loadConfig.py:28:  No name 'worker' in module 'buildbot'  [pylint/E0611] [5]
ERROR: Tools/BuildSlaveSupport/ews-build/loadConfig.py:29:  No name 'identifiers' in module 'buildbot.util'  [pylint/E0611] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 341011 [details]
Updated patch

rs=me. Does the unit test get run on build.webkit.org bots?

[Aakash Jain]

> rs=me.
Thanks. 
> Does the unit test get run on build.webkit.org bots?
Not yet. Will add it later on.

[Aakash Jain]

Created attachment 341100 [details]
Patch for landing

[EWS Watchlist]

Attachment 341100 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/ews-build/loadConfig.py:28:  No name 'worker' in module 'buildbot'  [pylint/E0611] [5]
ERROR: Tools/BuildSlaveSupport/ews-build/loadConfig.py:29:  No name 'identifiers' in module 'buildbot.util'  [pylint/E0611] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 341100 [details]
Patch for landing

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

I did not check the correctness of this patch. I noticed some very minor cosmetic issues. This patch alternates between using single and double quoted string literal. We should pick one style and stick with it. I believe for webkitpy we prefer single quoted literal as in new code. Having said that I believe Buildbot code and the Buildbot project prefer double quoted literals.

> Tools/BuildSlaveSupport/ews-build/master.cfg:17
> +c['projectName'] = 'Webkit EWS'

Webkit  => WebKit

[Daniel Bates]

Comment on attachment 341100 [details]
Patch for landing

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

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:48
> +        builder.pop('platform')

We are underutilizing pop(), which returns the “popped” value. I suggest we use del builder[...].

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:49
> +        if builder.get('configuration'):

We should use “in” to test for existence instead of get().

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:50
> +            builder.pop('configuration')

We are underutilizing pop(), which returns the “popped” value. I suggest we use del builder[...].

> Tools/BuildSlaveSupport/ews-build/loadConfig.py:125
> +    tags_blacklist = [str(i) for i in range(0, 20)]

This is OK as-is, but is suboptimal because we alloc for range(). We should should xrange().

[Aakash Jain]

Created attachment 341141 [details]
Patch for landing

> We should pick one style and stick with it. I believe for webkitpy we prefer single quoted literal as in new code. 
Fixed, I have tried to use single quotes as much as possible in updated patch.

> > Tools/BuildSlaveSupport/ews-build/loadConfig.py:48
> > +        builder.pop('platform')
> 
> We are underutilizing pop(), which returns the “popped” value. I suggest we
> use del builder[...].
The plan was to use this popped value in subsequent patches. For now, I have changed it to del builder[...] as you suggested.

> > Tools/BuildSlaveSupport/ews-build/loadConfig.py:49
> > +        if builder.get('configuration'):
> 
> We should use “in” to test for existence instead of get().
Done.

> > Tools/BuildSlaveSupport/ews-build/loadConfig.py:50
> > +            builder.pop('configuration')
> 
> We are underutilizing pop(), which returns the “popped” value. I suggest we
> use del builder[...].
Ditto. Changed to del.

> > Tools/BuildSlaveSupport/ews-build/loadConfig.py:125
> > +    tags_blacklist = [str(i) for i in range(0, 20)]
> 
> This is OK as-is, but is suboptimal because we alloc for range(). We should
> should xrange().
Done.

[Dean Johnson]

(In reply to Aakash Jain from comment #4)
> (In reply to Dean Johnson from comment #3)
> > Looks mostly good; can you add tests for the loadSafariConfig functions? At
> > least for: getValidTags, getTagsForBuilder, checkValidWorker,
> > checkValidBuilder? IMO we don't need to check each message that's used, just
> > make sure each potential issue in config.json is adequately caught.
> Added unit-tests.
>  
> > > Tools/BuildSlaveSupport/ews-build/loadConfig.py:32
> > > +        kls = globals()[scheduler.pop('type')]
> > 
> > kls? Maybe we should name this `factoryName`?
> Renamed to schedulerType.
Sounds good.
> 
> > > Tools/BuildSlaveSupport/ews-build/loadConfig.py:79
> > > +            if not worker:
> > 
> > Nit: I'd recommend writing this like follows, since you're really checking
> > if the value is None as opposed to if it's Truth-y/False-y.
> > 
> > `if worker is None`
> Done
>  
> > > Tools/BuildSlaveSupport/ews-build/loadConfig.py:82
> > > +            if worker['platform'] != builder['platform']:
> > 
> > Do we need to define worker['platform']? Can't it be inferred from
> > builder['platform'] instead? It would simplify the config greatly.
> This is for verification and preventing manual mistakes like accidentally
> connecting worker to wrong builder. This is similar to all other buildbot
> configurations we have currently.
IMO "we already do this" is not a valid reason to start doing it here. I can't think of a single bug this check ever prevents, that wouldn't be caught by checking "do all workers on a builder queue have the same inferred platform as all other builders they may be associated with?" This would simplify the 'worker' entries greatly by allowing them to be organized in a list instead of multiple key-value pairs.

I have an implementation of this in-action if you want me to send you the code.
> 
> > > Tools/BuildSlaveSupport/ews-build/loadConfig.py:103
> > > +    keywords = filter(None, re.split("[, \-_:()]+", str(builder['name'])))
> > 
> > Is the `str` cast necessary here?
> Yeah, this is in order to convert from unicode to str. Otherwise tags are
> returned as Unicode and buildbot gives an error:
> builder 'iOS-11-EWS': tags list contains something that is not a string
Gotcha, thanks for explaining.
> Also added unit-test for this case.

[Aakash Jain]

> IMO "we already do this" is not a valid reason to start doing it here. I
> can't think of a single bug this check ever prevents, that wouldn't be
> caught by checking "do all workers on a builder queue have the same inferred
> platform as all other builders they may be associated with?" This would
> simplify the 'worker' entries greatly by allowing them to be organized in a
> list instead of multiple key-value pairs.
We will have to infer the platform in some other manner then. I think the approach you are suggesting would prevent from connecting one worker to multiple builders with different platform.

This check prevents the case when the bot-xyz connects to a single builder, and someone re-configures bot-xyz from "mac-sierra" to "mac-high-sierra", but forgets to update the corresponding builder.

[WebKit Commit Bot]

Comment on attachment 341141 [details]
Patch for landing

Clearing flags on attachment: 341141

Committed r232135: <https://trac.webkit.org/changeset/232135>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40503755>