Summary: | Add setup script for Web Driver part of Benchmark Runner script | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matthew Stewart <matthew_r_stewart> | ||||||||||||||||
Component: | New Bugs | Assignee: | 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
Matthew Stewart
2017-07-10 16:44:55 PDT
*** Bug 174332 has been marked as a duplicate of this bug. *** *** Bug 174335 has been marked as a duplicate of this bug. *** Created attachment 315054 [details]
Patch
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 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 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 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 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
Created attachment 315508 [details]
Patch
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 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. Created attachment 315857 [details]
Add functions to autoinstaller needed for Benchmark Runner script
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. Created attachment 316011 [details]
patch
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 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. Created attachment 316116 [details]
patch
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 Created attachment 316333 [details]
patch
Comment on attachment 316333 [details] patch Clearing flags on attachment: 316333 Committed r219853: <http://trac.webkit.org/changeset/219853> All reviewed patches have been landed. Closing bug. |