Bug 212009 – [ews] Split JSC MIPS queue into separate builder and tester queue

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Diego Pino]

[ews] Add build bots for JSC MIPS and ARMv7 archirectures

[Diego Pino]

Created attachment 399624 [details]
Patch

[Diego Pino]

Created attachment 399718 [details]
Patch

[Diego Pino]

Created attachment 399723 [details]
Patch

[Diego Pino]

Created attachment 399724 [details]
Patch

[Guillaume Emont]

As I commented on slack, a few words on why we want these additional bots. For mips+arm EWS bots, the bulk of the time is spent in running tests on (slow) target devices, while cross-compilation only takes a few minutes. Having a build-only EWS would allow developers to see more quickly build issues (which I think are a good part of the issues that we see detected by EWS), and to get quick feedback after they rework a patch to address the issue. The idea is that these build-only bots will only take a couple minutes to process a patch, and their queue would almost always be empty, so that new patches would be processed quickly on these.
Note also that this is in addition to our efforts to add more mips and arm devices to our EWS testbots so that these get faster too.

[Aakash Jain]

Comment on attachment 399724 [details]
Patch

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

Looks good in terms of functionality.

My biggest concern is UI. This would add two more status-bubbles. Do we really need to add two more status-bubbles for this, and are we ok with that?

> Tools/BuildSlaveSupport/ews-build/config.json:472
> +      "shortname": "jsc-build-mips",

This name will show up in status-bubble on bugzilla. Can we come up with a shorter name for all these status-bubbles?

maybe: jsc-mips and jsc-mips-tests

Also would need to update the shortnames in https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-app/ews/views/statusbubble.py#L46 so that they can start displaying in status-bubbles.

> Tools/BuildSlaveSupport/ews-build/config.json:483
> +      "shortname": "jsc-tests-mips",

Can we come up with a shorter name for all these status-bubbles?

> Tools/BuildSlaveSupport/ews-build/config.json:494
> +      "shortname": "jsc-armv7-build",

Can we come up with a shorter name for all these status-bubbles?

> Tools/BuildSlaveSupport/ews-build/config.json:505
> +      "shortname": "jsc-armv7-tests",

Can we come up with a shorter name for all these status-bubbles?

> Tools/BuildSlaveSupport/ews-build/config.json:601
> +                       "JSC-Build-ARMv7-32bits-EWS", "JSC-i386-32bits-EWS", "JSC-Build-MIPSEL-32bits-EWS", "JSC-Tests-EWS", "macOS-Mojave-Debug-Build-EWS",

Nit: let's keep it in alphabetical order.

> Tools/BuildSlaveSupport/ews-build/factories.py:140
> +class JSCTestOnlyFactory(Factory):

Let's rename this to JSCTestsFactory. Can rename it later in separate patch as well.

> Tools/BuildSlaveSupport/ews-build/factories.py:149
>  class JSCTestsFactory(Factory):

Let's rename this to JSCBuildAndTestsFactory. Can rename it later in separate patch as well.

[Aakash Jain]

Also it might be better to land patch in Bug 212803 first, make sure the new bots are working fine (no configuration issues), and then land this.

[Diego Pino]

Created attachment 402221 [details]
Patch

[Aakash Jain]

Comment on attachment 402221 [details]
Patch

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

r- due to services ews being red.

> Tools/BuildSlaveSupport/ews-build/config.json:17
> +      "name": "igalia-jsc32-mipsel-ews-03",

Let's add it in a separate patch and land it first (to ensure that the bot is working fine without any configuration isssues).

> Tools/BuildSlaveSupport/ews-build/config.json:491
> +      "shortname": "jsc-mips-t",

This name might be confusing for people. probably "jsc-mips-tests" is fine.

> Tools/BuildSlaveSupport/ews-build/config.json:513
> +      "shortname": "jsc-armv7-t",

Ditto. This name might be confusing for people. probably "jsc-armv7-tests" is fine.

[Diego Pino]

Created attachment 402267 [details]
Patch

[Diego Pino]

Created attachment 402558 [details]
Patch

[Aakash Jain]

Comment on attachment 402558 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:472
> +      "factory": "JSCTestsFactory",

I believe this is unintentional.

> Tools/BuildSlaveSupport/ews-build/factories.py:150
> +    def __init__(self, platform, configuration='release', architectures=None, remotes=None, additionalArguments=None, runTests='true', **kwargs):

runTests isn't required, please remove.

[Diego Pino]

Created attachment 402623 [details]
Patch

[Aakash Jain]

Comment on attachment 402623 [details]
Patch

In JSCBuildFactory.__init__ please add 'remotes' parameter and pass it to Factory.__init__ , otherwise triggers aren't working (as tested on uat instance https://ews-build.webkit-uat.org/#/builders/37/builds/47)

[Aakash Jain]

> In JSCBuildFactory.__init__ please add 'remotes' parameter
Typo, 'remotes' -> 'triggers'

[Diego Pino]

Created attachment 402823 [details]
Patch

[Diego Pino]

Created attachment 406851 [details]
Patch

[Diego Pino]

Created attachment 406852 [details]
Patch

[Aakash Jain]

Comment on attachment 406852 [details]
Patch

look good to me.

Also https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-app/ews/views/statusbubble.py would need updation as well in ALL_QUEUES and QUEUE_TRIGGERS. That can be done either in this or a separate patch.

[EWS]

Committed r265958: <https://trac.webkit.org/changeset/265958>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406852 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67500231>

[Aakash Jain]

Restarted EWS to pick up this change.