Bug 34879 - pywebsocket should support html and cgi in the same directory.
Summary: pywebsocket should support html and cgi in the same directory.
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: Nobody
URL:
Keywords:
Depends on: 34871
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-12 00:06 PST by Fumitoshi Ukai
Modified: 2010-03-09 02:41 PST (History)
4 users (show)

See Also:


Attachments
Patch (14.16 KB, patch)
2010-02-17 02:30 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (15.74 KB, patch)
2010-02-17 20:07 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (17.18 KB, patch)
2010-02-18 03:03 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (23.43 KB, patch)
2010-02-21 23:54 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (24.55 KB, patch)
2010-03-01 02:13 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (30.47 KB, patch)
2010-03-03 00:52 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (30.68 KB, patch)
2010-03-03 18:08 PST, Fumitoshi Ukai
ap: 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-02-12 00:06:54 PST
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.
Comment 1 Fumitoshi Ukai 2010-02-12 00:10:32 PST
http://code.google.com/p/pywebsocket/issues/detail?id=43 in upstream issue tracker.
Comment 2 Fumitoshi Ukai 2010-02-17 02:30:34 PST
Created attachment 48879 [details]
Patch
Comment 3 Alexey Proskuryakov 2010-02-17 10:01:46 PST
+        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+.
Comment 4 Fumitoshi Ukai 2010-02-17 19:21:06 PST
(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+.
Comment 5 Fumitoshi Ukai 2010-02-17 20:07:17 PST
Created attachment 48963 [details]
Patch
Comment 6 Alexey Proskuryakov 2010-02-17 21:37:58 PST
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.
Comment 7 Fumitoshi Ukai 2010-02-17 21:57:22 PST
(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?
Comment 8 Adam Barth 2010-02-18 00:16:51 PST
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.
Comment 9 Fumitoshi Ukai 2010-02-18 03:02:36 PST
(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 ukai@chromium.org...
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
Comment 10 Fumitoshi Ukai 2010-02-18 03:03:19 PST
Created attachment 48992 [details]
Patch
Comment 11 Fumitoshi Ukai 2010-02-21 23:54:46 PST
Created attachment 49188 [details]
Patch
Comment 12 Chris Jerdonek 2010-02-28 00:39:48 PST
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
Comment 13 Fumitoshi Ukai 2010-03-01 02:13:40 PST
Created attachment 49714 [details]
Patch
Comment 14 Fumitoshi Ukai 2010-03-03 00:52:50 PST
Created attachment 49885 [details]
Patch
Comment 15 Alexey Proskuryakov 2010-03-03 15:02:03 PST
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.
Comment 16 Fumitoshi Ukai 2010-03-03 18:08:40 PST
Created attachment 49969 [details]
Patch
Comment 17 Fumitoshi Ukai 2010-03-03 18:10:40 PST
(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
Comment 18 Eric Seidel (no email) 2010-03-05 14:28:05 PST
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.
Comment 19 Fumitoshi Ukai 2010-03-08 17:36:23 PST
(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?
Comment 20 Eric Seidel (no email) 2010-03-08 22:22:08 PST
Sounds like bug 34871.  Bug 14590 could also possibly be related.
Comment 21 Alexey Proskuryakov 2010-03-08 23:35:11 PST
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.
Comment 22 Fumitoshi Ukai 2010-03-09 02:41:18 PST
Committed r55719: <http://trac.webkit.org/changeset/55719>