Bug 105061 – rpt --profile --chromium-android throws exception

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

[Philip Rogers]

pdr@yourcomputer:/ssd/chromium$ ./src/third_party/WebKit/Tools/Scripts/run-perf-tests --release --profile --chromium-android src/third_party/WebKit/PerformanceTests/Bindings/first-child.html

Throws the following:
ANDROID_SYMFS not set, using /ssd/chromium/src/webkit/Release/layout-test-results/symfs
Updating symfs libary (/ssd/chromium/src/webkit/Release/layout-test-results/symfs/data/app-lib/org.chromium.native_test-1/libDumpRenderTree.so) from built copy (/ssd/chromium/src/out/Release/lib/libDumpRenderTree.so)
Running Bindings/first-child.html (1 of 1)
Traceback (most recent call last):
  File "./src/third_party/WebKit/Tools/Scripts/run-perf-tests", line 40, in <module>
    sys.exit(PerfTestsRunner().run())
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py", line 190, in run
    unexpected = self._run_tests_set(sorted(list(tests), key=lambda test: test.test_name()), self._port)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py", line 330, in _run_tests_set
    if self._run_single_test(test, driver):
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py", line 344, in _run_single_test
    new_results = test.run(driver, self._options.time_out_ms)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/performance_tests/perftest.py", line 71, in run
    output = self.run_single(driver, self.path_or_url(), time_out_ms)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/performance_tests/perftest.py", line 78, in run_single
    return driver.run_test(DriverInput(path_or_url, time_out_ms, image_hash=None, should_run_pixel_test=should_run_pixel_test), stop_when_done=False)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 745, in run_test
    return super(ChromiumAndroidDriver, self).run_test(driver_input, stop_when_done)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py", line 165, in run_test
    self.start(driver_input.should_run_pixel_test, driver_input.args)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 754, in start
    super(ChromiumAndroidDriver, self).start(pixel_tests, per_test_args)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py", line 280, in start
    self._start(pixel_tests, per_test_args)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 757, in _start
    self._setup_test()
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 553, in _setup_test
    self._setup_md5sum_and_push_data_if_needed()
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 544, in _setup_md5sum_and_push_data_if_needed
    self._push_executable()
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 597, in _push_executable
    self._push_file_if_needed(self._port.path_to_forwarder(), DEVICE_FORWARDER_PATH)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 591, in _push_file_if_needed
    stdout=subprocess.PIPE).stdout)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 488, in popen
    string_args = self._stringify_args(*args)
  File "/ssd/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 476, in _stringify_args
    string_args = map(unicode, *args)
TypeError: map() requires at least two args

[Eric Seidel (no email)]

I had a subtle misunderstanding of *args.  Fixing.

[Eric Seidel (no email)]

Created attachment 179579 [details]
Patch

[Eric Seidel (no email)]

At some point along the line, popen(args, **kwargs) got written as popen(*args, **kwargs), which worked fine when we were just passing along *args to subprocess.popen.

But popen actually only has one non-keyword argument, and that happens to be (confusingly) named "args".  So at least one caller (in ChromiumAndroidDriver) was calling popen(args=[...]) explicitly naming the "args" argument. 

Thus when you write:

def popen(*args, **kwargs)

"args" is then a tuple of the non-keyword arguments, commonly containing as args[0] the list of arguments known as subprocess.Popens "args".

We hit this bug because when you expand an empty tuple using the "*" operator, you get back nothing, no arguments, so:

map(unicode, *args), turns into:

map(unicode)

when args is an empty tuple.

The fix here is to not bother with the argument globbing (since we only have a single non-keyword argument anyway!) and instead write:

def popen(args, **kwargs)

The only reason this is at all confusing is because we have a naming collision between the standard "varargs" pattern for python and the "args" name for the first argument of subprocess.popen.

[Eric Seidel (no email)]

In python all arguments are "named" and can be passed as keyword args (aka kwargs) to a function.

When you want to glob the arguments (aka varargs in C) you use the standard pattern:

def my_function(*args, **kwargs)

Which gives you a tuple "args" containing all the arguments which were passed w/o keywords, and a dictionary kwargs with all the named args.

Using * and ** respectively will expand a tuple or dictionary back into arguments when calling a function:

some_other_function(*tuple, **dictionary)

You don't have to use both, of course, but it's very common to see *args, **kwargs in code.

[Daniel Bates]

Comment on attachment 179579 [details]
Patch

r=me

[Daniel Bates]

Comment on attachment 179579 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/executive.py:480
> +    # The only required arugment to popen is named "args", the rest are optional keyword arguments.

Nit: arugment => argument

[Dirk Pranke]

Comment on attachment 179579 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/executive.py:480
>> +    # The only required arugment to popen is named "args", the rest are optional keyword arguments.
> 
> Nit: arugment => argument

I probably wouldn't bother with the comment. I also considered suggesting changing the name of the argument from "args" to "cmd" (or something) to avoid the potential confusion, but I think leaving the name the same as the underlying popen() is probably a better idea.

[WebKit Review Bot]

Comment on attachment 179579 [details]
Patch

Clearing flags on attachment: 179579

Committed r137927: <http://trac.webkit.org/changeset/137927>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.