Bug 150859 – run-webkit-test should look in --root directory for LayoutTestRelay

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jason Marcell]

For iOS run-webkit-tests, Use LayoutTestRelay specified by --root; otherwise find LayoutTestRelay in Mac build directory when --root not specified.

[Jason Marcell]

Created attachment 264734 [details]
Patch

[WebKit Commit Bot]

Attachment 264734 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jason Marcell]

Created attachment 264737 [details]
Patch

[Daniel Bates]

Comment on attachment 264737 [details]
Patch

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

> Tools/ChangeLog:4
> +        For iOS run-webkit-tests, Use LayoutTestRelay specified by --root; otherwise find
> +        LayoutTestRelay in Mac build directory when --root not specified.

Please move this description such that it is under the Reviewed by line and add the bug title "run-webkit-test should look in --root directory for LayoutTestRelay" here.

[Daniel Bates]

Comment on attachment 264737 [details]
Patch

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

> Tools/ChangeLog:11
> +        (IOSSimulatorPort.relay_path): For iOS run-webkit-tests, Use LayoutTestRelay specified by
> +        --root; otherwise find LayoutTestRelay in Mac build directory when --root not specified.

We shouldn't repeat the description here.

[Jason Marcell]

Created attachment 264741 [details]
Patch

[Jason Marcell]

Created attachment 264743 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 264743 [details]
Patch

Clearing flags on attachment: 264743

Committed r191995: <http://trac.webkit.org/changeset/191995>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke iOS tests, rolling out:

https://build.webkit.org/builders/Apple%20iOS%209%20Simulator%20Release%20WK1%20%28Tests%29/builds/918/steps/layout-test/logs/stdio

[WebKit Commit Bot]

Re-opened since this is blocked by bug 150880

[Jason Marcell]

The property `_root_was_set` gets set in the base class around [Line 139](https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/base.py#L139).

The first time any port object is instantiated, this property is truthful in that it indicates whether `--root` was passed in on the command line.

However around [line 1135](https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/base.py#L1135) there is a method called `_build_path` which, unfortunately, mutates the `options` dictionary.

If `_build_path` is called on *any* port instance, then any subsequently instantiated port instance will have `_root_was_set` be set to true.

In order to fix this, we need to understand why the `options` dictionary is being mutated to propagate this information and find a better way to do this.

[Jason Marcell]

Created attachment 264882 [details]
Patch

[Daniel Bates]

Comment on attachment 264882 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:1145
> +            # We take advantage of the options being passed to workers as a way to precompute the root
> +            # directory path to pass to subprocesses and avoid making the slow call to config.build_directory()
> +            # N times in each worker.

Maybe a better way to write this would be:

We take advantage of the behavior that self._options is passed by reference to worker subprocesses to use it as data store to cache the computed root directory path. This avoids making each worker subprocess compute this path again which is slow because of the call to config.build_directory().

> Tools/Scripts/webkitpy/port/base.py:1150
> +            self.set_option_default('_cached_root', root_directory)

Can we use self.set_option() here instead of self.set_option_default()?

[Jason Marcell]

Comment on attachment 264882 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/base.py:1150
>> +            self.set_option_default('_cached_root', root_directory)
> 
> Can we use self.set_option() here instead of self.set_option_default()?

I'm double checking.

self.set_option_default() is defined as follows:

    def set_option_default(self, name, default_value):
        return self._options.ensure_value(name, default_value)

And ensure_value is defined as follows:

    def ensure_value(self, attr, value):
        if not hasattr(self, attr) or getattr(self, attr) is None:
            setattr(self, attr, value)
        return getattr(self, attire)

So, we only set it if it doesn't have the attribute or the attribute is None.

We've already established that self.get_option('_cached_root') was false-y (that's how we got into this branch of code to begin with).

Therefore it should be equivalent to use self.set_option()

[Jason Marcell]

Created attachment 264894 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 264894 [details]
Patch

Clearing flags on attachment: 264894

Committed r192087: <http://trac.webkit.org/changeset/192087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This seems to have broken Windows EWS:

Traceback (most recent call last):
 File "/home/buildbot/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
   main()
 File "/home/buildbot/WebKit/Tools/Scripts/webkit-patch", line 79, in main
   WebKitPatch(os.path.abspath(__file__)).main()
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
   result = command.check_arguments_and_execute(options, args, self)
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
   return self.execute(options, args, tool) or 0
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/tool/commands/queues.py", line 153, in execute
   return engine(self.name, self, self._tool.wakeup_event, self._options.seconds_to_sleep).run()
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/tool/bot/queueengine.py", line 91, in run
   self._delegate.begin_work_queue()
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py", line 56, in begin_work_queue
   self._layout_test_results_reader = LayoutTestResultsReader(self._tool, self._port.results_directory(), self._log_directory())
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/port/base.py", line 777, in results_directory
   option_val = self.get_option('results_directory') or self.default_results_directory()
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/port/base.py", line 791, in default_results_directory
   return self._build_path('layout-test-results')
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/port/win.py", line 134, in_build_path
   root_directory = self._filesystem.join(self.get_option('root'), binary_directory)
 File "/home/buildbot/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py", line 151, in join
   return os.path.join(*comps)
 File "/usr/lib/python2.7/posixpath.py", line 77, in join
   elif path == '' or path.endswith('/'):
AttributeError: 'NoneType' object has no attribute 'endswith'

[Daniel Bates]

Committed attempt to fix the Windows EWS build in <http://trac.webkit.org/changeset/192137>.