Bug 203946 - Setup EWS queues for JSConly 32bits ARMv7 and MIPSel
Summary: Setup EWS queues for JSConly 32bits ARMv7 and MIPSel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Paulo Matos
URL:
Keywords: InRadar
Depends on: 203899
Blocks: 204344
  Show dependency treegraph
 
Reported: 2019-11-07 02:53 PST by Paulo Matos
Modified: 2019-11-22 10:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.81 KB, patch)
2019-11-07 02:58 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2019-11-11 03:10 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (3.76 KB, patch)
2019-11-11 03:13 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (4.35 KB, patch)
2019-11-11 05:45 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2019-11-12 00:31 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (5.41 KB, patch)
2019-11-15 07:18 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (4.19 KB, patch)
2019-11-15 07:41 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (5.51 KB, patch)
2019-11-15 07:45 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (5.72 KB, patch)
2019-11-15 08:22 PST, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2019-11-19 02:20 PST, Paulo Matos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paulo Matos 2019-11-07 02:53:52 PST
Setup EWS queues for JSConly 32bits ARMv7 and MIPSel
Comment 1 Paulo Matos 2019-11-07 02:58:05 PST
Created attachment 383033 [details]
Patch
Comment 2 Guillaume Emont 2019-11-07 09:54:39 PST
Comment on attachment 383033 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:423
> +      "shortname": "mipsel-jsc32-only",

the name is a bit odd. Maybe mipsel-jsconly? or mipsel-jsc32? mipsel32-jsconly?  Unsure what's best, but the "-only" is a little ambiguous to me. I feel like mipsel-jsconly might be my favorite though, since mipsel is a name that AFAIK is only used to described mips32.

> Tools/BuildSlaveSupport/ews-build/config.json:434
> +      "shortname": "armv7-jsc32-only",

Same comment as above.
Comment 3 Aakash Jain 2019-11-07 09:58:36 PST
Comment on attachment 383033 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-build/config.json:423
>> +      "shortname": "mipsel-jsc32-only",
> 
> the name is a bit odd. Maybe mipsel-jsconly? or mipsel-jsc32? mipsel32-jsconly?  Unsure what's best, but the "-only" is a little ambiguous to me. I feel like mipsel-jsconly might be my favorite though, since mipsel is a name that AFAIK is only used to described mips32.

also, this shortname is the name which will show up in the status-bubble. so it would be better to keep it short (and something which people can easily understand).
Comment 4 Guillaume Emont 2019-11-07 10:04:49 PST
(In reply to Aakash Jain from comment #3)
> Comment on attachment 383033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383033&action=review
> 
> >> Tools/BuildSlaveSupport/ews-build/config.json:423
> >> +      "shortname": "mipsel-jsc32-only",
> > 
> > the name is a bit odd. Maybe mipsel-jsconly? or mipsel-jsc32? mipsel32-jsconly?  Unsure what's best, but the "-only" is a little ambiguous to me. I feel like mipsel-jsconly might be my favorite though, since mipsel is a name that AFAIK is only used to described mips32.
> 
> also, this shortname is the name which will show up in the status-bubble. so
> it would be better to keep it short (and something which people can easily
> understand).

Oh, then I think it makes most sense to keep the same names as the old queues: jsc-mips and jsc-armv7, to avoid anyone getting confused with a change of name.
Comment 5 Aakash Jain 2019-11-08 06:08:40 PST
Comment on attachment 383033 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:10
> +      "platform": "jsc-only",

can you please change this to 'jsconly' (without the hypen).

Also in the PrintConfiguration::run() in steps.py can you please add 'jsconly' along-with 'gtk' and 'wpe' to the linux platforms (in https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-build/steps.py#L1723). That would make sure we run the linux commands to print the bot configuration for this platform as well.

> Tools/BuildSlaveSupport/ews-build/config.json:11
> +      "max_builds": 1

max_builds isn't required, it's default value is 1, (configured in loadConfig.py).

> Tools/BuildSlaveSupport/ews-build/config.json:426
> +      "platform": "jsc-only",

Ditto about platform here.
Comment 6 Paulo Matos 2019-11-11 03:10:15 PST
Created attachment 383259 [details]
Patch
Comment 7 Paulo Matos 2019-11-11 03:13:42 PST
Created attachment 383260 [details]
Patch
Comment 8 Paulo Matos 2019-11-11 03:15:16 PST
(In reply to Aakash Jain from comment #5)
> Comment on attachment 383033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383033&action=review
> 
> > Tools/BuildSlaveSupport/ews-build/config.json:10
> > +      "platform": "jsc-only",
> 
> can you please change this to 'jsconly' (without the hypen).
> 
> Also in the PrintConfiguration::run() in steps.py can you please add
> 'jsconly' along-with 'gtk' and 'wpe' to the linux platforms (in
> https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-
> build/steps.py#L1723). That would make sure we run the linux commands to
> print the bot configuration for this platform as well.
> 
> > Tools/BuildSlaveSupport/ews-build/config.json:11
> > +      "max_builds": 1
> 
> max_builds isn't required, it's default value is 1, (configured in
> loadConfig.py).
> 
> > Tools/BuildSlaveSupport/ews-build/config.json:426
> > +      "platform": "jsc-only",
> 
> Ditto about platform here.

Thanks for the review, new patch fixes these issues and changes the queue names to follow Guillaume suggestions.

In testing, there is currently an issue where the --remote-config-file is not being added to the testing command line. Either this patch was not properly added to the testing master or there might be a bug in 203899.
Comment 9 Paulo Matos 2019-11-11 05:45:10 PST
Created attachment 383264 [details]
Patch
Comment 10 Paulo Matos 2019-11-12 00:31:58 PST
Created attachment 383342 [details]
Patch
Comment 11 Paulo Matos 2019-11-12 00:33:06 PST
I have added some extra flags: --jsc-only and --memory-limited to the test run. I am wondering if --memory-limited should instead be moved to a property of its own or kept as is. @Aakash, any suggestions here?
Comment 12 Aakash Jain 2019-11-12 05:14:01 PST
Comment on attachment 383342 [details]
Patch

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

(In reply to Paulo Matos from comment #11)
> I have added some extra flags: --jsc-only and --memory-limited to the test
> run. I am wondering if --memory-limited should instead be moved to a
> property of its own or kept as is. @Aakash, any suggestions here?
I think it's good to have memory-limited flag here, so that it's clear in the configuration.

Please add 'remotes' in valid_builder_keys in loadConfig_unittest.py.ConfigDotJSONTest::test_builder_keys() to pass the unit-tests.

> Tools/BuildSlaveSupport/ews-build/config.json:9
> +      "name": "igalia-jsconly-armv7_rpi3-ews",

Would be good to have shorter bot names.

> Tools/BuildSlaveSupport/ews-build/config.json:439
> +      "remotes": "../../EWS-test-devices.json"

What happens if this file is not present on the bot, is it clear from the failure that this is the cause?

> Tools/BuildSlaveSupport/ews-build/steps.py:945
> +            self.command.extend(['--no-testmasm', '--no-testair', '--no-testb3', '--no-testdfg', '--no-testapi', '--jsc-only', '--memory-limited'])

--jsc-only flag is automatically added based on platform by the appendCustomBuildFlags() in the next line.
Comment 13 Paulo Matos 2019-11-12 10:24:43 PST
Our ARMv7 worker seems to be working on the testing master: https://ews-build.webkit-uat.org/#/builders/22

Tomorrow I will start a mips worker.
Comment 14 Aakash Jain 2019-11-14 04:16:54 PST
Comment on attachment 383342 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:419
> +    {

Please move this code next to JSC block.

> Tools/BuildSlaveSupport/ews-build/config.json:517
> +                       "Services-EWS", "Style-EWS", "WebKitPerl-Tests-EWS", "WebKitPy-Tests-EWS", "WPE-EWS", "WinCairo-EWS", "JSCOnly-MIPSEL-32bits-EWS", "JSCOnly-ARMv7-32bits-EWS"]

This list is currently maintained in alphabetical order. Please move these accordingly.
Comment 15 Paulo Matos 2019-11-15 07:18:00 PST
Created attachment 383614 [details]
Patch
Comment 16 Paulo Matos 2019-11-15 07:41:25 PST
Created attachment 383615 [details]
Patch
Comment 17 Paulo Matos 2019-11-15 07:45:49 PST
Created attachment 383617 [details]
Patch
Comment 18 Jonathan Bedard 2019-11-15 07:50:58 PST
Comment on attachment 383617 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:10
> +      "platform": "jsc-only"

Aakash will need to verify this: But I think this well get the retry behavior by default, right? Seems like something we don't want for these queues, given our throughput problems.

I don't think this blocks this change, since I'm pretty sure these queues were getting retries with old EWS anyways, but I do want to bring it up.
Comment 19 Aakash Jain 2019-11-15 08:16:39 PST
Comment on attachment 383617 [details]
Patch

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

> Tools/ChangeLog:3
> +        Setup EWS queues for JSConly 32bits ARMv7 and MIPSel

What's the plan for jsc-i386 queue? Would that be added separately?

> Tools/BuildSlaveSupport/ews-build/steps.py:1888
>          platform = platform.split('-')[0]

Can you please add following if condition here:
if platform != 'jsc-only':
    platform = platform.split('-')[0]
Comment 20 Paulo Matos 2019-11-15 08:22:58 PST
Created attachment 383620 [details]
Patch
Comment 21 Paulo Matos 2019-11-15 08:23:32 PST
(In reply to Aakash Jain from comment #19)
> Comment on attachment 383617 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383617&action=review
> 
> > Tools/ChangeLog:3
> > +        Setup EWS queues for JSConly 32bits ARMv7 and MIPSel
> 
> What's the plan for jsc-i386 queue? Would that be added separately?
> 

That will be dealt with separately.

> > Tools/BuildSlaveSupport/ews-build/steps.py:1888
> >          platform = platform.split('-')[0]
> 
> Can you please add following if condition here:
> if platform != 'jsc-only':
>     platform = platform.split('-')[0]

Pushed that change with new patch.
Comment 22 Aakash Jain 2019-11-18 09:23:13 PST
Comment on attachment 383620 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:439
> +    

Nit: extra newline.

> Tools/BuildSlaveSupport/ews-build/config.json:441
> +      "name": "JSC-MIPSEL-32bits-EWS",

Previous patch has the builder name: "JSCOnly-MIPSEL-32bits-EWS", while this one changed "JSCOnly" to "JSC". Shortening the name is not a priority in builder names. I am fine either way. Just want to make sure that this is clear enough for you guys.
Comment 23 Radar WebKit Bug Importer 2019-11-18 12:08:34 PST
<rdar://problem/57292313>
Comment 24 Paulo Matos 2019-11-19 02:20:13 PST
Created attachment 383852 [details]
Patch
Comment 25 Paulo Matos 2019-11-19 02:21:47 PST
Aakash, I have added a new patch with the nit corrected. I have all the workers in place and tested in the UAT to get these into production. Sending you passwords privately when you get online.

Will also open a new bug to remove these from old EWS.
Comment 26 WebKit Commit Bot 2019-11-19 04:46:00 PST
Comment on attachment 383852 [details]
Patch

Clearing flags on attachment: 383852

Committed r252625: <https://trac.webkit.org/changeset/252625>
Comment 27 WebKit Commit Bot 2019-11-19 04:46:02 PST
All reviewed patches have been landed.  Closing bug.