Bug 174331

Summary: Add setup script for Web Driver part of Benchmark Runner script
Product: WebKit Reporter: Matthew Stewart <matthew_r_stewart>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, buildbot, commit-queue, dean_johnson, dewei_zhu, glenn, slewis
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=177205
Bug Depends on: 174445    
Bug Blocks: 174056    
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews115 for mac-elcapitan
none
Patch
none
Add functions to autoinstaller needed for Benchmark Runner script
none
patch
none
patch
slewis: review+, commit-queue: commit-queue-
patch none

Description Matthew Stewart 2017-07-10 16:44:55 PDT
Add setup script for Web Driver part of Benchmark Runner script
Comment 1 Matthew Stewart 2017-07-10 17:15:29 PDT
*** Bug 174332 has been marked as a duplicate of this bug. ***
Comment 2 Matthew Stewart 2017-07-10 17:15:59 PDT
*** Bug 174335 has been marked as a duplicate of this bug. ***
Comment 3 Matthew Stewart 2017-07-10 17:33:41 PDT
Created attachment 315054 [details]
Patch
Comment 4 Dean Johnson 2017-07-10 18:17:01 PDT
Comment on attachment 315054 [details]
Patch

Nice patch Matthew! I think you may have done a little more work than would be required for this, though. Have you had a chance to look into "autoinstalled" module in webkitpy? Essentially, you just define a very short function containing a URL to download a package from along with what the package name should be, and if you do `from webkitpy.autoinstalled.thirdparty import selenium` it would automatically download, install and source the import for you!

You can see an example of this running `git grep mechanize` from OpenSource/Tools/Scripts/webkitpy:
common/net/bugzilla/bugzilla.py:            from webkitpy.thirdparty.autoinstalled.mechanize import Browser.

Feel free to follow-up if you have any questions!
Comment 5 Dean Johnson 2017-07-10 18:17:16 PDT
Comment on attachment 315054 [details]
Patch

Nice patch Matthew! I think you may have done a little more work than would be required for this, though. Have you had a chance to look into "autoinstalled" module in webkitpy? Essentially, you just define a very short function containing a URL to download a package from along with what the package name should be, and if you do `from webkitpy.autoinstalled.thirdparty import selenium` it would automatically download, install and source the import for you!

You can see an example of this running `git grep mechanize` from OpenSource/Tools/Scripts/webkitpy:
common/net/bugzilla/bugzilla.py:            from webkitpy.thirdparty.autoinstalled.mechanize import Browser.

Feel free to follow-up if you have any questions!
Comment 6 Stephanie Lewis 2017-07-10 18:21:01 PDT
Comment on attachment 315054 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/__init__.py:29
>  

This needs to have a proper error message.

> Tools/Scripts/webkitpy/benchmark_runner/setup.py:15
> +

Constants should be uppercase

> Tools/Scripts/webkitpy/benchmark_runner/setup.py:39
> +        if subprocess.call(['curl', '--silent', '--location', '--remote-name', 'https://bootstrap.pypa.io/get-pip.py'], shell=False):

Check with Dean but I believe we should be installing pip using easy_install.  This won't work in the lab due to the firewalls.

> Tools/Scripts/webkitpy/benchmark_runner/setup.py:52
> +        output = subprocess.check_output(['chromedriver', '--version']).strip()

Lets break the version check out into its own function.  Bonus points if firefox and chrome can use the same function.

> Tools/Scripts/webkitpy/benchmark_runner/setup.py:128
> +        print("Note: This script may not put the binaries in the correct place\nYou'll have to move them manually")

Where will it put them and where do they need to be.  Let's make these instructions detailed.
Comment 7 Build Bot 2017-07-10 18:31:35 PDT
Comment on attachment 315054 [details]
Patch

Attachment 315054 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4096716

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html
Comment 8 Build Bot 2017-07-10 18:31:36 PDT
Created attachment 315059 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Matthew Stewart 2017-07-14 17:38:57 PDT
Created attachment 315508 [details]
Patch
Comment 10 Stephanie Lewis 2017-07-17 15:41:24 PDT
Comment on attachment 315508 [details]
Patch

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

Looks good

> Tools/Scripts/webkitpy/thirdparty/__init__.py:180
> +            open(os.path.join(directory, '__init__.py'), 'w+')

This probably needs to be closed
Comment 11 Dean Johnson 2017-07-17 18:11:54 PDT
Comment on attachment 315508 [details]
Patch

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

Great work so far!

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:22
> +driver_executable = os.path.join(os.path.dirname(os.path.realpath(driver_init_file)), 'chromedriver')

Can you adopt this for geckodriver and chromedriver above, too? This seems very concise!

> Tools/Scripts/webkitpy/thirdparty/__init__.py:173
> +        version = subprocess.check_output(['curl', '--silent', CHROME_DRIVER_URL + "LATEST_RELEASE"]).strip()

We should use urllib2 here instead of subprocess. urllib2.urlopen(url).read() I believe should be all that's needed.

It may also be useful to have a function called install_binary (without a prepended _ to avoid getting captured in autoinstall_everything()), which you pass a url and it will download the binary and initialize it as a mock python object.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:174
> +        filename_postfix = self.get_driver_filename()[0]

Nit, instead of using indices, consider using _ as a placeholder like so:
filename_postfix, _ = self.get_driver_filename()

> Tools/Scripts/webkitpy/thirdparty/__init__.py:186
> +        filename_postfix = self.get_driver_filename()[1]

Ditto to _ comment.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:192
> +            open(os.path.join(directory, '__init__.py'), 'w+')

I think utilizing install_binary will reduce much of the duplication here.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:200
> +        import json

Nit: These imports can be done at the top of the file.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:202
> +        response = urlopen(json_url)

Nit: Usually only urllib2 functions all called from urllib2 rather than being imported directly. Example: urllib2.urlopen.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:208
> +    def get_driver_filename(self):

We should put this outside of this class since it's not dependent on self (except for get_os_info).

I would recommend rewriting this function for conciseness / flatness as follows:
def get_os_info():
  (os_name, os_type) = get_os_info()
  chrome_os, filefox_os = 'unsupported', 'unsupported'
  if os_name == 'Linux' and os_type == '64':
    chrome_os, firefox_os = 'linux64', 'linux64'
  elif os_name == 'Linux':
    chrome_os, firefox_os = 'linux32', 'linux32'
  elif os_name == 'Darwin':
    chrome_os, firefox_os = 'mac64', 'macos'
  return (chrome_os, firefox_os)

> Tools/Scripts/webkitpy/thirdparty/__init__.py:224
> +    def get_os_info(self):

Ditto to get_driver_filename. We should put this outside of this class.
Comment 12 Matthew Stewart 2017-07-18 16:40:22 PDT
Created attachment 315857 [details]
Add functions to autoinstaller needed for Benchmark Runner script
Comment 13 dewei_zhu 2017-07-18 17:49:55 PDT
Comment on attachment 315857 [details]
Add functions to autoinstaller needed for Benchmark Runner script

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_chrome_driver.py:28
> +import webkitpy.thirdparty.autoinstalled.chromedriver

As we discussed in person, we should avoid installing those binaries every time we call run-benchmark script.
A lazy installation is preferred.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:22
> +driver_executable = os.path.join(os.path.dirname(os.path.realpath(driver_init_file)), 'chromedriver')

Maybe worth abstract this function into utils.py since this has been called multiple times.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:33
> +import json

Nit: sort import by alphabet.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:170
> +        (url, url_subpath) = self.get_latest_pypi_url('selenium')

No need to change, but worth to mention that braces are not necessary here.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:226
> +def get_driver_filename():
> +    os_name, os_type = get_os_info()
> +    chrome_os, filefox_os = 'unsupported', 'unsupported'
> +    if os_name == 'Linux' and os_type == '64':
> +        chrome_os, firefox_os = 'linux64', 'linux64'
> +    elif os_name == 'Linux':
> +        chrome_os, firefox_os = 'linux32', 'linux32'
> +    elif os_name == 'Darwin':
> +        chrome_os, firefox_os = 'mac64', 'macos'
> +    return (chrome_os, firefox_os)

Would it better to become a function that takes an argument and returns the platform string base on the argument?
This could make it less coupled. Otherwise, we are making an assumption on the order of returned tuple.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:232
> +    return (os_name, os_type)

No need to change, but worth to mention that braces are not necessary here.
Comment 14 Matthew Stewart 2017-07-20 12:13:57 PDT
Created attachment 316011 [details]
patch
Comment 15 dewei_zhu 2017-07-20 14:32:12 PDT
Comment on attachment 316011 [details]
patch

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

> Tools/ChangeLog:6
> +        Reviewed by Dean Johnson.

r=me

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver.py:38
> +    def get_webdriver_binary_path(self):
> +        return get_driver_binary_path(self.browser_name)

This looks good enough. It would even better if we use @property here since it makes the code cleaner. Then rather than calling it get_webdriver_binary_path, we can just call it webdriver_binary_path,
Comment 16 dewei_zhu 2017-07-20 17:06:41 PDT
Comment on attachment 316011 [details]
patch

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

> Tools/Scripts/webkitpy/thirdparty/__init__.py:226
> +    return (chrome_os, firefox_os)

Using namedtuple would improve the readability of this code and get rid of the assumption we made on the indices of the return value.
DriverFilenameForBrowser = namedtuple('DriverFilenameForBrowser', ['chrome', 'firefox'])
return DriverFilenameForBrowser((chrome_os, firefox_os))
And update the use of this function's return value accordingly.
Comment 17 Matthew Stewart 2017-07-21 13:16:20 PDT
Created attachment 316116 [details]
patch
Comment 18 WebKit Commit Bot 2017-07-24 17:16:36 PDT
Comment on attachment 316116 [details]
patch

Rejecting attachment 316116 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 316116, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
tpy/benchmark_runner/browser_driver/osx_firefox_driver.py.rej
patching file Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py
patching file Tools/Scripts/webkitpy/benchmark_runner/utils.py
patching file Tools/Scripts/webkitpy/common/system/autoinstall.py
patching file Tools/Scripts/webkitpy/thirdparty/__init__.py

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Stephanie Lewis']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4181297
Comment 19 Matthew Stewart 2017-07-24 18:01:24 PDT
Created attachment 316333 [details]
patch
Comment 20 WebKit Commit Bot 2017-07-24 18:42:07 PDT
Comment on attachment 316333 [details]
patch

Clearing flags on attachment: 316333

Committed r219853: <http://trac.webkit.org/changeset/219853>
Comment 21 WebKit Commit Bot 2017-07-24 18:42:09 PDT
All reviewed patches have been landed.  Closing bug.