1028090 - --debugger argument does not support Visual Studio (Windows debugging)

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Alessio Placitelli [:Dexter]]

The xpcshell-test suite does not seem to support debugging of script on Windows. 
It fails with the error:

$ ./mach xpcshell-test --debugger WDExpress.exe --interactive --debugger-interactive my_test.js
Error: Path WDExpress.exe doesn't exist.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_dbg.patch

This simple patch adds the support for Visual Studio and Visual C++ Express.

The following command correctly spawns a Visual Studio instance and attaches the debugger:

./mach xpcshell-test --debugger WDExpress.exe --interactive --debugger-interactive my_test.js

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8443367 [details] [diff] [review]
bug_xpcshell_dbg.patch

Review of attachment 8443367 [details] [diff] [review]:
-----------------------------------------------------------------

this patch seems too easy, I don't have an easy way to test this- but looking at the debugger hooks we seem to be setting the right things.

THanks for supporting our #1 os of users.

Comment 3

[Alessio Placitelli [:Dexter]]

You're welcome! 

You're right, it looks like it's extremely simple. I wonder if there's anything else I can do to or somebody else to ask a feedback to. Suggestions?

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 8443367 [details] [diff] [review]
> bug_xpcshell_dbg.patch
> 
> Review of attachment 8443367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this patch seems too easy, I don't have an easy way to test this- but
> looking at the debugger hooks we seem to be setting the right things.
> 
> THanks for supporting our #1 os of users.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

If this works for you locally, then that is the testing we need.  Further work to complete this loop would be to make a comment in the MDN article:
https://developer.mozilla.org/en-US/docs/Mochitest#Debugging_individual_tests

maybe just indicate which debuggers we support.

Comment 5

[(not currently active) Ted Mielczarek]

bug 928397 claims what we want is "devenv -debugexe ". Also I wish "mach debug" and "--debugger in our test harnesses" shared code.

Comment 6

[Alessio Placitelli [:Dexter]]

Sure, that's true: that's why I've added both devenv support (Visual Studio - non express) and WDExpress (Visual C++ Express Edition) - See http://benjamin.smedbergs.us/blog/2014-03-10/use-debugexe-to-debug-apps-in-visual-studio/ .

I'll take a shot at "mach debug" as well! :) What do you mean by "--debugger in our test harnesses" ?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> bug 928397 claims what we want is "devenv -debugexe ". Also I wish "mach
> debug" and "--debugger in our test harnesses" shared code.

Comment 7

[(not currently active) Ted Mielczarek]

Oops, sorry, hadn't read your patch. Cool! You fixed the "--debugger for test harness" case. What I was saying is that I wish that was the same code behind "mach debug" so we wouldn't have to fix this in two places!

Comment 8

[Alessio Placitelli [:Dexter]]

Ah, awesome :)

I'll dig into that and take on Bug 928397. Meanwhile, should I push this to m-c?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Oops, sorry, hadn't read your patch. Cool! You fixed the "--debugger for
> test harness" case. What I was saying is that I wish that was the same code
> behind "mach debug" so we wouldn't have to fix this in two places!

Comment 9

[(not currently active) Ted Mielczarek]

Yes, that's fine. Thanks for the patch!

Comment 10

[(not currently active) Ted Mielczarek]

I'm assuming this is ready to go?

Comment 11

[Alessio Placitelli [:Dexter]]

Yes, but bug 928397 embeds the code of this patch and moves it to a new package (mozdebug), so I'm not sure it makes sense to commit this one. I was waiting on your feedback on that bug before adding the checkin flag! Should we commit it anyway?

Comment 12

[(not currently active) Ted Mielczarek]

Yeah, it's fine, we can land this one until I get the other one reviewed.

Comment 13

[Carsten Book [:Tomcat]]

patch didn't apply :

applying bug_xpcshell_dbg.patch
patching file build/automationutils.py
Hunk #1 FAILED at 67
1 out of 1 hunks FAILED -- saving rejects to file build/automationutils.py.rej
patch failed, unable to continue (try -v)

could you take a look at this and maybe rebase against  a current tree ?

also we need maybe here a try run or ?

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_dbg.patch

Here we go! I've rebased the patch to work with the latest m-c.

I'm not quite sure a try push would be useful, since I'm not sure the part which I've modified gets tested by that. Does it?

Comment 15

[(not currently active) Ted Mielczarek]

A try run wouldn't be useful here, no. This is almost DONTBUILD (although it does touch automationutils.py, so not completely).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8923901b8b54

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8923901b8b54