Bug 199710 - Remove pywebsockets from thirdparty
Summary: Remove pywebsockets from thirdparty
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-11 08:40 PDT by Carlos Garcia Campos
Modified: 2019-07-19 01:36 PDT (History)
8 users (show)

See Also:


Attachments
Patch (322.44 KB, patch)
2019-07-11 08:42 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (323.11 KB, patch)
2019-07-17 02:53 PDT, Carlos Garcia Campos
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.21 MB, application/zip)
2019-07-17 04:01 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.76 MB, application/zip)
2019-07-17 04:11 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.99 MB, application/zip)
2019-07-17 04:43 PDT, EWS Watchlist
no flags Details
Patch (327.64 KB, patch)
2019-07-18 07:35 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-highsierra (3.11 MB, application/zip)
2019-07-18 09:23 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-07-11 08:40:51 PDT
We have a very old version of pywebsocket in webkitpy thirdparty, but we are also importing pywebsocket as part of wpt tools. We can simply use the wpt one for all websocket tests and remove the old copy from sources.
Comment 1 Carlos Garcia Campos 2019-07-11 08:42:54 PDT
Created attachment 373924 [details]
Patch
Comment 2 Carlos Garcia Campos 2019-07-11 09:30:21 PDT
It seems to be failing to start the websockets sever in the bots, but there's no much info in the output. It's working for me locally...
Comment 3 Ryan Haddad 2019-07-16 13:20:15 PDT
I released the patch from the unhappy EWS queues since it was causing them to retry in a loop.
Comment 4 Carlos Garcia Campos 2019-07-17 02:52:47 PDT
Traceback (most recent call last):
  File "/Volumes/external/cgarcia/WebKit/LayoutTests/imported/w3c/web-platform-tests/tools/pywebsocket/mod_pywebsocket/standalone.py", line 177, in <module>
    from mod_pywebsocket import common
  File "/Volumes/external/cgarcia/WebKit/LayoutTests/imported/w3c/web-platform-tests/tools/pywebsocket/mod_pywebsocket/common.py", line 35, in <module>
    from mod_pywebsocket import http_header_util
  File "/Volumes/external/cgarcia/WebKit/LayoutTests/imported/w3c/web-platform-tests/tools/pywebsocket/mod_pywebsocket/http_header_util.py", line 256, in <module>
    urllib.parse.uses_netloc.index('ws')
AttributeError: 'Module_six_moves_urllib_parse' object has no attribute 'uses_netloc'

It seems mac has an old version of six. We need to ensure we use six from wpt tools thirdparty dir too.
Comment 5 Carlos Garcia Campos 2019-07-17 02:53:07 PDT
Created attachment 374283 [details]
Patch
Comment 6 EWS Watchlist 2019-07-17 04:01:18 PDT
Comment on attachment 374283 [details]
Patch

Attachment 374283 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12757338

New failing tests:
http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html
Comment 7 EWS Watchlist 2019-07-17 04:01:20 PDT
Created attachment 374284 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 8 Carlos Garcia Campos 2019-07-17 04:03:53 PDT
(In reply to Build Bot from comment #6)
> Comment on attachment 374283 [details]
> Patch
> 
> Attachment 374283 [details] did not pass mac-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12757338
> 
> New failing tests:
> http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-
> headers.html

Interesting, it could be that this test depends on old impl of pywebsockets somehow. I'll investigate.
Comment 9 EWS Watchlist 2019-07-17 04:11:34 PDT
Comment on attachment 374283 [details]
Patch

Attachment 374283 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12757348

New failing tests:
http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html
Comment 10 EWS Watchlist 2019-07-17 04:11:36 PDT
Created attachment 374285 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 11 EWS Watchlist 2019-07-17 04:43:05 PDT
Comment on attachment 374283 [details]
Patch

Attachment 374283 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12757362

New failing tests:
http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html
Comment 12 EWS Watchlist 2019-07-17 04:43:06 PDT
Created attachment 374289 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 13 Carlos Garcia Campos 2019-07-18 07:02:16 PDT
The thing is that now the server is not sending the close message back after we close the connection in this test. So, we end up waiting for a long time in m_closingTimer (TCPMaximumSegmentLifetime * 2). I've tested it with chromium and the test takes 60 seconds to finish (which is the closing timer timeout used by chromium). I still don't know why this is happening with this test only.
Comment 14 Carlos Garcia Campos 2019-07-18 07:15:12 PDT
Other tests using handshake.AbortedByUserException() are either expected failures or don't close the socket, just considering that onopen called is a success. We can do the same for this one.
Comment 15 Carlos Garcia Campos 2019-07-18 07:35:12 PDT
Created attachment 374383 [details]
Patch
Comment 16 EWS Watchlist 2019-07-18 09:23:11 PDT
Comment on attachment 374383 [details]
Patch

Attachment 374383 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12766458

New failing tests:
storage/indexeddb/dont-wedge.html
Comment 17 EWS Watchlist 2019-07-18 09:23:13 PDT
Created attachment 374390 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 18 Carlos Garcia Campos 2019-07-18 09:32:56 PDT
(In reply to Build Bot from comment #16)
> Comment on attachment 374383 [details]
> Patch
> 
> Attachment 374383 [details] did not pass mac-debug-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12766458
> 
> New failing tests:
> storage/indexeddb/dont-wedge.html

This is unrelated for sure.
Comment 19 Jonathan Bedard 2019-07-18 10:17:06 PDT
Comment on attachment 374383 [details]
Patch

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

Python changes mostly looks good, I have one small question about six.

> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:123
> +        pywebsocket_deps = [self._filesystem.join(wpt_tools_base, "third_party", "six")]

We have our own version of six in Tools/Scripts/webkitpy/thirdparty/autoinstalled. I don't think that we have ambiguity problems because it's always imported through 'import autoinstalled.six',  but this still seems potentially dangerous...especially if the wpt version of six is different than the one we have have previously loaded. Do we actually need this? We probably do, it just feels super weird to have six both autoinstalled and checked into LayoutTests/imported/w3c.
Comment 20 Carlos Garcia Campos 2019-07-18 10:23:45 PDT
I think we have six in autoinstalled, because it's a dependency of another module that is autoinstalled (maybe pytest). wpt also includes six because it's a depedency of other wpt tools. So, in this case I'm using the six from wpt tools because I'm using another wpt tool. wpt tools include all tools in the sys path (using the localpaths module) and webkitpy uses the wk autoinstall module, so I don't think there's conflict.
Comment 21 Jonathan Bedard 2019-07-18 13:59:45 PDT
Comment on attachment 374383 [details]
Patch

storage/indexeddb/dont-wedge.html is a flakey test at the moment, so I'd say this is good to land (See <https://bugs.webkit.org/show_bug.cgi?id=199883>)
Comment 22 WebKit Commit Bot 2019-07-19 01:35:04 PDT
Comment on attachment 374383 [details]
Patch

Clearing flags on attachment: 374383

Committed r247640: <https://trac.webkit.org/changeset/247640>
Comment 23 WebKit Commit Bot 2019-07-19 01:35:06 PDT
All reviewed patches have been landed.  Closing bug.