Bug 55811 - Fix webkit-patch on Win32 python
Summary: Fix webkit-patch on Win32 python
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: Patrick R. Gansterer
URL:
Keywords:
Depends on: 53822 61248 48614 49474 55925 55927 61250
Blocks: 48517
  Show dependency treegraph
 
Reported: 2011-03-04 18:09 PST by Patrick R. Gansterer
Modified: 2011-06-18 13:03 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.94 KB, patch)
2011-03-04 18:15 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (1.34 KB, patch)
2011-04-02 08:56 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2011-03-04 18:09:05 PST
Fix webkit-patch on Win32 python
Comment 1 Patrick R. Gansterer 2011-03-04 18:15:18 PST
Created attachment 84838 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-03-07 19:06:55 PST
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.
Comment 3 Patrick R. Gansterer 2011-03-07 21:53:46 PST
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".
Comment 4 Adam Barth 2011-03-07 21:58:14 PST
Maybe it's not worth making webkit-patch work on Win32 Python?  These changes seem bad.
Comment 5 Dirk Pranke 2011-03-07 22:37:37 PST
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 ...
Comment 6 Patrick R. Gansterer 2011-04-02 08:56:15 PDT
Created attachment 87976 [details]
Patch
Comment 7 Eric Seidel (no email) 2011-04-03 04:51:59 PDT
Comment on attachment 87976 [details]
Patch

OK.  Assuming it works. :)
Comment 8 WebKit Review Bot 2011-06-18 13:03:36 PDT
Comment on attachment 87976 [details]
Patch

Clearing flags on attachment: 87976

Committed r89201: <http://trac.webkit.org/changeset/89201>
Comment 9 WebKit Review Bot 2011-06-18 13:03:41 PDT
All reviewed patches have been landed.  Closing bug.