optparse to argparse management commands

[jayvdb]

A lot of Django 1 projects are still usable except for this needed fix.

[pakal]

Are you well talking about this ? https://github.com/django/django/commit/6a70cb53971a19f2d9e71d5ee24bfb0e844b4d9d

I'll have a look at adding a fixer for this then (https://docs.djangoproject.com/en/3.0/releases/1.10/#features-removed-in-1-10)

[jayvdb]

Yup.

The usual breaking point is code like self.option_list = self.option_list + [...] in the old command class, which breaks because these attributes were removed.

[pakal]

Hm this one is far from trivial, since https://github.com/django/django/commit/6a70cb53971a19f2d9e71d5ee24bfb0e844b4d9d#diff-dfc45ab8548a0777543d12d6e77c9173L210 has impacts inside lots of functions - and patching the whole function would lead to regression when django gets updated.

I'll think about a custom magical "parser.parse_args()" maybe, which would fallback to cmd.option_list and to optparse while still having the same signature as argparse.

[pakal]

I've added most "trivial" fixers for Django3.0, and restored support for pre-Django1.10 support of Optparse in management commands.

All Tox tests seem to pass, but would you mind checking if the development branch below works properly for your own projects, thanks to these fixers ? https://github.com/pakal/django-compat-patcher/tree/development

[jayvdb]

will do today!

[pakal]

(on second thoughts, CI tests fail, there seems to be an issue in some cases of Optparse, i'm checking)

[pakal]

OK back to green - https://travis-ci.com/github/pakal/django-compat-patcher/builds/173328414

[jayvdb]

I've run it on some very heavy Django 3.0 projects, with thousands of smoke & unit tests, and no problems.

Now trying to use some optparse based apps.

[jayvdb]

Some testing done on Python 3/Django 3 with commands from

• unclebob (with modernize)
• django-nose
• django-test-utils (with modernize)
• librarywatch (fails due to pip version, but otherwise looks good)
• django-cli-query

[jayvdb]

https://github.com/semente/django-smuggler is the first case where it looks like the option monkey patching isnt working correctly

 Internal Server Error: /admin/dump/
Traceback (most recent call last):
  File "/usr/lib/python3.8/site-packages/django_compat_patcher/fixers/django1_10.py", line 438, in call_command
    return original_call_command(name, *args, **options)
  File "/usr/lib/python3.8/site-packages/django/core/management/__init__.py", line 118, in call_command
    for s_opt in parser._actions if s_opt.option_strings
AttributeError: 'OptionParser' object has no attribute '_actions'

(smuggler isnt the problem; I must have had another app overriding dumpdata, and that unidentified app is the cause)

This one also looks options related

  File "/usr/lib/python3.8/site-packages/fast_fixtures/management/commands/dumpdata.py", line 21, in handle
    return super(Command, self).handle(*app_labels, **options)
  File "/usr/lib/python3.8/site-packages/django/core/management/commands/dumpdata.py", line 70, in handle
    using = options['database']
KeyError: 'database'

  File "/usr/lib/python3.8/site-packages/uploadstatic/management/commands/uploadstatic.py", line 7, in <module>
    from django.core.management.base import CommandError, NoArgsCommand
ImportError: cannot import name 'NoArgsCommand' from 'django.core.management.base' (/usr/lib/python3.8/site-packages/django/core/management/base.py)

  File "/usr/lib/python3.8/site-packages/django/core/management/__init__.py", line 395, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/lib/python3.8/site-packages/modeltree/management/commands/modeltree.py", line 60, in run_from_argv
    options, args = parser.parse_args(argv[2:])
TypeError: cannot unpack non-iterable Namespace object

  File "/usr/lib/python3.8/site-packages/djangular/management/commands/makeangularsite.py", line 26, in handle
    super(Command, self).handle('site', name=site_name, target=site_name, **options)
TypeError: handle() got multiple values for keyword argument 'name'

dbbackups python3 ./manage.py listbackups

  File "./manage.py", line 47, in main
    execute_from_command_line(sys.argv)
  File "/usr/lib/python3.8/site-packages/django/core/management/__init__.py", line 401, in execute_from_command_line
    utility.execute()
  File "/usr/lib/python3.8/site-packages/django/core/management/__init__.py", line 395, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/lib/python3.8/site-packages/django_compat_patcher/fixers/django1_10.py", line 409, in run_from_argv
    parser = self.create_parser(argv[0], argv[1])
  File "/usr/lib/python3.8/site-packages/django_compat_patcher/fixers/django1_10.py", line 381, in create_parser
    parser.add_option(opt)
  File "/usr/lib64/python3.8/optparse.py", line 1004, in add_option
    raise TypeError("not an Option instance: %r" % option)
TypeError: not all arguments converted during string formatting

  File "/usr/lib/python3.8/site-packages/django_compat_patcher/fixers/django1_10.py", line 409, in run_from_argv
    parser = self.create_parser(argv[0], argv[1])
  File "/usr/lib/python3.8/site-packages/admin_steroids/management/commands/delete_duplicate_record.py", line 67, in create_parser
    parser.add_argument('name')
AttributeError: 'OptionParser' object has no attribute 'add_argument'

  File "/usr/lib/python3.8/site-packages/django_pdb/management/commands/test.py", line 39, in add_arguments
    parser.add_argument(name, **kwargs)
  File "/usr/lib64/python3.8/argparse.py", line 1386, in add_argument
    return self._add_action(action)
  File "/usr/lib64/python3.8/argparse.py", line 1749, in _add_action
    self._optionals._add_action(action)
  File "/usr/lib64/python3.8/argparse.py", line 1590, in _add_action
    action = super(_ArgumentGroup, self)._add_action(action)
  File "/usr/lib64/python3.8/argparse.py", line 1400, in _add_action
    self._check_conflict(action)
  File "/usr/lib64/python3.8/argparse.py", line 1539, in _check_conflict
    conflict_handler(action, confl_optionals)
  File "/usr/lib64/python3.8/argparse.py", line 1548, in _handle_conflict_error
    raise ArgumentError(action, message % conflict_string)
argparse.ArgumentError: argument --pdb: conflicting option string: --pdb

[jayvdb]

I have done rudimentary testing against the following (obviously some are guarded with if to support optparse and argparse)

> grep option_list /usr/lib/python3.8/site-packages/*/management/commands/*.py
admin_steroids/management/commands/delete_duplicate_record.py:    option_list = getattr(BaseCommand, 'option_list', ()) + tuple(get_options())
autofixture/management/commands/loadtestdata.py:            self.option_list = BaseCommand.option_list + params
celerymon/management/commands/celerymon.py:    option_list = CeleryCommand.option_list + monitor.get_options()
chroniker/management/commands/calculate_job_chain.py:    option_list = getattr(BaseCommand, 'option_list',
chroniker/management/commands/check_monitor.py:    option_list = getattr(BaseCommand, 'option_list', ()) + (
chroniker/management/commands/cron.py:    option_list = getattr(BaseCommand, 'option_list', ()) + (
chroniker/management/commands/run_job.py:    option_list = getattr(BaseCommand, 'option_list', ()) + (
chroniker/management/commands/test_status_update.py:    option_list = getattr(BaseCommand, 'option_list',
cities/management/commands/cities.py:        option_list = getattr(BaseCommand, 'option_list', ()) + (
cli_query/management/commands/query.py:    option_list = BaseCommand.option_list + (
data_migration/management/commands/migrate_legacy_data.py:    option_list = BaseCommand.option_list + (
data_migration/management/commands/migrate_this_shit.py:    option_list = BaseCommand.option_list + (
dbbackup/management/commands/_base.py:    base_option_list = (
dbbackup/management/commands/_base.py:    option_list = ()
dbbackup/management/commands/_base.py:        self.option_list = self.base_option_list + self.option_list
dbbackup/management/commands/_base.py:                             for _args, _kwargs in self.option_list])
dbbackup/management/commands/_base.py:            self.option_list = options + BaseCommand.option_list
dbbackup/management/commands/_base.py:        for args, kwargs in self.option_list:
dbbackup/management/commands/dbbackup.py:    option_list = BaseDbBackupCommand.option_list + (
dbbackup/management/commands/dbrestore.py:    option_list = BaseDbBackupCommand.option_list + (
dbbackup/management/commands/listbackups.py:    option_list = (
dbbackup/management/commands/mediabackup.py:    option_list = BaseDbBackupCommand.option_list + (
dbbackup/management/commands/mediarestore.py:    option_list = (
dbsnapshot/management/commands/test_client.py:    option_list = BaseCommand.option_list + (
debug_logging/management/commands/log_urls.py:    option_list = BaseCommand.option_list + (
devserver/management/commands/runserver.py:    option_list = BaseCommand.option_list + (
devserver/management/commands/runserver.py:        option_list += make_option(
djangobower/management/commands/bower_install.py:    option_list = BaseBowerCommand.option_list + (
django_fsm/management/commands/graph_transitions.py:        option_list = BaseCommand.option_list + (
django_nose/management/commands/test.py:    option_list = getattr(Command, 'option_list', ()) + tuple(extra_options)
django_pdb/management/commands/runserver.py:        # option_list is depecated since django 1.8 because optparse
django_pdb/management/commands/runserver.py:        option_list = RunServerCommand.option_list + tuple(
django_pdb/management/commands/test.py:        # option_list is depecated since django 1.8 because optparse
django_pdb/management/commands/test.py:        Command.option_list += type(Command.option_list)([
django_quicky/management/commands/clear_sessions.py:    option_list = BaseCommand.option_list + (
djangoredisshell/management/commands/redisshell.py:    option_list = BaseCommand.option_list + (
django_reset/management/commands/reset.py:    option_list = AppCommand.option_list + (
django_reset/management/commands/sqlreset.py:    option_list = AppCommand.option_list + (
django_runner/management/commands/runner.py:    option_list = BaseCommand.option_list + (
django_scripts_tracker/management/commands/check_scripts.py:    if hasattr(BaseCommand, 'option_list'):
django_scripts_tracker/management/commands/check_scripts.py:        option_list = BaseCommand.option_list + (
django_scripts_tracker/management/commands/run_all_scripts.py:    if hasattr(BaseCommand, 'option_list'):
django_scripts_tracker/management/commands/run_all_scripts.py:        option_list = BaseCommand.option_list + (
django_seed/management/commands/seed.py:    option_list = [
django_tenants/management/commands/all_tenants_command.py:        Changes the option_list to use the options from the wrapped command.
django_tenants/management/commands/__init__.py:        Sets option_list and help dynamically.
django_tenants/management/commands/tenant_command.py:        Changes the option_list to use the options from the wrapped command.
django_testmigrate/management/commands/testmigrate.py:option_list = [
django_testmigrate/management/commands/testmigrate.py:            for args, kwargs in option_list
django_urls_map/management/commands/urlmap.py:    option_list = BaseCommand.option_list + (
djangular/management/commands/makeangularsite.py:    option_list = base.BaseCommand.option_list
djangular/management/commands/testjs.py:    option_list = mgmt.base.BaseCommand.option_list + (
djcelery/management/commands/djcelerymon.py:    options = (runserver.Command.option_list +
eadred/management/commands/generatedata.py:    option_list = BaseCommand.option_list + (
fast_fixtures/management/commands/dumpdata.py:    option_list = DumpCommand.option_list + (
fixturemigration/management/commands/make_fixturemigration.py:    if hasattr(MigrationCommand, 'option_list'):
fixturemigration/management/commands/make_fixturemigration.py:        option_list = MigrationCommand.option_list + (
graceful_session_cleanup/management/commands/graceful_session_cleanup.py:    option_list = BaseCommand.option_list + (
helpdesk/management/commands/create_escalation_exclusions.py:        self.option_list += (
helpdesk/management/commands/create_queue_permissions.py:        self.option_list += (
helpdesk/management/commands/escalate_tickets.py:        self.option_list = (
mmc/management/commands/mmc_cleanup.py:    option_list = BaseCommand.option_list + (
modeltranslation/management/commands/loaddata.py:        option_list = LoadDataCommand.option_list + (
modeltranslation/management/commands/sync_translation_fields.py:        option_list = BaseCommand.option_list + (
modeltree/management/commands/modeltree.py:                                  option_list=klass.option_list)
modeltree/management/commands/modeltree.py:        for opt in klass.option_list:
orchestra/management/commands/postupgradeorchestra.py:        self.option_list = BaseCommand.option_list + (
orchestra/management/commands/postupgradeorchestra.py:    option_list = BaseCommand.option_list
orchestra/management/commands/restartservices.py:    option_list = BaseCommand.option_list
orchestra/management/commands/setupcelery.py:        self.option_list = BaseCommand.option_list + (
orchestra/management/commands/setupcelery.py:    option_list = BaseCommand.option_list
orchestra/management/commands/setupnginx.py:        self.option_list = BaseCommand.option_list + (
orchestra/management/commands/setupnginx.py:    option_list = BaseCommand.option_list
orchestra/management/commands/setuppostfix.py:        self.option_list = BaseCommand.option_list + (
orchestra/management/commands/setuppostfix.py:    option_list = BaseCommand.option_list
orchestra/management/commands/setuppostgres.py:        self.option_list = BaseCommand.option_list + (
orchestra/management/commands/setuppostgres.py:    option_list = BaseCommand.option_list
orchestra/management/commands/startservices.py:        self.option_list = BaseCommand.option_list + tuple(
orchestra/management/commands/startservices.py:    option_list = BaseCommand.option_list
orchestra/management/commands/stopservices.py:    option_list = BaseCommand.option_list
orchestra/management/commands/upgradeorchestra.py:        self.option_list = BaseCommand.option_list + (
orchestra/management/commands/upgradeorchestra.py:    option_list = BaseCommand.option_list
oscar_mws/management/commands/mws_feed_results.py:    option_list = NoArgsCommand.option_list + (
oscar_mws/management/commands/mws_submit_product_feed.py:    option_list = NoArgsCommand.option_list + (
oscar_mws/management/commands/mws_switch_product_fulfillment.py:    option_list = NoArgsCommand.option_list + (
oscar_mws/management/commands/mws_update_products.py:    option_list = NoArgsCommand.option_list + (
remote_fixtures/management/commands/push_fixtures.py:    option_list = BaseCommand.option_list + (
rest_framework_fine_permissions/management/commands/fine_permissions_load.py:    option_list = BaseCommand.option_list + (
sitetree/management/commands/sitetreedump.py:    option_list = get_options()
sitetree/management/commands/sitetreeload.py:    option_list = get_options()
sitetree/management/commands/sitetree_resync_apps.py:    option_list = get_options()
storage_migration/management/commands/migrate_storage.py:    option_list = LabelCommand.option_list + (
tenant_schemas/management/commands/__init__.py:        Sets option_list and help dynamically.
test_utils/management/commands/crawlurls.py:    option_list = BaseCommand.option_list + (
test_utils/management/commands/makefixture.py:    option_list = BaseCommand.option_list + (
test_utils/management/commands/quicktest.py:    option_list = BaseCommand.option_list + (
test_utils/management/commands/relational_dumpdata.py:    option_list = BaseCommand.option_list + (
test_utils/management/commands/testmaker.py:    option_list = BaseCommand.option_list + (
test_utils/management/commands/testshell.py:    option_list = BaseCommand.option_list + (
test_without_migrations/management/commands/_base.py:        # So we only define option_list for Django 1.7
test_without_migrations/management/commands/_base.py:            self.option_list = super(CommandMixin, self).option_list + (
tower/management/commands/merge.py:    option_list = BaseCommand.option_list + (
trunserv/management/commands/trunserver.py:    option_list = BaseCommand.option_list + (
unclebob/management/commands/test.py:    option_list = test.Command.option_list + (
uploadstatic/management/commands/uploadstatic.py:    option_list = NoArgsCommand.option_list + (
watchman/management/commands/watchman.py:    if hasattr(BaseCommand, 'option_list'):
watchman/management/commands/watchman.py:        option_list = BaseCommand.option_list + _add_options(make_option)

I didnt add orchestra, django_tenants, modeltranslation, or oscar_mws to INSTALLED_APPS as they conflict with other more important apps in my playpen. orchestra and satchmo are two I would like to revive.

I wont bother trunserv or devserver as I know they fail for reasons beyond the reach of compat on Django 3. devserver is probably worth rescuing, but needs a real port. debug_logger is beyond saving.

I havent yet configured as it is a bit finiky, nor setup deps for djangobower, and I have a tower fork which I had partially working on Python/Django & celerymon working from master, but need to configure them.

[pakal]

Hm it's very weird, I've quite closely mimicked the compatibility code which was in Django istelf before they removed it, but here it seems that these libs mix optparse and argpars API calls... In this system, if cls.option_list is not empty, the code switches entirely to optparse mode.

The "AttributeError: 'OptionParser' object has no attribute '_actions'" doesn't seem to be the whole traceback, since this error is expected, and is caught by the fixer to fallback on optparse-related code (some other error must happen later though).

I'll be very busy for the upcoming weeks, if you have the oppotunity to investigate one of those we might find things related to older behaviours of django (before even their own compatibility shims), else i'll look at end of summer

[jayvdb]

No worries; I can submit patches for a few of these.

Is the development branch suitable for me to submit patches against? Or is it a rewrite scratch area for you, before you push final work to master?

[pakal]

Thanks, I've merged my devs into master, so PRs can now target master as usual :)

[jayvdb]

I believe the current solution fails for https://gitlab.com/chrisspen/django-admin-steroids/-/blob/master/admin_steroids/management/commands/loaddatanaturally.py#L50

class Command(BaseCommand):
    help = 'Installs the named fixture(s) in the database.'
    args = "fixture [fixture ...]"

    def add_arguments(self, parser):
        parser.add_argument('args', nargs='+')
        ...

    def handle(self, *fixture_labels, **options):
        ...

My dirty patch for fixing it is below, but it depends on inspect.signature, which I think would need a backport. I am a bit surprised I couldnt find existing use of inspect.

diff --git a/src/django_compat_patcher/fixers/django1_10.py b/src/django_compat_patcher/fixers/django1_10.py
index 48c3152..738bd59 100644
--- a/src/django_compat_patcher/fixers/django1_10.py
+++ b/src/django_compat_patcher/fixers/django1_10.py
@@ -1,5 +1,6 @@
 from __future__ import absolute_import, print_function, unicode_literals

+from inspect import Parameter, signature
 from functools import partial

 from django.core.exceptions import ImproperlyConfigured
@@ -381,9 +382,16 @@ def fix_behaviour_core_management_parser_optparse(utils):
                 parser.add_option(opt)
         else:
             parser = original_create_parser(self, prog_name, subcommand)
-            if self.args:
+
+            sig = signature(self.handle)
+            var_args = [
+                arg.name for arg in sig.parameters.values()
+                if arg.kind == Parameter.VAR_POSITIONAL
+            ]
+
+            if var_args:
                 # Keep compatibility and always accept positional arguments, like optparse when args is set
-                parser.add_argument('args', nargs='*')
+                parser.add_argument(var_args[0], nargs='*')
         return parser
     utils.inject_callable(BaseCommand, "create_parser", create_parser)

If you are happy with me adding a py27 backport of inspect.signature, I'll continue and get a PR up. But if you have a specific direction to take this, feel free to take over.

[jayvdb]

django-chroniker doesnt work with the current solution, because it prefills option_list, so use_argparse is false, and then switches to argparse if the django version is 1.10 or more. I cant see an easy way to weave between those- will think about it some more though. (The alternative would be to have a Parser class which can act like / delegate to argparse or optparse, and decides which mode to use only when it gets the first positive sign from the class about which is needed. Ouch)

Traceback (most recent call last):
  File "./manage.py", line 52, in <module>
    main()
  File "./manage.py", line 48, in main
    execute_from_command_line(sys.argv)
  File "/usr/lib/python3.8/site-packages/django/core/management/__init__.py", line 401, in execute_from_command_line
    utility.execute()
  File "/usr/lib/python3.8/site-packages/django/core/management/__init__.py", line 395, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "django_compat_patcher-master/django_compat_patcher/fixers/django1_10.py", line 409, in run_from_argv
    parser = self.create_parser(argv[0], argv[1])
  File "/usr/lib/python3.8/site-packages/chroniker/management/commands/cron.py", line 318, in create_parser
    parser.add_argument('--update_heartbeat',
AttributeError: 'OptionParser' object has no attribute 'add_argument'

See https://github.com/jayvdb/django-compat-patcher/commit/2526892c4079d02f96a304a8014f51a8a20f9dc8 for the horrible hacky way of solving this ;-)

https://gitlab.com/chrisspen/django-admin-steroids (same maintainer) has the same problem.

[pakal]

So the varargs fix above is for handle() methods which don't name their "var_args" variable as "args", right ? If so, why not use the old inspect.getargspec() method (pthon2&3), instead of relying on new Signature API?

It should be doable to add more magic to workaround projects that have their own compatibility shims, but we must ensure that this fixer doesn't break projects relying on newest features. Maybe DCP would need a distinction between safe and unsafe fixers, and only enable safe ones by default. Or we'd need a Github CI running test suites of lots of django projects with DCP enabled.

[jayvdb]

Using inspect.getfullargspec / inspect.getargspec is ok by me. I understand your rationale for that, and should be forward compatible for quite a while. I'll work on that next.

I think the "lots of CI for many django apps" approach will turn into a maintenance problem. Constantly changing dependencies means they break , and many times the problem wont be DCP.

I would prefer to have some extra voodoo which by default turns off some fixers for specific known bad scenarios, such as turning off optparse for the command runner when the command is in app "chroniker" or "admin_steroids", possibly with some version constraints , and allows users to add similar voodoo disabling logic.

Then this repo is only maintaining knowledge about a few apps with specific known problems, which someone bothered submitting a patch for, but otherwise the assumption is that all fixers are always safe to use.

[pakal]

The CI wouldn't be on whole projects, rather cloning and launching unit-tests on a bunch of well-behaved Django projects - unit-tests that are supposed to be always working, thanks to mockups and dependency-version pinning. But even just that is some work that I can't do right now ^^'

I don't see quite well how this automatic system would function - in a real Django project, there are lots of lib which each may have their own allowlists/blocklists of fixers ; so this would need a full config-merging strategy, and different special cases like this "django command" context. This could be some extra "DCP-autoconfig" package indeed, but I'm not sure that there is much need for this : for now this optparse/argparse fixer is the only "unsafe" fixer, so since it's documented in the Readme, this could be sufficient for users to act when needed.

For safety, I have disabled this optarse/argparse fixer in default DCP settings, and users can reactivate it by tweaking settings (environment variables are now supported!)

I'll be happy to merge improvements of this fixer if you have the opportunity to prepare some B-)

[pakal]

Having the fixer excluded by default should do the trick for now B-)