Bug 55811 – Fix webkit-patch on Win32 python

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Patrick R. Gansterer]

Fix webkit-patch on Win32 python

[Patrick R. Gansterer]

Created attachment 84838 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 84838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84838&action=review

> Tools/Scripts/webkitpy/common/config/ports.py:43
> +        # Win32 perl needs a slash here to determinate the correct module directories.
> +        return "Tools/Scripts/" + script_name

Are you sure this is really what we want?  I would suspect this is used in other places which might want a \ instead on windows.

> Tools/Scripts/webkitpy/common/config/ports.py:92
> +        command = [cls.perl_executable(), cls.script_path("build-webkit")]

This is kinda sad.  To encode the perl vs. python in the caller.  I take it there is no way to run these under a shell in naked win32?

> Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:64
> +        args = ["perl", self._tool.port().script_path("prepare-ChangeLog")]

Why not perl_executable() here?  Don't we expose both types of port objects on tool (I know, it's sad that we have 2 types!)

> Tools/Scripts/webkitpy/tool/steps/preparechangelogfordepsroll.py:37
> +        self._run_script("perl", "prepare-ChangeLog")

Seems prepare-changeLog should just be one of the commands exposed on the Port object.

[Patrick R. Gansterer]

Comment on attachment 84838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84838&action=review

>> Tools/Scripts/webkitpy/common/config/ports.py:43
>> +        return "Tools/Scripts/" + script_name
> 
> Are you sure this is really what we want?  I would suspect this is used in other places which might want a \ instead on windows.

I don't like this too, but if i keep the backslash Win32 perl fail, because it can't find VCSUtils module. I'll try if passing the include directory via command line works.

>> Tools/Scripts/webkitpy/common/config/ports.py:92
>> +        command = [cls.perl_executable(), cls.script_path("build-webkit")]
> 
> This is kinda sad.  To encode the perl vs. python in the caller.  I take it there is no way to run these under a shell in naked win32?

Nope :-(, you find the same change in the previous "Win32 fixes".

[Adam Barth]

Maybe it's not worth making webkit-patch work on Win32 Python?  These changes seem bad.

[Dirk Pranke]

Comment on attachment 84838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84838&action=review

>>> Tools/Scripts/webkitpy/common/config/ports.py:43
>>> +        return "Tools/Scripts/" + script_name
>> 
>> Are you sure this is really what we want?  I would suspect this is used in other places which might want a \ instead on windows.
> 
> I don't like this too, but if i keep the backslash Win32 perl fail, because it can't find VCSUtils module. I'll try if passing the include directory via command line works.

At the very least this should probably be protected by an if platform == 'win32' sort of thing. Although, ironically, the code will work on all the non-win32 platforms as is.

>>> Tools/Scripts/webkitpy/common/config/ports.py:92
>>> +        command = [cls.perl_executable(), cls.script_path("build-webkit")]
>> 
>> This is kinda sad.  To encode the perl vs. python in the caller.  I take it there is no way to run these under a shell in naked win32?
> 
> Nope :-(, you find the same change in the previous "Win32 fixes".

I suggest that we revamp this in a way such that it could be overridden in a port-specific manner. Maybe you need a routine that just takes the basename of the script and returns a list of arguments. The default implementation could return list(script_path(X)) and the win32 version could return list(lookup_interpreter_for(X), script_path(X)). Or, it could even figure out the interpreter by opening the file and reading the first line ...

[Patrick R. Gansterer]

Created attachment 87976 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 87976 [details]
Patch

OK.  Assuming it works. :)

[WebKit Review Bot]

Comment on attachment 87976 [details]
Patch

Clearing flags on attachment: 87976

Committed r89201: <http://trac.webkit.org/changeset/89201>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.