Closed
Bug 1075072
Opened 10 years ago
Closed 10 years ago
debuggerArgs is a list not str
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox35 fixed, firefox36 fixed)
RESOLVED
FIXED
mozilla36
People
(Reporter: sfink, Assigned: tromey)
References
Details
Attachments
(2 files, 2 obsolete files)
729 bytes,
text/plain
|
Details | |
3.47 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
I was attempting to run something, either mach mochitest-plain or mach xpcshell-test, with --debugger-args and it was bombing out.
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8497684 -
Flags: review?(gps)
Updated•10 years ago
|
Attachment #8497684 -
Flags: review?(gps) → review+
Reporter | ||
Comment 2•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b43ed5b85b1a
https://hg.mozilla.org/mozilla-central/rev/b43ed5b85b1a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 4•10 years ago
|
||
Historically --debugger-args had been a string containing the arguments to be passed to the debugger. bug 1067037 broke that temporarily but fixed it.At least one other location in the tree still assume --debugger-args to be a string. http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#1287 Other locations use store instead of append and will treat --debugger-args as a string: http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#168 If we want to change --debugger-args to specify arguments to the debugger using multiple options instead of passing them as a string, I think this patch needs more work.
Comment 5•10 years ago
|
||
Reporter | ||
Comment 6•10 years ago
|
||
I tried the command ./mach mochitest-chrome --debugger=debug --debugger-args=-i toolkit/components/osfile/tests/mochi/test_osfile_async.xul and it ended up calling my "debug" command with "-" as an argument, which smells like another list/str confusion.
Reporter | ||
Comment 7•10 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d29a91fd9c40
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 8•10 years ago
|
||
This is a command that fails with the current code: ./mach xpcshell-test --debugger=debug --debugger-args=-i
The backout missed the uplift to Aurora, so this is probably still broken there.
Flags: needinfo?(sphink)
Comment 10•10 years ago
|
||
(In reply to Steve Fink [:sfink] from comment #8) > This is a command that fails with the current code: > > ./mach xpcshell-test --debugger=debug --debugger-args=-i Looks like the xpcshell-test splits the string argument before getting the debug info http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#1284 1284 if debugger: 1285 # We need a list of arguments, not a string, to feed into 1286 # the debugger 1287 if debuggerArgs: 1288 debuggerArgs = debuggerArgs.split(); 1289 1290 self.debuggerInfo = mozdebug.get_debugger_info(debugger, debuggerArgs, debuggerInteractive) This was done in Bug 928397 http://hg.mozilla.org/mozilla-central/rev/75043539c509 But the other test runners like layout/tools/reftest/runreftest.py, and testing/mochitest/runtests.py do not split the debuggerArgs. I can successfully run ./mach reftest --debugger=valgrind --debugger-args='--tool=memcheck' ./mach mochitest --debugger=valgrind --debugger-args='--tool=memcheck' while ./mach xpcshell-test --debugger=valgrind --debugger-args='--tool=memcheck' fails. Even though http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozdebug/mozdebug/mozdebug.py#77 states that def get_debugger_info(debugger, debuggerArgs = None, debuggerInteractive = False): 78 ''' ... 87 :param debuggerArgs: If specified, it's the list of arguments to pass to the 88 debugger. A debugger specific separator arguments is appended at the end of 89 that list. ... Everyone except runxpcshelltests.py passes a string. I think the proper fix here is to change runxpcshelltests.py to not split debuggerArgs and to change get_debugger_info's docstring to state that it takes a string containing the debugger arguments.
Comment 11•10 years ago
|
||
The xpcshell bit was filed as bug 1088059 (with a patch) FYI.
Assignee | ||
Comment 12•10 years ago
|
||
Sorry about that other bug; my search didn't find this one. I think this patch implements the approach outlined in comment #10. It works for the case that caused me to file the bug, but I did not test other scenarios.
Updated•10 years ago
|
Assignee: sphink → ttromey
Status: REOPENED → ASSIGNED
Updated•10 years ago
|
Attachment #8510471 -
Flags: review?(ted)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(sphink)
Comment 14•10 years ago
|
||
Comment on attachment 8510471 [details] [diff] [review] pass debuggerArgs as a string to get_debugger_info Review of attachment 8510471 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I missed this in the initial landing. ::: testing/mozbase/mozdebug/mozdebug/mozdebug.py @@ +89,1 @@ > that list. You should probably say something like "separated by spaces", and not say "that list" since it's not really a list.
Attachment #8510471 -
Flags: review?(ted) → review+
Assignee | ||
Comment 15•10 years ago
|
||
This fixes the comment. However, while fixing the comment I read the code and noticed that it does not do what the comment said. For gdb it is important that user arguments come before "--args", so this patch changes the order in which "debugger_arguments" is built as well.
Attachment #8497684 -
Attachment is obsolete: true
Attachment #8510471 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8511120 -
Flags: review?(ted)
Updated•10 years ago
|
Attachment #8511120 -
Flags: review?(ted) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/43a764935c1b (In reply to Wes Kocher (:KWierso) from comment #9) > The backout missed the uplift to Aurora, so this is probably still broken > there.
status-firefox35:
--- → affected
Flags: needinfo?(sphink)
Keywords: checkin-needed
Target Milestone: mozilla35 → ---
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/43a764935c1b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Reporter | ||
Comment 18•10 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/868e0ac431f8 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/050f5af86323
Updated•10 years ago
|
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•