Closed Bug 1075072 Opened 10 years ago Closed 10 years ago

debuggerArgs is a list not str

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: sfink, Assigned: tromey)

References

Details

Attachments

(2 files, 2 obsolete files)

I was attempting to run something, either mach mochitest-plain or mach xpcshell-test, with --debugger-args and it was bombing out.
Attached patch debuggerArgs is a list not str (obsolete) — Splinter Review
Attachment #8497684 - Flags: review?(gps)
Attachment #8497684 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/b43ed5b85b1a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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.
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.
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d29a91fd9c40
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
(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.
The xpcshell bit was filed as bug 1088059 (with a patch) FYI.
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.
Assignee: sphink → ttromey
Status: REOPENED → ASSIGNED
Attachment #8510471 - Flags: review?(ted)
Flags: needinfo?(sphink)
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+
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
Attachment #8511120 - Flags: review?(ted)
Attachment #8511120 - Flags: review?(ted) → review+
Keywords: checkin-needed
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.
Flags: needinfo?(sphink)
Keywords: checkin-needed
Target Milestone: mozilla35 → ---
https://hg.mozilla.org/mozilla-central/rev/43a764935c1b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Flags: needinfo?(sphink)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.