Bug 55811

Created attachment 84838 [details] Patch

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

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

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

Created attachment 87976 [details] Patch

Comment on attachment 87976 [details] Patch OK. Assuming it works. :)

Comment on attachment 87976 [details] Patch Clearing flags on attachment: 87976 Committed r89201: <http://trac.webkit.org/changeset/89201>

All reviewed patches have been landed. Closing bug.