Bug 203899 – Add support for remotes file for EWS builders

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

[Paulo Matos]

Add support for remotes file for EWS builders

[Paulo Matos]

Created attachment 382925 [details]
Patch

[Aakash Jain]

Comment on attachment 382925 [details]
Patch

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

Logic looks fine.

Please fix the unit-tests as well. You can run them using 'python Tools/BuildSlaveSupport/ews-build/runUnittests.py'

> Tools/BuildSlaveSupport/ews-build/steps.py:929
> +    command = ['perl', 'Tools/Scripts/run-javascriptcore-tests', '--no-build', '--no-fail-fast', '--json-output={0}'.format(jsonFileName), WithProperties('--%(configuration)s')] + [WithProperties('--remote-config-file=%(remotes)s')] if WithProperties('%(remotes)s') != '' else []

is this if-else correct? Please check what does command contains in case of else.

[Paulo Matos]

Thanks for the comments - I will take a look at the unit tests. Didn't know we had them.

[Paulo Matos]

python Tools/BuildSlaveSupport/ews-build/runUnittests.py
E.......................
======================================================================
ERROR: steps_unittest (unittest.loader.ModuleImportFailure)
----------------------------------------------------------------------
ImportError: Failed to import test module: steps_unittest
Traceback (most recent call last):
  File "/usr/lib64/python2.7/unittest/loader.py", line 254, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib64/python2.7/unittest/loader.py", line 232, in _get_module_from_name
    __import__(name)
  File "/home/pmatos/Projects/igalia/WebKit/Tools/BuildSlaveSupport/ews-build/steps_unittest.py", line 30, in <module>
    from buildbot.test.fake.remotecommand import Expect, ExpectRemoteRef, ExpectShell
ImportError: No module named test.fake.remotecommand


Are you using buildbot 1.8.2?

[Paulo Matos]

(In reply to Aakash Jain from comment #2)
> Comment on attachment 382925 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382925&action=review
> 
> Logic looks fine.
> 
> Please fix the unit-tests as well. You can run them using 'python
> Tools/BuildSlaveSupport/ews-build/runUnittests.py'
> 

No updates seem to be required as the unittests don't call into ConfigureBuild.

[Paulo Matos]

Created attachment 383035 [details]
Patch

[Paulo Matos]

Created attachment 383037 [details]
Patch

[Paulo Matos]

Created attachment 383038 [details]
Patch

[Paulo Matos]

I am having some issues getting ahold of the property in the RunJavaScriptCoreTests step. Need to understand the best way to do this.

[Aakash Jain]

Comment on attachment 383038 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/steps.py:937
> +        # add remotes configuration file path to the command line if needed

start() is a better place to add parameters to self.command. If I remember correctly, you will need to use self.getProperty('remotes') instead of WithProperties/util.Property().

[Aakash Jain]

(In reply to Paulo Matos from comment #4)
> Are you using buildbot 1.8.2?
Yes, see https://ews-build.webkit.org/#/about
Did you used '--no-binary :all:' while installing buildbot, as per: https://trac.webkit.org/wiki/EarlyWarningSystem#Installingrequiredpackages:

(In reply to Paulo Matos from comment #5)
> No updates seem to be required as the unittests don't call into ConfigureBuild.
The unit-test failures are in RunJavaScriptCoreTests (and related) unit-tests. See the red services ews bubble https://ews-build.webkit.org/#/builders/20/builds/7508

[Aakash Jain]

(In reply to Paulo Matos from comment #9)
> I am having some issues getting ahold of the property in the
> RunJavaScriptCoreTests step. Need to understand the best way to do this.
s
__init__ is called at the very beginning, when buildbot starts. While start() is called after the build, and the corresponding build-step has started. So at the time start() is called, all the build properties are already set, and you can simply use self.getProperty().

We modify self.command inside start() in our current code as well, e.g.: in CompileWebKit::start(), and even inside RunJavaScriptCoreTests:: start() using appendCustomBuildFlags().

[Paulo Matos]

Created attachment 383042 [details]
Patch

[Paulo Matos]

Created attachment 383044 [details]
Patch

[Paulo Matos]

(In reply to Aakash Jain from comment #12)
> (In reply to Paulo Matos from comment #9)
> > I am having some issues getting ahold of the property in the
> > RunJavaScriptCoreTests step. Need to understand the best way to do this.
> s
> __init__ is called at the very beginning, when buildbot starts. While
> start() is called after the build, and the corresponding build-step has
> started. So at the time start() is called, all the build properties are
> already set, and you can simply use self.getProperty().
> 
> We modify self.command inside start() in our current code as well, e.g.: in
> CompileWebKit::start(), and even inside RunJavaScriptCoreTests:: start()
> using appendCustomBuildFlags().

Thanks for the quick review and comments. r+ and cq+ pls if all is good.

[Aakash Jain]

(In reply to Paulo Matos from comment #15)
> Thanks for the quick review and comments. r+ and cq+ pls if all is good.
Code looks good. Can you just move the unit-tests inside TestRunJavaScriptCoreTests? Usually we have a class in steps_unittest corresponding to a class in steps.

[Paulo Matos]

Created attachment 383047 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 383047 [details]
Patch

Clearing flags on attachment: 383047

Committed r252184: <https://trac.webkit.org/changeset/252184>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56983361>

[Aakash Jain]

We need to pass 'remotes' parameter in JSCTestsFactory.__init__() as well.

Also, it's probably better to use named arguments while using Factory.__init().

e.g.:
class JSCTestsFactory(Factory):
    def __init__(self, platform, configuration='release', architectures=None, remotes=None, additionalArguments=None, runTests='true', **kwargs):
        Factory.__init__(self, platform=platform, configuration=configuration, architectures=architectures, buildOnly=False, remotes=remotes, additionalArguments=additionalArguments, checkRelevance=True)

[Aakash Jain]

(In reply to Aakash Jain from comment #21)
> We need to pass 'remotes' parameter in JSCTestsFactory.__init__() as well.
Done in https://trac.webkit.org/changeset/252454/webkit