Bug 150859

Summary: run-webkit-test should look in --root directory for LayoutTestRelay
Product: WebKit Reporter: Jason Marcell <jmarcell>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, commit-queue, dbates, glenn, jmarcell, lforschler
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150880    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jason Marcell 2015-11-03 15:07:07 PST
For iOS run-webkit-tests, Use LayoutTestRelay specified by --root; otherwise find LayoutTestRelay in Mac build directory when --root not specified.
Comment 1 Jason Marcell 2015-11-03 15:08:09 PST
Created attachment 264734 [details]
Patch
Comment 2 WebKit Commit Bot 2015-11-03 15:11:00 PST
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.
Comment 3 Jason Marcell 2015-11-03 15:24:35 PST
Created attachment 264737 [details]
Patch
Comment 4 Daniel Bates 2015-11-03 15:35:57 PST
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.
Comment 5 Daniel Bates 2015-11-03 15:37:58 PST
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.
Comment 6 Jason Marcell 2015-11-03 15:43:10 PST
Created attachment 264741 [details]
Patch
Comment 7 Jason Marcell 2015-11-03 15:51:37 PST
Created attachment 264743 [details]
Patch
Comment 8 WebKit Commit Bot 2015-11-03 16:40:22 PST
Comment on attachment 264743 [details]
Patch

Clearing flags on attachment: 264743

Committed r191995: <http://trac.webkit.org/changeset/191995>
Comment 9 WebKit Commit Bot 2015-11-03 16:40:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Commit Bot 2015-11-03 21:43:05 PST
Re-opened since this is blocked by bug 150880
Comment 12 Jason Marcell 2015-11-05 11:09:23 PST
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.
Comment 13 Jason Marcell 2015-11-05 14:30:12 PST
Created attachment 264882 [details]
Patch
Comment 14 Daniel Bates 2015-11-05 14:44:06 PST
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()?
Comment 15 Jason Marcell 2015-11-05 15:01:26 PST
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()
Comment 16 Jason Marcell 2015-11-05 15:14:55 PST
Created attachment 264894 [details]
Patch
Comment 17 WebKit Commit Bot 2015-11-05 16:07:50 PST
Comment on attachment 264894 [details]
Patch

Clearing flags on attachment: 264894

Committed r192087: <http://trac.webkit.org/changeset/192087>
Comment 18 WebKit Commit Bot 2015-11-05 16:07:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Alexey Proskuryakov 2015-11-07 19:45:33 PST
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'
Comment 20 Daniel Bates 2015-11-07 20:05:01 PST
Committed attempt to fix the Windows EWS build in <http://trac.webkit.org/changeset/192137>.