Bug 34879 – pywebsocket should support html and cgi in the same directory.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

Currently, we set  "-x", "/websocket/tests/cookies" for pywebsocket invocation to specify /websocket/tests/cookies is cgi directory, so contents will be served as CGI instead of static file.

Ideally, we should be able to set his to "/websocket/tests". That way, no one will get surprised by trying to add a .pl test to another subdirectory. Of course, pywebsocket would need to learn how to distinguish .html and .pl files.

[Fumitoshi Ukai]

http://code.google.com/p/pywebsocket/issues/detail?id=43 in upstream issue tracker.

[Fumitoshi Ukai]

Created attachment 48879 [details]
Patch

[Alexey Proskuryakov]

+        specify --server_host 127.0.0.1 -x /websocket/tests

What is --server-host, and why is it good to specify it? I think I know the answer, but please explain it in ChangeLog.

Seems untidy to have a mix of short and long option names.

>             if '..' in self.path:
>                 return False

Do functions that we use for path manipulation perform tilde expansion (for home directory)? Writing a truly secure server is out of scope for the test tool, it's just too similar to existing check for "..", so I wanted to ask.

+            scriptfile = self.translate_path(self.path.split('?', 2)[0])

I don't understand what this line does. Someone who is better familiar with Python might understand it, and give their r+.

[Fumitoshi Ukai]

(In reply to comment #3)
> +        specify --server_host 127.0.0.1 -x /websocket/tests
> 
> What is --server-host, and why is it good to specify it? I think I know the
> answer, but please explain it in ChangeLog.

Ok.

> Seems untidy to have a mix of short and long option names.

Changed to use long option names.

> >             if '..' in self.path:
> >                 return False
> 
> Do functions that we use for path manipulation perform tilde expansion (for
> home directory)? Writing a truly secure server is out of scope for the test
> tool, it's just too similar to existing check for "..", so I wanted to ask.

I believe it doesn't perform tilde expansion.

> +            scriptfile = self.translate_path(self.path.split('?', 2)[0])
> 
> I don't understand what this line does. Someone who is better familiar with
> Python might understand it, and give their r+.

[Fumitoshi Ukai]

Created attachment 48963 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 48963 [details]
Patch

> +        "--server_host", "127.0.0.1",
> +        "--port", "$webSocketPort",
> +        "--document_root", "$webSocketHandlerDir",
> +        "--scan_dir", "$webSocketHandlerScanDir",
> +        "--websock_handlers_map_file", "$webSocketHandlerMapFile",
> +        "--cgi_paths", "/websocket/tests",
> +        "--log_file", "$logFile",

I think that the usual Unix style for long options is to use dashes, not underscores.

[Fumitoshi Ukai]

(In reply to comment #6)
> (From update of attachment 48963 [details])
> > +        "--server_host", "127.0.0.1",
> > +        "--port", "$webSocketPort",
> > +        "--document_root", "$webSocketHandlerDir",
> > +        "--scan_dir", "$webSocketHandlerScanDir",
> > +        "--websock_handlers_map_file", "$webSocketHandlerMapFile",
> > +        "--cgi_paths", "/websocket/tests",
> > +        "--log_file", "$logFile",
> 
> I think that the usual Unix style for long options is to use dashes, not
> underscores.

We've used userscores...
Since this options is in pywebsocket, and no WebKitTools/Scripts uses underscores, 
should I fix it to use dashes?

[Adam Barth]

Comment on attachment 48963 [details]
Patch

Underscores are a strange Google convention.  WebKit-land uses dashes.  Also, you patch does not appear to apply cleanly to top of tree.

[Fumitoshi Ukai]

(In reply to comment #8)
> (From update of attachment 48963 [details])
> Underscores are a strange Google convention.  WebKit-land uses dashes. 

I think pywebsocket is third party code, so don't need to flow WebKit-style.

> Also,
> you patch does not appear to apply cleanly to top of tree.

Hmm, this is webkit-patch (with git) bug, when moving files and directory becomes empty?

webkit-patch apply-attachment says
$ ./WebKitTools/Scripts/webkit-patch apply-attachment 48963 [details]
Cleaning working directory
Updating working directory
Reading Keychain for bugs.webkit.org account and password.  Click "Allow" to continue...
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=48963&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=34879&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 48963 from bug 34879.
Failed to run "['/Users/ukai/src/chromium-webkit2/src/third_party/WebKit/WebKitTools/Scripts/svn-apply']" exit_code: 2
Last 500 characters of output:
h fuzz 3.
patching file WebKitTools/Scripts/run-webkit-tests
patching file WebKitTools/Scripts/run-webkit-websocketserver
patching file WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py
patching file WebKitTools/pywebsocket/mod_pywebsocket/standalone.py
patching file WebKitTools/pywebsocket/setup.py
Could not open 'LayoutTests/websocket/tests/cookies' to list files: 0 at /Users/ukai/src/chromium-webkit2/src/third_party/WebKit/WebKitTools/Scripts/svn-apply line 314, <> line 417.

Failed to run "['/Users/ukai/src/chromium-webkit2/src/third_party/WebKit/WebKitTools/Scripts/svn-apply']" exit_code: 2
ukai-macpro:WebKit ukai$ git reset --hard

[Fumitoshi Ukai]

Created attachment 48992 [details]
Patch

[Fumitoshi Ukai]

Created attachment 49188 [details]
Patch

[Chris Jerdonek]

FYI, we now have a README.webkit file in pywebsocket's directory that can be updated whenever local changes are made to pywebsocket:

https://bugs.webkit.org/show_bug.cgi?id=35198

[Fumitoshi Ukai]

Created attachment 49714 [details]
Patch

[Fumitoshi Ukai]

Created attachment 49885 [details]
Patch

[Alexey Proskuryakov]

I think I could review this patch if you explained what this line did:

+            scriptfile = self.translate_path(self.path.split('?', 2)[0])

It seems worth both a short comment, and a longer explanation in the bug.

[Fumitoshi Ukai]

Created attachment 49969 [details]
Patch

[Fumitoshi Ukai]

(In reply to comment #15)
> I think I could review this patch if you explained what this line did:
> 
> +            scriptfile = self.translate_path(self.path.split('?', 2)[0])

it gets real pathname from request path.
 self.path contains request path in HTTP request header
 self.path.split('?', 2)[0] strips query parameter if any
 self.translate_path() converts resource name into pathname on filesystem.
 
> It seems worth both a short comment, and a longer explanation in the bug.

added comments

[Eric Seidel (no email)]

I'm not sure what the svn-apply bug which this is tripping over is:
Could not open 'LayoutTests/websocket/tests/cookies' to list files: 0 at /mnt/git/webkit-chromium-ews/WebKitTools/Scripts/svn-apply line 314, <> line 715.

[Fumitoshi Ukai]

(In reply to comment #18)
> I'm not sure what the svn-apply bug which this is tripping over is:
> Could not open 'LayoutTests/websocket/tests/cookies' to list files: 0 at
> /mnt/git/webkit-chromium-ews/WebKitTools/Scripts/svn-apply line 314, <> line
> 715.

Hmm, this is because the patch is trying to remove the directory?

[Eric Seidel (no email)]

Sounds like bug 34871.  Bug 14590 could also possibly be related.

[Alexey Proskuryakov]

Comment on attachment 49969 [details]
Patch

r=me. Please land this with svn - as far as I know, git wrappers won't preserve history of file copies correctly.

[Fumitoshi Ukai]

Committed r55719: <http://trac.webkit.org/changeset/55719>