Setup EWS queues for JSConly 32bits ARMv7 and MIPSel
Created attachment 383033 [details] Patch
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 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).
(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 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.
Created attachment 383259 [details] Patch
Created attachment 383260 [details] Patch
(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.
Created attachment 383264 [details] Patch
Created attachment 383342 [details] Patch
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 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.
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 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.
Created attachment 383614 [details] Patch
Created attachment 383615 [details] Patch
Created attachment 383617 [details] Patch
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 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]
Created attachment 383620 [details] Patch
(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 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.
<rdar://problem/57292313>
Created attachment 383852 [details] Patch
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 on attachment 383852 [details] Patch Clearing flags on attachment: 383852 Committed r252625: <https://trac.webkit.org/changeset/252625>
All reviewed patches have been landed. Closing bug.