Bug 185484 - Add Buildbot configuration for Opensource EWS
Summary: Add Buildbot configuration for Opensource EWS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-09 12:46 PDT by Aakash Jain
Modified: 2018-05-24 09:29 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch (13.95 KB, patch)
2018-05-09 12:58 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Updated patch (24.99 KB, patch)
2018-05-22 13:23 PDT, Aakash Jain
ap: review+
Details | Formatted Diff | Diff
Patch for landing (24.99 KB, patch)
2018-05-23 10:07 PDT, Aakash Jain
dbates: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (24.95 KB, patch)
2018-05-23 16:00 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2018-05-09 12:46:55 PDT
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.
Comment 1 Aakash Jain 2018-05-09 12:58:37 PDT
Created attachment 340011 [details]
Proposed patch

Patch for basic buildbot configuration. Doesn't have factories, steps etc yet. Tested locally.
Comment 2 EWS Watchlist 2018-05-09 13:01:11 PDT
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.
Comment 3 Dean Johnson 2018-05-14 21:22:47 PDT
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?
Comment 4 Aakash Jain 2018-05-22 13:22:00 PDT
(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.
Comment 5 Aakash Jain 2018-05-22 13:23:10 PDT
Created attachment 341011 [details]
Updated patch

Updated patch with unit-tests and review comments incorporated.
Comment 6 EWS Watchlist 2018-05-22 13:25:25 PDT
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.
Comment 7 Alexey Proskuryakov 2018-05-23 09:37:21 PDT
Comment on attachment 341011 [details]
Updated patch

rs=me. Does the unit test get run on build.webkit.org bots?
Comment 8 Aakash Jain 2018-05-23 10:07:12 PDT
> rs=me.
Thanks. 
> Does the unit test get run on build.webkit.org bots?
Not yet. Will add it later on.
Comment 9 Aakash Jain 2018-05-23 10:07:20 PDT
Created attachment 341100 [details]
Patch for landing
Comment 10 EWS Watchlist 2018-05-23 15:03:39 PDT
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.
Comment 11 Daniel Bates 2018-05-23 15:05:16 PDT
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
Comment 12 Daniel Bates 2018-05-23 15:17:01 PDT
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().
Comment 13 Aakash Jain 2018-05-23 16:00:57 PDT
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.
Comment 14 Dean Johnson 2018-05-23 16:05:28 PDT
(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.
Comment 15 Aakash Jain 2018-05-23 16:40:28 PDT
> 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.
Comment 16 WebKit Commit Bot 2018-05-23 16:41:49 PDT
Comment on attachment 341141 [details]
Patch for landing

Clearing flags on attachment: 341141

Committed r232135: <https://trac.webkit.org/changeset/232135>
Comment 17 WebKit Commit Bot 2018-05-23 16:41:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2018-05-23 16:42:25 PDT
<rdar://problem/40503755>