Summary: | Emulate shebang on Win32 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick R. Gansterer <paroga> | ||||||||||||
Component: | Tools / Tests | Assignee: | Patrick R. Gansterer <paroga> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, dpranke, eric | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | 55925 | ||||||||||||||
Bug Blocks: | 55811 | ||||||||||||||
Attachments: |
|
Description
Patrick R. Gansterer
2011-03-08 00:33:29 PST
Created attachment 85031 [details]
Patch
Created attachment 85032 [details]
Patch
Comment on attachment 85032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85032&action=review > Tools/Scripts/webkitpy/common/config/ports.py:56 > + if first_line.find("python") > -1: > + return [sys.executable, script_file] > + if first_line.find("perl") > -1: > + return ["perl", script_file] Should we generate some kind of error if we can't figure out the right interpreter? (In reply to comment #3) > (From update of attachment 85032 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85032&action=review > > > Tools/Scripts/webkitpy/common/config/ports.py:56 > > + if first_line.find("python") > -1: > > + return [sys.executable, script_file] > > + if first_line.find("perl") > -1: > > + return ["perl", script_file] > > Should we generate some kind of error if we can't figure out the right interpreter? Might be an option. At the moment we can pass a file with a file extention and no shebang to this function and windows can detect the correct interpreter via file extension. So it's not wrong all the time. Comment on attachment 85032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85032&action=review > Tools/Scripts/webkitpy/common/config/ports.py:55 > + first_line = open(script_file, "r").readline() > + if first_line.startswith("#!"): > + if first_line.find("python") > -1: > + return [sys.executable, script_file] > + if first_line.find("perl") > -1: I would have split this off into a separate function (which would then be unit-testable). Something like interpreter_for_script, or something. In general this looks fine. I'm not sure how much this slows down execution. I'm ready to r+ this with a unit test. (See filesystem.py and filesystem_mock.py for how we do open and test open) Searching the first line using find() seems perhaps a bit dangerous to me. I'd be happier if we did some exact string matches instead. That way things like shebangs that reference python2.5 instead of python will fail in a more reliable manner. Dunno how many different shebangs we have though, but it wouldn't be a bad thing to standardize on a couple ... Also, at some point you might want to handle ruby as well (thank you, pretty patch). Created attachment 87602 [details]
Patch
command_for_file() and and read_text_file_line() are too specialized to belong in the filesystem class. I would just use read_text_file().splitlines()[0] and a local function in config/ports.py. In the unlikely case that this turns out to be a performance problem, we can add something like open_binary_file_for_reading() that returns an Iterable that would be more generically useful. Created attachment 87621 [details]
Patch
Looks good, and is simpler than the last patch to boot. Thanks! (note that I am not actualy a reviewer so can't r+ it). Comment on attachment 87621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87621&action=review This patch is great! > Tools/Scripts/webkitpy/common/config/ports.py:47 > + def interpreter_for_script(cls, script_path): Maybe we should put this in executive.py? It's a win32 feature, not releated to any specific webkit port. > Tools/Scripts/webkitpy/common/config/ports.py:66 > + # Win32 does not suppot shebang. We need to detect the interpreter ourself. support Comment on attachment 87621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87621&action=review >> Tools/Scripts/webkitpy/common/config/ports.py:47 >> + def interpreter_for_script(cls, script_path): > > Maybe we should put this in executive.py? It's a win32 feature, not releated to any specific webkit port. You should pass in a FileSystem for mocking. Then your test doesn't need to even write out to disk if you use a MockFileSystem. Please be sure to make that change before landing. Created attachment 87972 [details]
Patch
Comment on attachment 87972 [details] Patch Clearing flags on attachment: 87972 Committed r82770: <http://trac.webkit.org/changeset/82770> All reviewed patches have been landed. Closing bug. |