Bug 55927

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.