sugarlabs / sugar-toolkit-gtk3

Sugar Learning Environment, Activity Toolkit, GTK 3.
GNU Lesser General Public License v2.1
21 stars 80 forks source link

Fix bundlebuilder not handling empty arguments #452

Closed shaansubbaiah closed 4 years ago

shaansubbaiah commented 4 years ago

If no arguments are passed, parser.parse_args() returns command = None

Traceback:

$ python2 ./setup.py
usage: ./setup.py [-h]
                  {install,check,dist_xo,dist_source,build,fix_manifest,genpot,dev}
                  ...
./setup.py: error: too few arguments

$ python3 ./setup.py
Traceback (most recent call last):
  File "./setup.py", line 5, in <module>
    bundlebuilder.start()
  File "/usr/lib/python3.7/dist-packages/sugar3/activity/bundlebuilder.py", line 637, in start
    globals()['cmd_' + options.command](config, options)
TypeError: can only concatenate str (not "NoneType") to str
shaansubbaiah commented 4 years ago

@quozl please take a look, for some reason there was no issue with Python2 (before changes in PR).

Tried:

import argparse

parser = argparse.ArgumentParser(prog='PROG')
subparsers = parser.add_subparsers(
        dest="command", help="Options")

option = parser.parse_args([''])
print(option.command)

Output:

usage: PROG [-h] {} ...
PROG: error: argument command: invalid choice: '' (choose from )
quozl commented 4 years ago

Using pdb with the Python 2 Sugar Toolkit, the call to parser.parse_args does print usage.

(Pdb) next
> /usr/lib/python2.7/dist-packages/sugar3/activity/bundlebuilder.py(623)start()
-> options = parser.parse_args()
(Pdb) next
usage: ./setup.py [-h]
                  {install,check,dist_xo,dist_source,build,fix_manifest,genpot,dev}
                  ...
./setup.py: error: too few arguments
SystemExit: SystemExit(2,)
> /usr/lib/python2.7/dist-packages/sugar3/activity/bundlebuilder.py(623)start()
-> options = parser.parse_args()
(Pdb) quit

Now with Python 3, options.command is None, and so raised TypeError.

Adding TypeError to the exception handler is a simple fix.

Also, when command is unknown, given that Python 2 and Python 3 raises KeyError;

(Pdb) globals()['cmd_'+'test']()
*** KeyError: 'cmd_test'
(Pdb) 

It may be that IndexError is no longer required.

However, this existing design of using an exception handler hides any of these exceptions if they occur inside the called command functions. A fix to that is to check options.command for None before using it, and to check the key is in globals() before calling it.

shaansubbaiah commented 4 years ago

@quozl added the check if key in globals().

Now,

  1. Typing no key python3 ./setup.py : Prints help
  2. Typing a valid key python3 ./setup.py dev : Runs the associated function
  3. Typing a wrong key python3 ./setup.py thisDoesNotExist : prints that command is invalid and the help message. (Done by argparse)