Bug 185484

Created attachment 340011 [details] Proposed patch Patch for basic buildbot configuration. Doesn't have factories, steps etc yet. Tested locally.

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 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?

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

Created attachment 341011 [details] Updated patch Updated patch with unit-tests and review comments incorporated.

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 on attachment 341011 [details] Updated patch rs=me. Does the unit test get run on build.webkit.org bots?

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

Created attachment 341100 [details] Patch for landing

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

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.

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

> 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 on attachment 341141 [details] Patch for landing Clearing flags on attachment: 341141 Committed r232135: <https://trac.webkit.org/changeset/232135>

All reviewed patches have been landed. Closing bug.

<rdar://problem/40503755>