Bug 38034 - WebSocket: pywebsocket 0.5
Summary: WebSocket: pywebsocket 0.5
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Fumitoshi Ukai
URL:
Keywords:
Depends on: 38717
Blocks: 35572
  Show dependency treegraph
 
Reported: 2010-04-22 23:15 PDT by Fumitoshi Ukai
Modified: 2010-05-17 15:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (114.73 KB, patch)
2010-04-22 23:26 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (166.04 KB, patch)
2010-04-27 01:33 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (165.48 KB, patch)
2010-05-05 23:11 PDT, Fumitoshi Ukai
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2010-04-22 23:15:24 PDT
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
Comment 1 Fumitoshi Ukai 2010-04-22 23:26:53 PDT
Created attachment 54134 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-04-26 15:53:48 PDT
Comment on attachment 54134 [details]
Patch

Should we just autoinstall this code instead of keeping a local copy?
Comment 3 Fumitoshi Ukai 2010-04-26 17:45:47 PDT
(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.
Comment 4 Fumitoshi Ukai 2010-04-27 01:33:57 PDT
Created attachment 54396 [details]
Patch
Comment 5 David Levin 2010-04-28 14:16:29 PDT
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 @@
> +2010-04-27  Fumitoshi Ukai  <ukai@chromium.org>
> +
> +        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
Comment 6 Fumitoshi Ukai 2010-05-05 23:11:22 PDT
Created attachment 55206 [details]
Patch
Comment 7 Fumitoshi Ukai 2010-05-05 23:26:50 PDT
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.
Comment 8 Fumitoshi Ukai 2010-05-06 20:56:24 PDT
Committed r58933: <http://trac.webkit.org/changeset/58933>
Comment 9 Eric Seidel (no email) 2010-05-06 21:23:03 PDT
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.
Comment 10 Eric Seidel (no email) 2010-05-06 21:54:05 PDT
We can land this again once the Tiger bot is updated to python 2.5+
Comment 11 Eric Seidel (no email) 2010-05-08 23:18:30 PDT
Attachment 55206 [details] was posted by a committer and has review+, assigning to Fumitoshi Ukai for commit.
Comment 12 Alexey Proskuryakov 2010-05-10 13:12:09 PDT
What exactly is the Python 2.5 dependency here? Can this script be made to work on Tiger without toolchain changes?
Comment 13 Eric Seidel (no email) 2010-05-10 13:16:25 PDT
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.
Comment 14 Eric Seidel (no email) 2010-05-10 13:19:19 PDT
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.
Comment 15 Chris Jerdonek 2010-05-10 13:32:03 PDT
(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.
Comment 16 Fumitoshi Ukai 2010-05-13 00:29:00 PDT
Committed r59350: <http://trac.webkit.org/changeset/59350>
Comment 17 WebKit Review Bot 2010-05-13 01:11:00 PDT
http://trac.webkit.org/changeset/59350 might have broken Tiger Intel Release
Comment 18 Fumitoshi Ukai 2010-05-13 01:23:15 PDT
(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.
Comment 19 Adam Roben (:aroben) 2010-05-13 07:56:37 PDT
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.
Comment 20 Adam Roben (:aroben) 2010-05-13 08:09:12 PDT
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
Comment 21 Fumitoshi Ukai 2010-05-13 19:47:24 PDT
(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.
Comment 22 Adam Roben (:aroben) 2010-05-14 05:41:08 PDT
(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.
Comment 23 Eric Seidel (no email) 2010-05-14 14:13:29 PDT
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.
Comment 24 Eric Seidel (no email) 2010-05-17 15:17:09 PDT
Bug 37664 is fixed now.  Is Windows fixed?
Comment 25 Eric Seidel (no email) 2010-05-17 15:22:14 PDT
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.