Bug 38034 – WebSocket: pywebsocket 0.5

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fumitoshi Ukai]

pywebsocket 0.5 adds implementation of new WebSocket protocol http://www.whatwg.org/specs/web-socket-protocol/ draft-hixie-thewebsocketprotocol-76.

It is required to implement new protocol in WebCore/websockets

[Fumitoshi Ukai]

Created attachment 54134 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 54134 [details]
Patch

Should we just autoinstall this code instead of keeping a local copy?

[Fumitoshi Ukai]

(In reply to comment #2)
> (From update of attachment 54134 [details])
> Should we just autoinstall this code instead of keeping a local copy?

Ah, we should.
I'll update the patch.

[Fumitoshi Ukai]

Created attachment 54396 [details]
Patch

[David Levin]

Comment on attachment 54396 [details]
Patch

Looking good. Just a few things to address.

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 167c788bb4972264e4e014b5f76110728c73765b..5fc56a4717d538c358d0bac2768b4c6655bdb91a 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,25 @@
> [phone]  Fumitoshi Ukai  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        WebSocket: pywebsocket 0.5
> +        https://bugs.webkit.org/show_bug.cgi?id=38034
> +
> +        Remove pywebsocket from webkitpy/thirdparty.
> +        Make pywebsocket autoinstalled.
> +
> +        * Scripts/new-run-webkit-websocketserver:
> +          Add --output-dir option.
> +        * Scripts/old-run-webkit-tests:
> +          Use new-run-webkit-websocketserver, rather than directly run pywebsocket's standalone.py.
> +        * Scripts/run-webkit-websocketserver:
> +          Use new-run-webkit-websocketserver, rather than directly run pywebsocket's standalone.py.

"Ditto." works instead of repeating the exact same comment.


btw, can we just get rid of "run-webkit-websocketserver" and rename "new-run-webkit-websocketserver" to "run-webkit-websocketserver"? *Not in this patch* but a follow up patch.


> diff --git a/WebKitTools/Scripts/old-run-webkit-tests b/WebKitTools/Scripts/old-run-webkit-tests

The following lines are repeated in "closeWebSocketServer" It feels like it should be a common function that both of these places call.

>      my $webSocketServerPath = "/usr/bin/python";
> +    my $pid = open3(\*WEBSOCKETSERVER_IN, \*WEBSOCKETSERVER_OUT, \*WEBSOCKETSERVER_ERR, $webSocketServerPath, @args);
> +    waitpid $pid, 0;
> +    close WEBSOCKETSERVER_IN;
> +    close WEBSOCKETSERVER_OUT;
> +    close WEBSOCKETSERVER_ERR;


> diff --git a/WebKitTools/Scripts/run-webkit-websocketserver b/WebKitTools/Scripts/run-webkit-websocketserver

This has the same problem of repeating a bunch of lines related to running webSocketServer

[Fumitoshi Ukai]

Created attachment 55206 [details]
Patch

[Fumitoshi Ukai]

Thanks for review!

(In reply to comment #5)

> "Ditto." works instead of repeating the exact same comment.

Ok. Fixed.
  
> btw, can we just get rid of "run-webkit-websocketserver" and rename
> "new-run-webkit-websocketserver" to "run-webkit-websocketserver"? *Not in this
> patch* but a follow up patch.

Hmm, usage is a bit different (run-webkit-websocketserver runs until ENTER is hit, new-run-webkit-websocketserver controls websocket server by --server=start and --server=stop), but I think we could get rid of "run-webkit-websocketsever" and make "new-run-webkit-websocketserver" to be "run-webkit-websocketserver".

 
> 
> > diff --git a/WebKitTools/Scripts/old-run-webkit-tests b/WebKitTools/Scripts/old-run-webkit-tests
> 
> The following lines are repeated in "closeWebSocketServer" It feels like it
> should be a common function that both of these places call.

I think we can simply use system here, instead of open3.

> > diff --git a/WebKitTools/Scripts/run-webkit-websocketserver b/WebKitTools/Scripts/run-webkit-websocketserver
> 
> This has the same problem of repeating a bunch of lines related to running
> webSocketServer

Ditto.

[Fumitoshi Ukai]

Committed r58933: <http://trac.webkit.org/changeset/58933>

[Eric Seidel (no email)]

This broke all websocket tests on Tiger.  You can't tell because Tiger was already broken earlier this evening.  Please roll this back unless you know the fix.

[Eric Seidel (no email)]

We can land this again once the Tiger bot is updated to python 2.5+

[Eric Seidel (no email)]

Attachment 55206 [details] was posted by a committer and has review+, assigning to Fumitoshi Ukai for commit.

[Alexey Proskuryakov]

What exactly is the Python 2.5 dependency here? Can this script be made to work on Tiger without toolchain changes?

[Eric Seidel (no email)]

This change made the websocket code depend on webkitpy which is 2.5+.

There are numerous (very bad) threading and subprocess bugs in python 2.4 which were fixed in 2.5.  (There are a bunch more in 2.5 as well, but not much we can do about that ATM since we don't want to make Leopard users install anything.)

It's easiest to maintain the code if it's all the same version python.  There is nothing in the websockets code which can't be made to run in 2.3 (to my knowledge).  However that is not true of webkitpy in general and supporting multi-versioned python is not a road we've historically been very interested in walking down. :)  (And I would still strongly recommend against.)  We'd have to segregate our python code into versioned directories and be careful not to include between them.

[Eric Seidel (no email)]

Python versioning is not the best I've ever seen.  If you don't just pick a minimum version for all your code, you end up with a never ending trail of tears.  After discussion on webkit-dev a few months back, we picked 2.5 (because that matched Leopard and was new enough to not have all the 2.4 subprocess bugs).

We didn't have nearly as much trouble with perl versioning in webkit probably because perl has been around (and stable) for forever.

[Chris Jerdonek]

(In reply to comment #14)
> We didn't have nearly as much trouble with perl versioning in webkit probably because perl has been around (and stable) for forever.

FYI, I have run into some Perl versioning issues in the past, e.g.:

https://bugs.webkit.org/show_bug.cgi?id=33415#c6
https://bugs.webkit.org/show_bug.cgi?id=33415#c9

If we were trying to use Perl for everything that we're currently doing in Python, I'm guessing we would probably run into these types of issues more.

[Fumitoshi Ukai]

Committed r59350: <http://trac.webkit.org/changeset/59350>

[WebKit Review Bot]

http://trac.webkit.org/changeset/59350 might have broken Tiger Intel Release

[Fumitoshi Ukai]

(In reply to comment #17)
> http://trac.webkit.org/changeset/59350 might have broken Tiger Intel Release

Seems tiger still use old python?
http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/11971/steps/layout-test/logs/stdio

websocket/tests .Traceback (most recent call last):
  File "WebKitTools/Scripts/new-run-webkit-websocketserver", line 38, in ?
    import webkitpy.layout_tests.port.websocket_server as websocket_server
  File "/Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py", line 241
    with codecs.open(self._pidfile, "w", "ascii") as file:
              ^
SyntaxError: invalid syntax

it looks strange, websocket_server.py has "from __future__ import with_statement"...
Anyway, I'll try to fix this by removing with statement from websocket_server.py.

[Adam Roben (:aroben)]

This broke Apple's Windows port. See <http://build.webkit.org/builders/Windows%20Debug%20%28Tests%29/builds/13532/steps/layout-test/logs/stdio>.

I guess we were never calling new-run-webkit-websocketserver on Windows before. That script doesn't seem able to handle Apple's Windows port.

[Adam Roben (:aroben)]

As pointed out by Eric, websocket_server.py assumes that all Windows platforms are Chromium:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py#L193

[Fumitoshi Ukai]

(In reply to comment #20)
> As pointed out by Eric, websocket_server.py assumes that all Windows platforms are Chromium:
> 
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py#L193

I think https://bugs.webkit.org/show_bug.cgi?id=37664 will fix this issue.

[Adam Roben (:aroben)]

(In reply to comment #21)
> (In reply to comment #20)
> > As pointed out by Eric, websocket_server.py assumes that all Windows platforms are Chromium:
> > 
> > http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py#L193
> 
> I think bug 37664 will fix this issue.

Is there some more expedient approach we can take? Bug 37664 has been around for over a month now.

[Eric Seidel (no email)]

I don' think this patch shoudl have landed if it was known to break windows.  I think it should be rolled out until the Win issues can be fixed.

[Eric Seidel (no email)]

Bug 37664 is fixed now.  Is Windows fixed?

[Eric Seidel (no email)]

Looks like it.  However the bots are in a bad state. :(  Will see if I can fix some of the other test failures on the win bots.