Bug 55927 – Emulate shebang on Win32

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 85031 [details]
Patch

[Patrick R. Gansterer]

Created attachment 85032 [details]
Patch

[Adam Barth]

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?

[Patrick R. Gansterer]

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

[Eric Seidel (no email)]

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)

[Dirk Pranke]

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

[Dirk Pranke]

Also, at some point you might want to handle ruby as well (thank you, pretty patch).

[Patrick R. Gansterer]

Created attachment 87602 [details]
Patch

[Dirk Pranke]

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.

[Patrick R. Gansterer]

Created attachment 87621 [details]
Patch

[Dirk Pranke]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Patrick R. Gansterer]

Created attachment 87972 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 87972 [details]
Patch

Clearing flags on attachment: 87972

Committed r82770: <http://trac.webkit.org/changeset/82770>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.