Bug 203946 – Setup EWS queues for JSConly 32bits ARMv7 and MIPSel

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

[Paulo Matos]

Setup EWS queues for JSConly 32bits ARMv7 and MIPSel

[Paulo Matos]

Created attachment 383033 [details]
Patch

[Guillaume Emont]

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.

[Aakash Jain]

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

[Guillaume Emont]

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

[Aakash Jain]

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.

[Paulo Matos]

Created attachment 383259 [details]
Patch

[Paulo Matos]

Created attachment 383260 [details]
Patch

[Paulo Matos]

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

[Paulo Matos]

Created attachment 383264 [details]
Patch

[Paulo Matos]

Created attachment 383342 [details]
Patch

[Paulo Matos]

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?

[Aakash Jain]

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.

[Paulo Matos]

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.

[Aakash Jain]

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.

[Paulo Matos]

Created attachment 383614 [details]
Patch

[Paulo Matos]

Created attachment 383615 [details]
Patch

[Paulo Matos]

Created attachment 383617 [details]
Patch

[Jonathan Bedard]

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.

[Aakash Jain]

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]

[Paulo Matos]

Created attachment 383620 [details]
Patch

[Paulo Matos]

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

[Aakash Jain]

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.

[Radar WebKit Bug Importer]

<rdar://problem/57292313>

[Paulo Matos]

Created attachment 383852 [details]
Patch

[Paulo Matos]

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.

[WebKit Commit Bot]

Comment on attachment 383852 [details]
Patch

Clearing flags on attachment: 383852

Committed r252625: <https://trac.webkit.org/changeset/252625>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.