| Summary: | webkitpy: WebSocket server doesn't support Python 3 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||||
| Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||||
| Status: | RESOLVED DUPLICATE | ||||||||||||||
| Severity: | Normal | CC: | aakash_jain, ap, cgarcia, commit-queue, ews-watchlist, glenn, gsnedders, Hironori.Fujii, slewis, webkit-bug-importer, youennf | ||||||||||||
| Priority: | P2 | Keywords: | DoNotImportToRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=230513 https://bugs.webkit.org/show_bug.cgi?id=236375 https://bugs.webkit.org/show_bug.cgi?id=230319 |
||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Jonathan Bedard
2020-01-21 11:44:45 PST
Created attachment 388331 [details]
Patch
Comment on attachment 388331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388331&action=review > Tools/ChangeLog:8 > + websocket_server.py doesn't support Python 3, and needs to be run with Is there a new version of the server that supports Python 3? Perhaps Chromium implemented the changes already? (In reply to Alexey Proskuryakov from comment #2) > Comment on attachment 388331 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388331&action=review > > > Tools/ChangeLog:8 > > + websocket_server.py doesn't support Python 3, and needs to be run with > > Is there a new version of the server that supports Python 3? Perhaps > Chromium implemented the changes already? There is not, https://github.com/web-platform-tests/wpt/issues/8820 tracks this on their side. We need to update our import anyways, and then I can see what it takes to get this specific script Python 3. Two things that make it look like something is off here: 1. We have our own server, our tests were failing. So that needs a fix or a workaround too. 2. But only one of WPT WebSocket tests was failing. Is it actually broken? (In reply to Alexey Proskuryakov from comment #4) > Two things that make it look like something is off here: > > 1. We have our own server, our tests were failing. So that needs a fix or a > workaround too. > > 2. But only one of WPT WebSocket tests was failing. Is it actually broken? We did have an 'old' server, but it wasn't our own, it was just an old version of a third party script. Carlos replaced the old one with the w3c one a few months back, https://trac.webkit.org/changeset/247640/webkit. Now we have a single WebSocket server, the w3c one. Comment on attachment 388331 [details]
Patch
Seems fine as an interim measure. May be worth considering python2 when it exists though, as I’m not sure what this patch will do on systems where “python” is python3.
Still, would prefer to understand why wpt tests aren’t failing. Created attachment 388453 [details]
Patch
(In reply to Alexey Proskuryakov from comment #6) > Comment on attachment 388331 [details] > Patch > > Seems fine as an interim measure. May be worth considering python2 when it > exists though, as I’m not sure what this patch will do on systems where > “python” is python3. This is a good point. I changed the code to reference python2 instead of python, and it will use the current interpreter if that interpreter is Python 2. If a script is running on a system in Python 3 that doesn't have python2, this will fail, but that seems acceptable, or at least, no worse than the current situation. (In reply to Alexey Proskuryakov from comment #7) > Still, would prefer to understand why wpt tests aren’t failing. When I run those tests independently, they don't even start the WebSocket server. So it's not surprising they don't fail when the WebSocket server fails to start. What I don't understand is what those tests are actually supposed to be testing. But I'm not sure that question is within the scope of this patch. Comment on attachment 388453 [details] Patch Clearing flags on attachment: 388453 Committed r254944: <https://trac.webkit.org/changeset/254944> All reviewed patches have been landed. Closing bug. > When I run those tests independently, they don't even start the WebSocket server. So it's not surprising they don't fail when the WebSocket server fails to start.
This doesn't necessarily tell us much. The server is started anyway when running all tests, so this problem may be masked most of the time.
Looks like https://github.com/web-platform-tests/wpt/issues/8820 was resolved, so there now is a new version that supports Python 3. Reopening to attach new patch. Created attachment 451740 [details]
Patch
Created attachment 451751 [details]
Patch
Comment on attachment 451751 [details]
Patch
rs=me, as long as the license is unchanged.
(In reply to Alexey Proskuryakov from comment #19) > Comment on attachment 451751 [details] > Patch > > rs=me, as long as the license is unchanged. Double checked that the license in https://github.com/web-platform-tests/wpt/blob/master/LICENSE.md matches what we have in LayoutTests/imported/w3c/web-platform-tests/LICENSE.md Looks like I need to look at the tests we are importing as well, will do so next week. Will not be landing until EWS is green. Note that bug 230319 already previously started to try and update pywebsocket; one of these should be dupe'd to the other. Bug 227255 actually already imported a recent enough version of the WPT tools for our purposes, but this didn't get noticed because it didn't remove the (gone upstream) web-platform-tests/tools/pywebsocket/. It may well be worthwhile just focusing on removing that, and moving to web-platform-tests/tools/third_party/pywebsocket3/, rather than doing this combined with a reimport. Created attachment 451914 [details]
Patch
I'm going to move work over to bug 230319, since that already has more work done that I've done here. *** This bug has been marked as a duplicate of bug 230319 *** |