`@cmd2.with_argparser`: parser's `prog` value

[fixeria]

In Osmocom, we use cmd2 in pysim — command-line tools for SIM/UICC/USIM/ISIM card analysis and programming. For documentation, we rely on sphinx together with sphinx-argparse to auto-generate docs for each cmd2 command.

This setup has worked well until recently, but we've started seeing artifacts in the generated documentation:

https://ftp.osmocom.org/docs/pysim/master/osmopysim-usermanual.pdf#subsubsection*.56

change_chv

usage: build.py [-h] [--pin-nr PIN_NR] [NEWPIN] [PIN]

Suddenly we're getting build.py instead of the command name (change_chv) in usage.

The usage we're getting when actually running the program looks good:

pySIM-shell (00:MF)> change_chv --help
usage: change_chv [-h] [--pin-nr PIN_NR] [NEWPIN] [PIN]
 
Change PIN code to a new PIN code
 
positional arguments:
  NEWPIN           PIN code value. If none given, CSV file will be queried
  PIN              PIN code value. If none given, CSV file will be queried
 
options:
  -h, --help       show this help message and exit
  --pin-nr PIN_NR  PUK Number, 1=PIN1, 2=PIN2 or custom value (decimal)

Below is definition of the change_chv command:

https://gitea.osmocom.org/sim-card/pysim/src/commit/949c2a2d579943f5286e1f4505be0835f149eab6/pySim-shell.py#L950

@with_default_category('ISO7816 Commands')
class Iso7816Commands(CommandSet):
    # ...
    change_chv_parser = argparse.ArgumentParser()
    change_chv_parser.add_argument('NEWPIN', nargs='?', type=is_decimal,
                                   help='PIN code value. If none given, CSV file will be queried')
    change_chv_parser.add_argument('PIN', nargs='?', type=is_decimal,
                                   help='PIN code value. If none given, CSV file will be queried')
    change_chv_parser.add_argument(
        '--pin-nr', type=int, default=1, help='PUK Number, 1=PIN1, 2=PIN2 or custom value (decimal)')
 
    @cmd2.with_argparser(change_chv_parser)
    def do_change_chv(self, opts):
        """Change PIN code to a new PIN code"""
        new_pin = self.get_code(opts.NEWPIN, "PIN" + str(opts.pin_nr))
        pin = self.get_code(opts.PIN, "PIN" + str(opts.pin_nr))
        (data, sw) = self._cmd.lchan.scc.change_chv(
            opts.pin_nr, h2b(pin), h2b(new_pin))
        self._cmd.poutput("CHV change successful")
 
    print(f'prog={change_chv_parser.prog}') # XXX: debug

I figured out that @cmd2.with_argparser (actually _set_parser_prog()) is in charge of updating the prog attribute. Sphinx is using this attribute to generate the usage.

• v2.4.3 was the last version to retain the old behavior (prog=change_chv).
• v2.5.0 exhibits the new behavior (prog=build.py).

git-bisect pointed out to 8d88c357ca0764b114c68d8175d2cd33146b83d7:

8d88c357ca0764b114c68d8175d2cd33146b83d7 is the first bad commit
commit 8d88c357ca0764b114c68d8175d2cd33146b83d7 (HEAD)
Author: Eric Lin
Date:   Mon Sep 25 17:44:21 2023 -0400
 
    Changed with_argparser() and as_subcommand_to() to accept either an ArgumentParser or a
    factory callable that returns an ArgumentParser.
    Changed Cmd constructor to construct an instance-specific ArgumentParser using either the factory callable
    or by deep-copying the provided ArgumentParser.
    With this change a new argparse instance should be created for each instance of Cmd.
    
    Addresses #1002

Apparently we end up with prog=__name__ after this commit. This looks like a side-effect.

For the record, here's our internal ticket: https://osmocom.org/issues/6776.

[tleonhardt]

@anselor Since this relates to a change you made, would you have any quick ideas for how to address it?

[kmvanbrunt]

Before 2.5.0, command parsers were created when a CLI class was defined. Each parser was a class member and configured with _set_parser_prog() upon creation.

As of 2.5.0, command parsers are configured when a CLI object is instantiated. We make deep copies of the class member parsers, configure them with _set_parser_prog(), and add them to self._command_parsers. The class member parsers aren't ever updated with the new prog value. Only the copies are.

I don't know anything about sphinx-argparse, but I have two suggestions.

1. If sphinx-argparse can generate documentation from an existing object, you can obtain the already configured parsers from self._command_parsers.
2. Otherwise, you can set prog in the parsers manually when you define them.

@with_default_category('ISO7816 Commands')
class Iso7816Commands(CommandSet):
    # ...
    change_chv_parser = argparse.ArgumentParser(prog='change_chv')

[laf0rge]

Before 2.5.0, command parsers were created when a CLI class was defined. Each parser was a class member and configured with _set_parser_prog() upon creation.

excellent.

As of 2.5.0, command parsers are configured when a CLI object is instantiated. We make deep copies of the class member parsers, configure them with _set_parser_prog(), and add them to self._command_parsers. The class member parsers aren't ever updated with the new prog value. Only the copies are.

that is sad, as this causes a regression and breaks the previously perfectly working use of sphinx-argparse together with cmd2. What is the specific rationale of no longer updating the class member parsers prog attribute?

I don't know anything about sphinx-argparse,

I think it is the perfect match for generating reference documentation for applications creating CLI interfaces using cmd2.

Look at https://downloads.osmocom.org/docs/pysim/master/html/shell.html#pysim-commands where we render a manual for an application with dozens and dozens of cmd2 commands using argparse. The input can be seen at https://gitea.osmocom.org/sim-card/pysim/src/branch/master/docs/shell.rst where we simply put blocks like

open_channel
~~~~~~~~~~~~
.. argparse::
   :module: pySim-shell
   :func: Iso7816Commands.open_chan_parser

and sphinx-argparse then renders the command reference from that argparse instance.

If there is another similarly powerful way of creating command line reference manuals for cmd2+argparse using programs, I'm happy to hear about them.

but I have two suggestions.

1. If `sphinx-argparse` can generate documentation from an existing object, you can obtain the already configured parsers from `self._command_parsers`.

sphinx-argparse has no knowledge of cmd2. It is primarily used for command line programs hat use a single argparse instance for parsing the command line. All it knows is argparse, and how to extract the relevant bits to generate documentation from it. I think it's architecturally wrong to make sphinx-argparse understand about cmd2 - something it doesn't even depend upon.

2. Otherwise, you can set `prog` in the parsers manually when you define them.

@with_default_category('ISO7816 Commands') class Iso7816Commands(CommandSet): # ... change_chv_parser = argparse.ArgumentParser(prog='change_chv')

This would be super bad programming style, as it means the name of each command has to be specified twice:

• in the argparse.ArgumentParse constructor as prog argument, as you state
• in the name of the method that is annotated with the @cmd2.with_argparser decorator

Duplicating that information is

• a lot of effort, boilerplate code
• leads to future inconsistencies when a command is renamed but only one of the two instances is adjusted.

In my point of view, software/libraries should not force the programmer to be more verbose and repeat the same information several times; software should be smart enough to do the right thing.

To me, this bug is clearly a regression of cmd2. This used to work perfectly fine for many years, until cmd2 2.5.0 was released.

[tleonhardt]

@fixeria @laf0rge @kmvanbrunt So we find ourselves in an unpleasant situation here and I'm open to suggestions for how we can best move forward in a manner which results in the least overall pain.

We made a change somewhere in the 2.5.x line of releases which we thought wouldn't impact anyone in a backwards incompatible way. It turns out we were wrong in that assumption. if we had it to do over again, we would we would have either not made it or put out a 3.0 release to indicate it was backwards incompatible.

Now we are 6+ months down the road and have other users now relying on this backwards incompatible behavior, so we can't simply roll it back.

Ideally I would like to find a way to do something like add an additional boolean argument to the cmd2.Cmd.__init__() method which would default to the current behavior but allow optional apparent backwards compatibility. We would likely mark and document this parameter as something which exists temporarily and will be removed in the 3.0 release.

Please let me know what you think. And if any of you have a good idea for an implementation along these lines, please consider yourselves cordially invited to submit a PR.

[tleonhardt]

Actually, from chatting with @kmvanbrunt , the fix I suggested above won't help because the issue deals with class objects and not instance objects.

He can comment in more detail later, but my understanding is he can restore the old behavior where the decorator updates parser.prog when a parser is passed in versus a parser creation function, but this functionality won't be a part of a future 3.0 release.

This approach should restore your legacy behavior for 2.x releases.

[laf0rge]

Hi @tleonhardt. Sorry, I can't really comment on suggested fixes as I know too little about cmd2 internals and do not really understand why exactly and for what functionality the change was introduced. All I know is that we need a way to revert back to the old behaviour in some way for any future versions of cmd2, as otherwise we'd need to pin an ancient old unmaintained version or start to use a custom fork of cmd2, which both are rather undesirable.

I don't mind if we need to add an argument to an API call, or if there's a different decorator to use, or whatever other option to revert back to the old behaviour where the pased-in argparse instance gets patched again. If eventually an updated cmd2 with this existed, we could either pin to such a new minimum version/release.

[kmvanbrunt]

@fixeria @laf0rge

For the sake of backward compatibility, I will restore the _set_parser_prog() call in the with_argparser decorator. However, this change will not be carried over to cmd2 3.0 since it adds a redundant call to _set_parser_prog().

The sphinx-argparse scenario presented in this issue only works with cmd2 when all parsers are fully defined before the application starts. But the moment you incorporate the as_subcommand_to decorator or pass a factory method to with_argparser, the full structure of a parser only exists at runtime. Our baseline needs to account for all these scenarios and the simplest way to do that is by calling _set_parser_prog() when a parser is fully instantiated at runtime.

So after cmd2 3.0 is released, you will need to manually set prog, just like you'd have to do in any other argparse-based application. Since you're worried about the "future inconsistencies" this approach might lead to, write a decorator which wraps with_argparser and calls _set_parser_prog().

[tleonhardt]

@fixeria @laf0rge cmd2 2.6.2 is now available on PyPI. It should have fixed this issue.

[laf0rge]

Thanks a lot for the updated release and for notifying us. We're currently a bit overloaded but will definitely try to use 2.6.2 and if positive update our requirement to require >= 2.6.2 but < 3.0 [as a safeguard].

[osmith42]

I can confirm that the fix works with pysim code mentioned on top of the issue. Thank you very much!