Bug 38034

Created attachment 54134 [details] Patch

Comment on attachment 54134 [details] Patch Should we just autoinstall this code instead of keeping a local copy?

(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.

Created attachment 54396 [details] Patch

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

Created attachment 55206 [details] Patch

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.

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

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.

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

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

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

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.

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.

(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.

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

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

(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.

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.

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

(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.

(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.

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.

Bug 37664 is fixed now. Is Windows fixed?

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.