Summary: | rpt --profile --chromium-android throws exception | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||
Component: | Tools / Tests | Assignee: | Eric Seidel (no email) <eric> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, dbates, dpranke, eric, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Philip Rogers
2012-12-14 15:28:15 PST
I had a subtle misunderstanding of *args. Fixing. Created attachment 179579 [details]
Patch
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. 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. Comment on attachment 179579 [details]
Patch
r=me
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 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. Comment on attachment 179579 [details] Patch Clearing flags on attachment: 179579 Committed r137927: <http://trac.webkit.org/changeset/137927> All reviewed patches have been landed. Closing bug. |