928397 - On Windows, mach debug should default to use devenv -debugexe

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Benjamin Smedberg]

mach debug currently looks for gdb on Windows. It should default to use devenv.exe -debugexe

Comment 1

[Alessio Placitelli [:Dexter]]

In order to avoid code duplication, "mach debug" ( http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#757 ) could make use of the getDebuggerInfo helper function (http://dxr.mozilla.org/mozilla-central/source/build/automationutils.py#241), being used in xpcshell test debugging.

We've added MSVC and VCExpress support for it in Bug 1028090. This would allow us to have a single function handling debugger information.

Do you thing that could be ok or do we need something more complex than that?

Comment 2

[(not currently active) Ted Mielczarek]

I'd like to move things out of automationutils (bug 1027119). The wrinkle here is that anything that our test harnesses use needs to either be in the test harness itself or in a module under testing/mozbase, because our tests are run from the test package that we produce and not from the source tree. I think we could probably move the debugger bits to mozbase somewhere. gps: what do you think about that?

Comment 3

[Gregory Szorc [:gps]]

I'm not going to speak from an ivory tower: do whatever needs done to make things work.

I'd like to think that any sufficiently standalone Python package can be added to our testing infrastructure (by way of tests packaging) relatively easily. mozbase does sound like the simplest solution, however.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_dbg.patch

I tried to move all the debug information code out of automation utils into its own new package, "mozdebug" (thanks both Gregory and Ted for the suggestions).

With the proposed patch both "test harness" and "./mach debug" use this new module, and thus the same code, to detect the default debugger for the platform and gather the related information.

This also affects Bug 1028090, making the patch I've proposed there obsolete (that's why I've flagged Ted Mielczarek as well).

I've tested ./mach debug and test harness both on my local Windows and Ubuntu machines: it works.

I've got a couple of doubts:

- How can I make sure it works on other platforms as well? (I don't think pushing to try server would help, but I might be wrong)

- I can't think of a proper unit test for this package, since it relies on locally installed debuggers to work (well, except testing the default debugger detection by faking the detected OS): do you have any suggestion?

Thanks!

Comment 5

[Gregory Szorc [:gps]]

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

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

Nit: you have a lot of trailing whitespace in the new code. I suggest you configure your editor to display it. You may even want to strip trailing whitespace on save. You can also install the mozext Mercurial extension (https://hg.mozilla.org/hgcustom/version-control-tools/file/default/hgext/mozext) to have it warn about Python style violations when you commit.

Testing this is painful. I believe a full Try push will exercise some of it. But I believe a lot doesn't have coverage. Spot checking should be sufficient. I'm sure developers will complain loudly if anything breaks. I'd plan on being around for a few days when you push this so people don't get inconvenienced.

The subject of testing mach commands has come up a lot recently. We really need a better story there. I've seriously considered modifying Mercurial's test runner to be portable, as it fits the bill nicely. https://bitbucket.org/brodie/cram/ is a knock-off of Mercurial's code that may also be suitable. I think testing developer-facing tools should be on someone's goals list, given the impact on productivity when things break.

To help with review, you may want to split this into multiple, self-contained patches.

Also, Ted is definitely a more appropriate reviewer than me, especially for the mozbase foo.

::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
@@ +76,5 @@
> +
> +    def __init__(self):
> +        self.cwd = os.getcwd()
> +
> +    def getDebuggerInfo(self, debugger, debuggerArgs = None, debuggerInteractive = False):

Please use methods_with_underscores for new code.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug928397_mozdebug.patch

I've split the patch in two, removed the whitespaces and renamed the function to use underscores.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: bug928397_mozdebug_integration.patch

This patch allows the tools to use mozdebug.

Comment 8

[Alessio Placitelli [:Dexter]]

Gregory, thank you for the time you spent reviewing my code. I've followed your suggestion (split the patch, renamed the functions and put Ted as the reviewer). You're definitely right: testing this components seems particularly tricky. I'll keep in mind to stick around once this gets into m-c!

Also, thanks for suggesting that mercurial extension: I didn't know about it!

Thanks again!

(In reply to Gregory Szorc [:gps] from comment #5)
> Comment on attachment 8447738 [details] [diff] [review]
> bug_xpcshell_dbg.patch
> 
> Review of attachment 8447738 [details] [diff] [review]:

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8449718 [details] [diff] [review]
bug928397_mozdebug_integration.patch

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

This patch was easier to review first. Looks good!

::: build/automationutils.py
@@ -40,5 @@
>    "dumpLeakLog",
>    "isURL",
>    "processLeakLog",
> -  "getDebuggerInfo",
> -  "DEBUGGER_INFO",

Removing things from automationutils makes me happy. :)

::: testing/mochitest/runtests.py
@@ +29,5 @@
>  import traceback
>  import urllib2
>  import zipfile
>  
> +from automationutils import environment, isURL, KeyValueParseError, parseKeyValue, processLeakLog, dumpScreen, ShutdownLeaks, printstatus, LSANLeaks

While you're here, can you reformat this like:
from automationutils import (
   environment,
   ...
)

::: testing/xpcshell/mach_commands.py
@@ +143,5 @@
> +        # We need to attach the '.exe' extension on Windows for the debugger to
> +        # work properly.
> +        xpcsExecutable = 'xpcshell'
> +        if os.name == 'nt':
> +          xpcsExecutable += '.exe'

You can use self.get_binary_path('xpcshell') here instead:
http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/base.py#299

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8449716 [details] [diff] [review]
bug928397_mozdebug.patch

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

Thanks for the patch! Sorry for the review delay, I'll re-review a new revision faster, I promise. I'm giving this r- because I have a lot of comments on it, but overall it's in pretty good shape. With a little bit of work this should be able to land.

One overall nit: we use 'single quote' strings in new Python nowadays, so it'd be great if you could change that. (Should be an easy search-and-replace.)

As a followup, it would be great to move the commandline options stuff from automationutils into mozdebug. That might be a little bit of a pain since we have a lot of stuff using optparse.OptionParser but other things using argparse, but we can sort that out.

::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
@@ +13,5 @@
> +
> +To add support for a new debugger, simply add the relative entry in
> +_DEBUGGER_INFO and optionally update the _DEBUGGER_PRIORITIES.
> +"""
> +_DEBUGGER_INFO = {

It'd be good to explicitly define __all__ to the set of things you want to export since you have these variables up in global scope.

@@ +61,5 @@
> +}
> +
> +# Maps each OS platform to the preferred debugger programs found in _DEBUGGER_INFO.
> +_DEBUGGER_PRIORITIES = {
> +      "windows": ["devenv.exe", "wdexpress.exe", "gdb"],

I don't think I'd list gdb here at all, honestly. If someone wants to use --debugger=gdb it should work, but I wouldn't give that to a Windows developer by default in any situation.

@@ +63,5 @@
> +# Maps each OS platform to the preferred debugger programs found in _DEBUGGER_INFO.
> +_DEBUGGER_PRIORITIES = {
> +      "windows": ["devenv.exe", "wdexpress.exe", "gdb"],
> +      "linux": ["gdb", "cgdb", "lldb"],
> +      "mac": ["gdb"],

Presumably this wants to be ['lldb', 'gdb'] nowadays?

@@ +67,5 @@
> +      "mac": ["gdb"],
> +      "unknown": ["gdb"]
> +}
> +
> +class MozDebug(object):

Is there a lot of utility to having this be a class rather than a collection of methods on the module? Seems like for callers, just calling:
mozdebug.get_debugger_info()
mozdebug.get_default_debugger_name()

would be simpler.

@@ +93,5 @@
> +
> +        debuggerInfo = None
> +
> +        if debugger:
> +            debuggerPath = self.search_path(debugger)

It would be nice to append .exe to the debugger on Windows if it's not present, so things like "--debugger=devenv" would work.

@@ +110,5 @@
> +            "path": debuggerPath,
> +            "interactive" : get_debugger_info("interactive", False),
> +            "args": get_debugger_info("args", "").split(),
> +            "requiresEscapedArgs": get_debugger_info("requiresEscapedArgs", False)
> +        }

I think it'd be a little nicer to return a class with attributes rather than a dict here, like:
class DebuggerInfo:
  def __init__(self, path, args, requiresEscapedArgs):
     self.path = path
     ...

so callers can just say "debuggerInfo.path".

@@ +122,5 @@
> +            debuggerInfo["interactive"] = debuggerInteractive
> +
> +        return debuggerInfo
> +
> +    def get_default_debugger_name(self, keepLooking = False):

I'm not wild about having a boolean argument here, they tend to make call sites confusing (since unless you read the source of this method you have no idea what it means). If Python would let you require the keyword at call sites it'd be fine, but it doesn't. You could make up an enum for this, like:
class DebuggerSearch:
  OnlyFirst = 1
  KeepLooking = 2

def get_default_debugger_name(self, search=DebuggerSearch.OnlyFirst):

@@ +138,5 @@
> +            debuggerPriorities = _DEBUGGER_PRIORITIES["windows"]
> +        elif mozinfo.isLinux:
> +            debuggerPriorities = _DEBUGGER_PRIORITIES["linux"]
> +        elif mozinfo.isMac:
> +            debuggerPriorities = _DEBUGGER_PRIORITIES["mac"]

I would just write:
priorities = _DEBUGGER_PRIORITIES[mozinfo.os if mozinfo.os in _DEBUGGER_PRIORITIES else 'unknown']

@@ +162,5 @@
> +    def search_path(self, execuablePath):
> +        """
> +        Try to find
> +        Go one step beyond get_full_path and try the various folders in PATH
> +        """

I don't think you should reinvent this wheel. You should be able to use:
http://nullege.com/codes/search/distutils.spawn.find_executable

Alternately, you could make the mozdebug package have a requirement for the 'which' package:
https://pypi.python.org/pypi/which

That's already installed in the objdir virtualenv, but might be an issue for running on our test machines.

::: testing/mozbase/mozdebug/setup.py
@@ +7,5 @@
> +PACKAGE_VERSION = '0.1'
> +
> +setup(name='mozdebug',
> +      version=PACKAGE_VERSION,
> +      description="Python debugging utilities intended for use with Mozilla testing",

I would phrase this as "Utilities for running applications under native code debuggers intended for use in Mozilla testing". (Writing "Python debugging utilities" seems misleading.)

::: testing/mozbase/mozdebug/tests/basic.py
@@ +4,5 @@
> +import unittest
> +
> +
> +class TestBasic(unittest.TestCase):
> +    """ Test basic Mozdebug capabilites """

If you don't have any unit tests it's probably better to just leave this out rather than give the appearance of having unit tests. Unit tests would be nice to have, but it's okay to punt on them for now. It's not like the existing code you're replacing has any tests.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 8449716 [details] [diff] [review]
bug928397_mozdebug.patch

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

::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
@@ +87,5 @@
> +        Please note that |debuggerArgs| needs to be a list. This function
> +        will append the debugger specific separator arguments at the end
> +        of that list.
> +
> +        If the debugger cannot be found in the system, returns |None|.

There's a more-correct way to write docstrings and label function parameters, FYI:
http://hg.mozilla.org/mozilla-central/annotate/1aab16f74817/testing/mozbase/mozdevice/mozdevice/devicemanager.py#l205

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> Comment on attachment 8449718 [details] [diff] [review]
> bug928397_mozdebug_integration.patch
> 
> Review of attachment 8449718 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch was easier to review first. Looks good!

Thanks :-) Removing was easier than adding :P

> ::: testing/xpcshell/mach_commands.py
> @@ +143,5 @@
> > +        # We need to attach the '.exe' extension on Windows for the debugger to
> > +        # work properly.
> > +        xpcsExecutable = 'xpcshell'
> > +        if os.name == 'nt':
> > +          xpcsExecutable += '.exe'
> 
> You can use self.get_binary_path('xpcshell') here instead:
> http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/base.
> py#299

Unfortunately, when using this function, it seems to look for the "xpcshell" executable in the wrong folder (bug it's correctly adding the .exe to the filename). The "where" parameter does not seem to help :(

"Exception: Binary expected at c:\mozilla-source\objdir-ff-debug\dist\bin\xpcshell.exe does not exist."

Should I keep doing what I was doing before?

Comment 13

[(not currently active) Ted Mielczarek]

That's where xpcshell should be, it's the same path you were calculating in your script. Do you have your mozconfig set properly? Have you done a build in the objdir you're using?

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_dbg_v2.patch

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: bug_debug_integration_v2.patch

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: bug_debug_integration_v2.patch

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_dbg_v2.patch

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: bug_debug_integration_v2.patch

Due to the changes in mozdebug from the other patch, the integration within the rest of the testing framework needs to change as well. This patch handles this!

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8469301 [details] [diff] [review]
bug_xpcshell_dbg_v2.patch

This patch includes the suggestions from Ted's latest review.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 8469301 [details] [diff] [review]
bug_xpcshell_dbg_v2.patch

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

Thanks, this looks good! Just a few nitpicky things.

::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
@@ +23,5 @@
> +    # gdb requires that you supply the '--args' flag in order to pass arguments
> +    # after the executable name to the executable.
> +    'gdb': {
> +        'interactive': True,
> +        'args': '-q --args'

Any reason not to just store these as lists in the dict here instead of .split()ting them later?

@@ +113,5 @@
> +        return default
> +
> +    # Define a class with attributes to access the debugger information
> +    # from the outside world.
> +    class DebuggerInfo:

You could just use a namedtuple here if you want:
from collections import namedtuple
DebuggerInfo = namedtuple('DebuggerInfo', ['path', 'interactive', 'args', 'requiresEscapedArgs'])

then you use it just like this class:
d = DebuggerInfo('gdb', True, ['x','y'], False)

@@ +130,5 @@
> +
> +    if debuggerArgs:
> +        # Make sure to append the argument separator (if any) after the
> +        # provided arguments.
> +        debuggerInfo.args = debuggerArgs.extend(debuggerInfo.args);

It seems wrong to call .extend here on the argument being passed in. I think you just want to say
debuggerInfo.args = debuggerArgs + debuggerInfo.args

@@ +152,5 @@
> +     looking for other compatible debuggers (|DebuggerSearch.KeepLooking|).
> +    '''
> +
> +    # Find out which debuggers are preferred for use on this platform.
> +    debuggerPriorities = _DEBUGGER_PRIORITIES[mozinfo.os if mozinfo.os in _DEBUGGER_PRIORITIES else 'unknown']

You could alternately just write:
debuggerPriorities = _DEBUGGER_PRIORITIES.get(mozinfo.os, ['gdb'])

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 8469308 [details] [diff] [review]
bug_debug_integration_v2.patch

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

Looks good, thanks! As a follow-up, it might be nice to move the --debugger-* things from automationutils.addCommonOptions into a method in mozdebug.

Comment 22

[Alessio Placitelli [:Dexter]]

Attachment: bug_debug_integration_v3.patch

I've added the missing r=ted and rebased the patch against latest m-c.

Comment 23

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_dbg_v3.patch

Thanks for the review! I've updated the patch to use namedtuples and to comply with the other suggestions.

As for the last suggestion, I'd rather keep all the debugger executable names in the  _DEBUGGER_PRIORITIES structure. Would you mind if I leave it as it is now?

Comment 24

[(not currently active) Ted Mielczarek]

That's fine, it was just a suggestion.

Comment 25

[Alessio Placitelli [:Dexter]]

Attachment: bug_debug_integration_v3.patch

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_dbg_v3.patch

Thanks Ted, and thank you for your time reviewing this (updated the patches with the correct name in r= and fixed a nit).

Comment 27

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_dbg_v4.patch

Comment 28

[Alessio Placitelli [:Dexter]]

Attachment: bug_xpcshell_docs.patch

This patch enables auto generation of mozdebug documentation (http://mozbase.readthedocs.org/en/latest/).

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 8486604 [details] [diff] [review]
bug_xpcshell_docs.patch

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

Thanks for fixing the docs!

Comment 30

[Carsten Book [:Tomcat]]

Hi,

could you provide a try link, just to make sure everything works and build? thanks!

Comment 31

[Alessio Placitelli [:Dexter]]

Hi Carsten, I thought a try build wasn't needed for this patch (see a similar issue with bug 1028090 comment 15)! But it's better to be safe than sorry so... Ted, could you confirm that we don't need a try push for this?

Comment 32

[(not currently active) Ted Mielczarek]

These patches do touch some of the test harnesses, so a Try push would probably make the sheriffs happier, but I'm happy to push them for you without one.

Comment 33

[Alessio Placitelli [:Dexter]]

No problem at all, thanks for the clarification!

The try push: https://tbpl.mozilla.org/?tree=Try&rev=9f66f7034b41

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #32)
> These patches do touch some of the test harnesses, so a Try push would
> probably make the sheriffs happier, but I'm happy to push them for you
> without one.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75043539c509
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1094720f84e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5fed405b904

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/75043539c509
https://hg.mozilla.org/mozilla-central/rev/d1094720f84e
https://hg.mozilla.org/mozilla-central/rev/a5fed405b904

Comment 36

[Stephen A Pohl [:spohl]]

Attachment: Patch for aurora

The v2 signature changes for OSX will be uplifted to aurora and one of the bugs (bug 1060562) relies on the changes here to function properly. We thought it best to simply uplift the patches here as well. This combines all three patches for uplift to aurora. Commit message has a=tests per lmandel, since this is tests related.

Comment 37

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf6abd726eac