Patchwork [01,of,14,RFC] dispatch: add ability to specify ipdb as a debugger

login
register
mail settings
Submitter Sean Farley
Date July 9, 2013, 9:54 p.m.
Message ID <2bda9c94e65cff8c6968.1373406872@laptop.local>
Download mbox | patch
Permalink /patch/1811/
State Superseded
Headers show

Comments

Sean Farley - July 9, 2013, 9:54 p.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1372876982 18000
#      Wed Jul 03 13:43:02 2013 -0500
# Node ID 2bda9c94e65cff8c6968d82cfc07ec6049b194c0
# Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
dispatch: add ability to specify ipdb as a debugger

Question: is there a better way to test a module's availability with
demandimport?
Matt Mackall - July 9, 2013, 10:10 p.m.
On Tue, 2013-07-09 at 16:54 -0500, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1372876982 18000
> #      Wed Jul 03 13:43:02 2013 -0500
> # Node ID 2bda9c94e65cff8c6968d82cfc07ec6049b194c0
> # Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
> dispatch: add ability to specify ipdb as a debugger
> 
> Question: is there a better way to test a module's availability with
> demandimport?
> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -37,11 +37,11 @@
>      ('v', 'verbose', None, _('enable additional output')),
>      ('', 'config', [],
>       _('set/override config option (use \'section.name=value\')'),
>       _('CONFIG')),
>      ('', 'debug', None, _('enable debugging output')),
> -    ('', 'debugger', None, _('start debugger')),
> +    ('', 'debugger', 'pdb', _('start debugger')),

Ok, so we're changing --debugger to accept an arg..

> +    if '--debugger' in req.args:
> +        debugger = 'pdb'
> +    debugarg = _earlygetopt(['--debugger'], req.args)

And then doing some special voodoo to allow it to not accept an arg?
This is unfortunate. We've carefully avoided having optional args until
now because they're stupidly complicated. And we can't have a mandatory
arg to --debugger either.

Btw, I don't see you using debugarg[1], so you've almost certainly got a
bug here.

This might need to be an environment variable or (if possible) an hgrc
setting.
Sean Farley - July 9, 2013, 10:33 p.m.
mpm@selenic.com writes:

> On Tue, 2013-07-09 at 16:54 -0500, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1372876982 18000
>> #      Wed Jul 03 13:43:02 2013 -0500
>> # Node ID 2bda9c94e65cff8c6968d82cfc07ec6049b194c0
>> # Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
>> dispatch: add ability to specify ipdb as a debugger
>> 
>> Question: is there a better way to test a module's availability with
>> demandimport?
>> 
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -37,11 +37,11 @@
>>      ('v', 'verbose', None, _('enable additional output')),
>>      ('', 'config', [],
>>       _('set/override config option (use \'section.name=value\')'),
>>       _('CONFIG')),
>>      ('', 'debug', None, _('enable debugging output')),
>> -    ('', 'debugger', None, _('start debugger')),
>> +    ('', 'debugger', 'pdb', _('start debugger')),
>
> Ok, so we're changing --debugger to accept an arg..
>
>> +    if '--debugger' in req.args:
>> +        debugger = 'pdb'
>> +    debugarg = _earlygetopt(['--debugger'], req.args)
>
> And then doing some special voodoo to allow it to not accept an arg?

Hmm, I thought that was how args were retrieved in dispatch.py? What is
usable in dispatch.py at this point in the code for getting an optional
argument?

> This is unfortunate. We've carefully avoided having optional args until
> now because they're stupidly complicated. And we can't have a mandatory
> arg to --debugger either.

Ah, I didn't realize there were no optional args.

> Btw, I don't see you using debugarg[1], so you've almost certainly got a
> bug here.

In the next line?

+    if debugarg:
+        debugger = debugarg[0]

Or was the "[1]" supposed to refer to something I missed?

> This might need to be an environment variable or (if possible) an hgrc
> setting.

Aha, that might indeed be a better idea. The question still remains on
how to check for an existence of a module with-in this demandimport
funny business?
Pierre-Yves David - July 10, 2013, 12:18 a.m.
On 10 juil. 2013, at 00:33, Sean Farley wrote:

>> This might need to be an environment variable or (if possible) an hgrc
>> setting.
> 
> Aha, that might indeed be a better idea. The question still remains on
> how to check for an existence of a module with-in this demandimport
> funny business?

import the module and actually try to use something in it. The demand import wiki page or module documentation most probably cover this question
Matt Mackall - July 10, 2013, 6:53 p.m.
On Tue, 2013-07-09 at 17:33 -0500, Sean Farley wrote:
> mpm@selenic.com writes:
> 
> > On Tue, 2013-07-09 at 16:54 -0500, Sean Farley wrote:
> >> # HG changeset patch
> >> # User Sean Farley <sean.michael.farley@gmail.com>
> >> # Date 1372876982 18000
> >> #      Wed Jul 03 13:43:02 2013 -0500
> >> # Node ID 2bda9c94e65cff8c6968d82cfc07ec6049b194c0
> >> # Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
> >> dispatch: add ability to specify ipdb as a debugger
> >> 
> >> Question: is there a better way to test a module's availability with
> >> demandimport?
> >> 
> >> diff --git a/mercurial/commands.py b/mercurial/commands.py
> >> --- a/mercurial/commands.py
> >> +++ b/mercurial/commands.py
> >> @@ -37,11 +37,11 @@
> >>      ('v', 'verbose', None, _('enable additional output')),
> >>      ('', 'config', [],
> >>       _('set/override config option (use \'section.name=value\')'),
> >>       _('CONFIG')),
> >>      ('', 'debug', None, _('enable debugging output')),
> >> -    ('', 'debugger', None, _('start debugger')),
> >> +    ('', 'debugger', 'pdb', _('start debugger')),
> >
> > Ok, so we're changing --debugger to accept an arg..
> >
> >> +    if '--debugger' in req.args:
> >> +        debugger = 'pdb'
> >> +    debugarg = _earlygetopt(['--debugger'], req.args)
> >
> > And then doing some special voodoo to allow it to not accept an arg?
> 
> Hmm, I thought that was how args were retrieved in dispatch.py? What is
> usable in dispatch.py at this point in the code for getting an optional
> argument?

It's voodoo because there's no support anywhere for optional args.

> > This is unfortunate. We've carefully avoided having optional args until
> > now because they're stupidly complicated. And we can't have a mandatory
> > arg to --debugger either.
> 
> Ah, I didn't realize there were no optional args.

Optional args make parsing ambiguous and require the parsers to have
intimate knowledge of what sorts of arguments are acceptable.

As a contrived example, imagine that someone had an ipdb alias. Now this
is ambiguous:

hg --debugger ipdb foo

> > Btw, I don't see you using debugarg[1], so you've almost certainly got a
> > bug here.
> 
> In the next line?
> 
> +    if debugarg:
> +        debugger = debugarg[0]
> 
> Or was the "[1]" supposed to refer to something I missed?

debugarg[1] returns the unused arguments.. so you can use them
elsewhere. If you're not using debugarg[1] somewhere, it probably means
you're using the _original_ argument list somewhere, still containing
pdb/ipdb. 

> > This might need to be an environment variable or (if possible) an hgrc
> > setting.
> 
> Aha, that might indeed be a better idea. The question still remains on
> how to check for an existence of a module with-in this demandimport
> funny business?

Reference anything contained in the module.
Sean Farley - July 10, 2013, 7 p.m.
On Wed, Jul 10, 2013 at 1:53 PM, Matt Mackall <mpm@selenic.com> wrote:
> On Tue, 2013-07-09 at 17:33 -0500, Sean Farley wrote:
>> mpm@selenic.com writes:
>>
>> > On Tue, 2013-07-09 at 16:54 -0500, Sean Farley wrote:
>> >> # HG changeset patch
>> >> # User Sean Farley <sean.michael.farley@gmail.com>
>> >> # Date 1372876982 18000
>> >> #      Wed Jul 03 13:43:02 2013 -0500
>> >> # Node ID 2bda9c94e65cff8c6968d82cfc07ec6049b194c0
>> >> # Parent  648d1974b3f328947ee6cf2d00c66815a33cd208
>> >> dispatch: add ability to specify ipdb as a debugger
>> >>
>> >> Question: is there a better way to test a module's availability with
>> >> demandimport?
>> >>
>> >> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> >> --- a/mercurial/commands.py
>> >> +++ b/mercurial/commands.py
>> >> @@ -37,11 +37,11 @@
>> >>      ('v', 'verbose', None, _('enable additional output')),
>> >>      ('', 'config', [],
>> >>       _('set/override config option (use \'section.name=value\')'),
>> >>       _('CONFIG')),
>> >>      ('', 'debug', None, _('enable debugging output')),
>> >> -    ('', 'debugger', None, _('start debugger')),
>> >> +    ('', 'debugger', 'pdb', _('start debugger')),
>> >
>> > Ok, so we're changing --debugger to accept an arg..
>> >
>> >> +    if '--debugger' in req.args:
>> >> +        debugger = 'pdb'
>> >> +    debugarg = _earlygetopt(['--debugger'], req.args)
>> >
>> > And then doing some special voodoo to allow it to not accept an arg?
>>
>> Hmm, I thought that was how args were retrieved in dispatch.py? What is
>> usable in dispatch.py at this point in the code for getting an optional
>> argument?
>
> It's voodoo because there's no support anywhere for optional args.

Ah, yes, I see now.

>> > This is unfortunate. We've carefully avoided having optional args until
>> > now because they're stupidly complicated. And we can't have a mandatory
>> > arg to --debugger either.
>>
>> Ah, I didn't realize there were no optional args.
>
> Optional args make parsing ambiguous and require the parsers to have
> intimate knowledge of what sorts of arguments are acceptable.
>
> As a contrived example, imagine that someone had an ipdb alias. Now this
> is ambiguous:
>
> hg --debugger ipdb foo

This was definitely in the category of
"didn't-think-through-other-combinations" on my part.

>> > Btw, I don't see you using debugarg[1], so you've almost certainly got a
>> > bug here.
>>
>> In the next line?
>>
>> +    if debugarg:
>> +        debugger = debugarg[0]
>>
>> Or was the "[1]" supposed to refer to something I missed?
>
> debugarg[1] returns the unused arguments.. so you can use them
> elsewhere. If you're not using debugarg[1] somewhere, it probably means
> you're using the _original_ argument list somewhere, still containing
> pdb/ipdb.

Derp, parsing error on my part. And also not using the method correctly.

>> > This might need to be an environment variable or (if possible) an hgrc
>> > setting.
>>
>> Aha, that might indeed be a better idea. The question still remains on
>> how to check for an existence of a module with-in this demandimport
>> funny business?
>
> Reference anything contained in the module.

As mentioned in IRC, my concern was forcing to load ipdb every time
dispatch.py is loaded. Kevin mentioned that we could perhaps move the
import inside the debug function?
Sean Farley - July 13, 2013, 9:50 p.m.
kbullock+mercurial@ringworld.org writes:

> On 10 Jul 2013, at 2:00 PM, Sean Farley wrote:
>
>> As mentioned in IRC, my concern was forcing to load ipdb every time
>> dispatch.py is loaded. Kevin mentioned that we could perhaps move the
>> import inside the debug function?
>
> The canonical reference for style issues is <http://mercurial.selenic.com/wiki/CodingStyle>; PEP 8 <http://www.python.org/dev/peps/pep-0008/> is also handy. Our own style guide is silent on the issue of imports, but there are certainly other cases where we delay importing a module until we know which version of it we want to use (see util.py).

Thanks for the references and info!

> As I mentioned on IRC (and repeat here for the sake of other contributors), this patch should really be on its own, apart from the series. It's not related to memctx.

I agree; I'll resend shortly.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -37,11 +37,11 @@ 
     ('v', 'verbose', None, _('enable additional output')),
     ('', 'config', [],
      _('set/override config option (use \'section.name=value\')'),
      _('CONFIG')),
     ('', 'debug', None, _('enable debugging output')),
-    ('', 'debugger', None, _('start debugger')),
+    ('', 'debugger', 'pdb', _('start debugger')),
     ('', 'encoding', encoding.encoding, _('set the charset encoding'),
      _('ENCODE')),
     ('', 'encodingmode', encoding.encodingmode,
      _('set the charset encoding mode'), _('MODE')),
     ('', 'traceback', None, _('always print a traceback on exception')),
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -9,10 +9,16 @@ 
 import os, sys, atexit, signal, pdb, socket, errno, shlex, time, traceback, re
 import util, commands, hg, fancyopts, extensions, hook, error
 import cmdutil, encoding
 import ui as uimod
 
+try:
+    import ipdb
+    ipdb.set_trace # should we really be forcing this load in dispatch.py?
+except ImportError:
+    ipdb = pdb
+
 class request(object):
     def __init__(self, args, ui=None, repo=None, fin=None, fout=None,
                  ferr=None):
         self.args = args
         self.ui = ui
@@ -84,26 +90,48 @@ 
             if num:
                 signal.signal(num, catchterm)
     except ValueError:
         pass # happens if called in a thread
 
+    debugger = None
+    if '--debugger' in req.args:
+        debugger = 'pdb'
+    debugarg = _earlygetopt(['--debugger'], req.args)
+    if debugarg:
+        debugger = debugarg[0]
+
+    debugtrace = {
+        'pdb'  : pdb.set_trace,
+        'ipdb' : ipdb.set_trace,
+    }
+
+    debugmortem = {
+        'pdb'  : pdb.post_mortem,
+        'ipdb' : ipdb.post_mortem,
+    }
+
     try:
         try:
             # enter the debugger before command execution
-            if '--debugger' in req.args:
+            if debugger:
                 ui.warn(_("entering debugger - "
                         "type c to continue starting hg or h for help\n"))
-                pdb.set_trace()
+
+                if debugger != 'pdb' and debugtrace[debugger] == debugtrace['pdb']:
+                    ui.warn(_("%s debugger specified "
+                              "but its module was not found\n") % debugger)
+
+                debugtrace[debugger]()
             try:
                 return _dispatch(req)
             finally:
                 ui.flush()
         except: # re-raises
             # enter the debugger when we hit an exception
-            if '--debugger' in req.args:
+            if debugger:
                 traceback.print_exc()
-                pdb.post_mortem(sys.exc_info()[2])
+                debugmortem[debugger](sys.exc_info()[2])
             ui.traceback()
             raise
 
     # Global exception handling, alphabetically
     # Mercurial-specific first, followed by built-in and library exceptions