Bug 154667 – Update twisted version in webkitpy.thirdparty.autoinstalled module.

• Component: New Bugs
• Status: RESOLVED FIXED

Update twisted version in webkitpy.thirdparty.autoinstalled module.

Created attachment 272171 [details]
Patch

[WebKit Commit Bot]

Attachment 272171 [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 on attachment 272171 [details]
Patch

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

> Tools/ChangeLog:11
> +

As far as I know, only benchmark runner uses twisted module in webkitpy module.

[Alexey Proskuryakov]

Comment on attachment 272171 [details]
Patch

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

>> Tools/ChangeLog:11
>> +
> 
> As far as I know, only benchmark runner uses twisted module in webkitpy module.

How can this be confirmed?

[Csaba Osztrogonác]

Comment on attachment 272171 [details]
Patch

Buildbot master uses an ancient buildbot version, which needs this old twisted. I added exactly this version to autoinstall to be able to run buildbot master.cfg unittest out of the box.

[Csaba Osztrogonác]

See https://bugs.webkit.org/show_bug.cgi?id=142219 for details.

[Csaba Osztrogonác]

Additionally this change is incorrect, because after this 
change autoinstaller still installs the old twisted version:

$ Tools/Scripts/test-webkitpy
Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really, slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

Checking autoinstalled packages ...Auto-installing package: jinja2
Auto-installing package: sqlalchemy
Auto-installing package: twisted
Auto-installing package: buildbot
Auto-installing package: coverage
Auto-installing package: eliza.py
Auto-installing package: mechanize
Auto-installing package: pep8.py
Auto-installing package: logilab.common
Auto-installing package: logilab.astng
Auto-installing package: pylint
Auto-installing package: twisted

$ cat Tools/Scripts/webkitpy/thirdparty/autoinstalled/twisted/.twisted.url
https://pypi.python.org/packages/source/T/Twisted/Twisted-12.1.0.tar.bz2#md5=f396f1d6f5321e869c2f89b2196a9eb5

The root of this problem is https://trac.webkit.org/changeset/187925
which took no account of already installed twisted by _install_buildbot

Created attachment 274048 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:12:  No name 'new_twisted' in module 'webkitpy.thirdparty.autoinstalled'  [pylint/E0611] [5]
Total errors found: 1 in 4 files


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

[Ryosuke Niwa]

Comment on attachment 274048 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:-32
> -        reactor.stop()

You should explain this change in the change log.

Created attachment 274052 [details]
Patch for landing

[Simon Fraser (smfr)]

Comment on attachment 274052 [details]
Patch for landing

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

> Tools/ChangeLog:8
> +        Add lasted twisted to webkitpy.thirdparty.autoinstalled.

lasted? latest?

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:12
> +    from webkitpy.thirdparty.autoinstalled.new_twisted import twisted

Using "new" in code is a mistake. In a year or two, it's no longer new and the read doesn't really know what changed. If it has a version number, say that explicitly.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:160
> +    def _install_new_twisted(self):

Don't say "new".

Created attachment 274062 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:12:  No name 'twisted_15_5_0' in module 'webkitpy.thirdparty.autoinstalled'  [pylint/E0611] [5]
Total errors found: 1 in 4 files


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

[Alexey Proskuryakov]

Comment on attachment 274062 [details]
Patch

Clearing r+ flag, since this has been landed and rolled out.

Additionally, it's not clear from bug comments if Ossy's concerns have been addressed.

Created attachment 291211 [details]
Patch

[Saam Barati]

*** Bug 163261 has been marked as a duplicate of this bug. ***

(In reply to comment #6)
> See https://bugs.webkit.org/show_bug.cgi?id=142219 for details.

Updated the patch. Could you take a look? Thanks.

[Carlos Alberto Lopez Perez]

I have tested this. It works fine on the GTK+ platform.

But I see that it won't auto-install twisted 15.0 or use it if the system already has python-twisted installed.

If you want to ensure this script always uses twisted 15.0 you can try something like:


--- a/Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py
+++ b/Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py
@@ -5,11 +5,8 @@ import logging
 import os
 import sys
 
-try:
-    import twisted
-except ImportError:
-    sys.path.append(os.path.abspath(os.path.join(os.path.dirname(os.path.abspath(__file__)), '../../../..')))
-    from webkitpy.thirdparty.autoinstalled.twisted import twisted
+sys.path.append(os.path.abspath(os.path.join(os.path.dirname(os.path.abspath(__file__)), '../../../..')))
+from webkitpy.thirdparty.autoinstalled.twisted_15_5_0 import twisted
 
 from twisted.web import static, server
 from twisted.web.resource import Resource




By the way.. what is the context for requiring twisted 15.0 here?
Is it necessary for some run-benchmark test?

[Ryosuke Niwa]

(In reply to comment #19)
>
> By the way.. what is the context for requiring twisted 15.0 here?
> Is it necessary for some run-benchmark test?

Yeah, older versions of twisted has a bug whereby which some benchmark page fails to load.

(In reply to comment #19)
> I have tested this. It works fine on the GTK+ platform.
> 
> But I see that it won't auto-install twisted 15.0 or use it if the system
> already has python-twisted installed.
> 
> If you want to ensure this script always uses twisted 15.0 you can try
> something like:
> 
> 
> ---
> a/Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/
> twisted_http_server.py
> +++
> b/Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/
> twisted_http_server.py
> @@ -5,11 +5,8 @@ import logging
>  import os
>  import sys
>  
> -try:
> -    import twisted
> -except ImportError:
> -   
> sys.path.append(os.path.abspath(os.path.join(os.path.dirname(os.path.
> abspath(__file__)), '../../../..')))
> -    from webkitpy.thirdparty.autoinstalled.twisted import twisted
> +sys.path.append(os.path.abspath(os.path.join(os.path.dirname(os.path.
> abspath(__file__)), '../../../..')))
> +from webkitpy.thirdparty.autoinstalled.twisted_15_5_0 import twisted
>  
>  from twisted.web import static, server
>  from twisted.web.resource import Resource
That sounds good to me. I think it's even better to check the version of twisted, if it is a version earlier than 15.5.0(this version is working for sure), we should always use autoinstalled module in webkitpy.thirdparty.
> 
> 
> 
> 
> By the way.. what is the context for requiring twisted 15.0 here?
> Is it necessary for some run-benchmark test?

Created attachment 293481 [details]
Patch

Created attachment 293504 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 293504 [details]
Patch for landing

Clearing flags on attachment: 293504

Committed r208205: <http://trac.webkit.org/changeset/208205>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.