- Revision
- 137692
- Author
- e...@webkit.org
- Date
- 2012-12-13 17:58:05 -0800 (Thu, 13 Dec 2012)
Log Message
Callers should not have to stringify args before calling Executive run_command/popen
https://bugs.webkit.org/show_bug.cgi?id=104975
Reviewed by Dirk Pranke.
One could argue that we should match the python call syntax here,
but I think it's a more friendly API if we automagically handle
stringification of args in run_command, etc.
This removes map(unicode, args) from several callsites.
When I first tried to land this change, I didn't realize that
Executive._command_for_printing depended on this behavior
having been applied to args in run_command. The fix is to
call _stringify_args in both run_command and popen.
This is slightly redundant, but given how short args have to be
(due to shell limits), I don't think the double-encode check
matters in practice.
This is slightly complicated by the fact that apache_http_server.py
is the one caller in our codebase which uses shell=True.
shell=True is a well-documented trail-of-tears:
http://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess
but to support this legacy (windows-only) code (which I can't easily test)
I've added an if-hack to avoid stringifying the the popen(shell=True) case.
* Scripts/webkitpy/common/system/executive.py:
(Executive.run_command):
(Executive._stringify_args):
(Executive.popen):
* Scripts/webkitpy/common/system/executive_unittest.py:
(ExecutiveTest.test_auto_stringify_args):
* Scripts/webkitpy/layout_tests/port/chromium_android.py:
(attach_to_pid):
Modified Paths
Diff
Modified: trunk/Tools/ChangeLog (137691 => 137692)
--- trunk/Tools/ChangeLog 2012-12-14 01:45:53 UTC (rev 137691)
+++ trunk/Tools/ChangeLog 2012-12-14 01:58:05 UTC (rev 137692)
@@ -1,3 +1,39 @@
+2012-12-13 Eric Seidel <e...@webkit.org>
+
+ Callers should not have to stringify args before calling Executive run_command/popen
+ https://bugs.webkit.org/show_bug.cgi?id=104975
+
+ Reviewed by Dirk Pranke.
+
+ One could argue that we should match the python call syntax here,
+ but I think it's a more friendly API if we automagically handle
+ stringification of args in run_command, etc.
+ This removes map(unicode, args) from several callsites.
+
+ When I first tried to land this change, I didn't realize that
+ Executive._command_for_printing depended on this behavior
+ having been applied to args in run_command. The fix is to
+ call _stringify_args in both run_command and popen.
+ This is slightly redundant, but given how short args have to be
+ (due to shell limits), I don't think the double-encode check
+ matters in practice.
+
+ This is slightly complicated by the fact that apache_http_server.py
+ is the one caller in our codebase which uses shell=True.
+ shell=True is a well-documented trail-of-tears:
+ http://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess
+ but to support this legacy (windows-only) code (which I can't easily test)
+ I've added an if-hack to avoid stringifying the the popen(shell=True) case.
+
+ * Scripts/webkitpy/common/system/executive.py:
+ (Executive.run_command):
+ (Executive._stringify_args):
+ (Executive.popen):
+ * Scripts/webkitpy/common/system/executive_unittest.py:
+ (ExecutiveTest.test_auto_stringify_args):
+ * Scripts/webkitpy/layout_tests/port/chromium_android.py:
+ (attach_to_pid):
+
2012-12-13 Adrienne Walker <e...@chromium.org>
Unreviewed, rolling out r137645, r137646, and r137667.
Modified: trunk/Tools/Scripts/webkitpy/common/system/executive.py (137691 => 137692)
--- trunk/Tools/Scripts/webkitpy/common/system/executive.py 2012-12-14 01:45:53 UTC (rev 137691)
+++ trunk/Tools/Scripts/webkitpy/common/system/executive.py 2012-12-14 01:58:05 UTC (rev 137692)
@@ -101,9 +101,6 @@
return sys.platform not in ('win32', 'cygwin')
def _run_command_with_teed_output(self, args, teed_output, **kwargs):
- args = map(unicode, args) # Popen will throw an exception if args are non-strings (like int())
- args = map(self._encode_argument_if_needed, args)
-
child_process = self.popen(args,
stdout=self.PIPE,
stderr=self.STDOUT,
@@ -390,8 +387,7 @@
"""Popen wrapper for convenience and to work around python bugs."""
assert(isinstance(args, list) or isinstance(args, tuple))
start_time = time.time()
- args = map(unicode, args) # Popen will throw an exception if args are non-strings (like int())
- args = map(self._encode_argument_if_needed, args)
+ args = self._stringify_args(args)
stdin, string_to_communicate = self._compute_stdin(input)
stderr = self.STDOUT if return_stderr else None
@@ -457,8 +453,22 @@
return argument
return argument.encode(self._child_process_encoding())
+ def _stringify_args(self, *args):
+ # Popen will throw an exception if args are non-strings (like int())
+ string_args = map(unicode, *args)
+ # The Windows implementation of Popen cannot handle unicode strings. :(
+ return map(self._encode_argument_if_needed, string_args)
+
def popen(self, *args, **kwargs):
- return subprocess.Popen(*args, **kwargs)
+ # FIXME: We should always be stringifying the args, but callers who pass shell=True
+ # expect that the exact bytes passed will get passed to the shell (even if they're wrongly encoded).
+ # shell=True is wrong for many other reasons, and we should remove this
+ # hack as soon as we can fix all callers to not use shell=True.
+ if kwargs.get('shell') == True:
+ string_args = args
+ else:
+ string_args = self._stringify_args(*args)
+ return subprocess.Popen(string_args, **kwargs)
def run_in_parallel(self, command_lines_and_cwds, processes=None):
"""Runs a list of (cmd_line list, cwd string) tuples in parallel and returns a list of (retcode, stdout, stderr) tuples."""
Modified: trunk/Tools/Scripts/webkitpy/common/system/executive_unittest.py (137691 => 137692)
--- trunk/Tools/Scripts/webkitpy/common/system/executive_unittest.py 2012-12-14 01:45:53 UTC (rev 137691)
+++ trunk/Tools/Scripts/webkitpy/common/system/executive_unittest.py 2012-12-14 01:58:05 UTC (rev 137692)
@@ -113,6 +113,11 @@
executive.run_command(command_line('echo', 'foo'))
executive.run_command(tuple(command_line('echo', 'foo')))
+ def test_auto_stringify_args(self):
+ executive = Executive()
+ executive.run_command(command_line('echo', 1))
+ executive.popen(command_line('echo', 1)).wait()
+
def test_run_command_with_unicode(self):
"""Validate that it is safe to pass unicode() objects
to Executive.run* methods, and they will return unicode()
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py (137691 => 137692)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py 2012-12-14 01:45:53 UTC (rev 137691)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py 2012-12-14 01:58:05 UTC (rev 137692)
@@ -415,7 +415,6 @@
assert(self._perf_process == None)
# FIXME: This can't be a fixed timeout!
cmd = self._adb_command + ['shell', 'perf', 'record', '-g', '-p', pid, 'sleep', 30]
- cmd = map(unicode, cmd)
self._perf_process = self._host.executive.popen(cmd)
def _perf_version_string(self, perf_path):
