Bug 147082

Summary: Install twisted package for http server when necessary.
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, glenn, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description dewei_zhu 2015-07-18 22:46:27 PDT
Install twisted package for http server when necessary.
Comment 1 dewei_zhu 2015-07-18 23:13:08 PDT
Created attachment 257054 [details]
Patch
Comment 2 Ryosuke Niwa 2015-07-19 00:13:44 PDT
Comment on attachment 257054 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:12
> +    from webkitpy.common.system.autoinstall import AutoInstaller

Why don't we modify __init__.py in autoinstalled instead?
What happens if we've already installed twisted in that directory by using buildbot?
Comment 3 dewei_zhu 2015-07-19 14:31:51 PDT
Comment on attachment 257054 [details]
Patch

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

>> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:12
>> +    from webkitpy.common.system.autoinstall import AutoInstaller
> 
> Why don't we modify __init__.py in autoinstalled instead?
> What happens if we've already installed twisted in that directory by using buildbot?

If we've installed twisted, we'll not catch ImportError.
If we modify __init__.py, how do we invoke the installation process? 
Do you mean we import autoinstall module in benchmark runner and it will check the installation of twisted and install it if necessary?
Comment 4 Ryosuke Niwa 2015-07-19 14:46:15 PDT
Comment on attachment 257054 [details]
Patch

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

>>> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:12
>>> +    from webkitpy.common.system.autoinstall import AutoInstaller
>> 
>> Why don't we modify __init__.py in autoinstalled instead?
>> What happens if we've already installed twisted in that directory by using buildbot?
> 
> If we've installed twisted, we'll not catch ImportError.
> If we modify __init__.py, how do we invoke the installation process? 
> Do you mean we import autoinstall module in benchmark runner and it will check the installation of twisted and install it if necessary?

But that's what we tried other day (importing autoinstall.buildbot) on my machine and we saw import didn't work.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:16
> +    installer = AutoInstaller(prepend_to_search_path=True, target_dir=install_path)
> +    installer.install(url="https://pypi.python.org/packages/source/T/Twisted/Twisted-12.1.0.tar.bz2#md5=f396f1d6f5321e869c2f89b2196a9eb5", url_subpath="Twisted-12.1.0/twisted")

There is already code that does this in __init__.py in webkitpy/thirdparty for when we import thirdparty.autoinstall.twisted.
Why don't we make that code more generic to do this instead?

It appears to me that having to download & install twisted every time you run benchmark is somewhat inefficient/redundant.
I already hate the fact some of the benchmark plans require downloading the benchmark and we should add some code to cache the downloaded content.
Comment 5 dewei_zhu 2015-08-04 16:26:15 PDT
Created attachment 258229 [details]
Patch
Comment 6 WebKit Commit Bot 2015-08-04 17:46:56 PDT
Comment on attachment 258229 [details]
Patch

Clearing flags on attachment: 258229

Committed r187925: <http://trac.webkit.org/changeset/187925>
Comment 7 WebKit Commit Bot 2015-08-04 17:46:59 PDT
All reviewed patches have been landed.  Closing bug.