Code

Opened 4 years ago

Last modified 7 months ago

#14087 new Bug

django.core.management.get_commands only sees commands in the last package of a namespace package

Reported by: KyleMac Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: joh, DavidReynolds, lnielsen@…, bhuztez, alex@…, daevaorn@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If using the namespace features of setuptools, then get_commands will only see the commands in the last package. If you have something like the following:

company.app1.management.commands.command1
company.app2.management.commands.command2

then Django will only spot command2. I don't know what the proper fix would be, but I use the following to fill in the gaps:

def find_namespace_commands():
    try:
        from django.conf import settings
        apps = settings.INSTALLED_APPS
    except (AttributeError, EnvironmentError, ImportError):
        apps = []
    import imp
    from django.core.management import _commands, find_commands
    from django.utils.importlib import import_module
    for app in apps:
        try:
            app_path = import_module(app).__path__
        except AttributeError:
            continue
        try:
            imp.find_module('management', app_path)
        except ImportError:
            continue
        mod = import_module('%s.management' % app)
        if hasattr(mod, '__path__'):
            for cmd in find_commands(mod.__path__[0]):
                if _commands is None:
                    _commands = {}
                if cmd not in _commands:
                    _commands[cmd] = app


find_namespace_commands()

Attachments (14)

patch_14087_manage.diff (3.9 KB) - added by joh 3 years ago.
Solution which tries to stay with old API
patch_14087_manage_trunk.diff (3.7 KB) - added by joh 3 years ago.
Some changes in the API
namespaced_managment_commands.diff (10.7 KB) - added by nfg 3 years ago.
find_managment_module with support for namespaced packages, with tests.
namespace_package_pth.diff (16.9 KB) - added by bhuztez 3 years ago.
a dirty fix for pth
eggs_broken.diff (18.8 KB) - added by bhuztez 3 years ago.
Zip-archived eggs are broken
zip_egg_fixed.diff (22.7 KB) - added by bhuztez 2 years ago.
fixed for zip-archived eggs
zip_egg_fixed.2.diff (21.3 KB) - added by bhuztez 2 years ago.
improved fix for zip-archived eggs
zip_egg_fixed.3.diff (22.3 KB) - added by bhuztez 2 years ago.
if management has already been imported and is not a package
zip_egg_fixed.4.diff (22.3 KB) - added by bhuztez 2 years ago.
update patch for revision 17517
namespace_package_pth.2.diff (15.6 KB) - added by bhuztez 2 years ago.
update patch for revision 17517
namespace_package_pth.3.diff (16.6 KB) - added by bhuztez 2 years ago.
add test from zip_egg_fixed.3.diff
namespace_package_pth.4.diff (16.6 KB) - added by bhuztez 2 years ago.
update patch for revision 17777
zip_egg_fixed.5.diff (22.3 KB) - added by bhuztez 2 years ago.
update patch for revision 17777
namespace_package_pth.5.diff (16.6 KB) - added by bhuztez 2 years ago.
remove trailing whitespaces

Download all attachments as: .zip

Change History (43)

comment:1 Changed 3 years ago by joh

  • Cc joh added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by joh

I just ran into the same trouble. After some research it seems to be a problem which is not so easy to solve as long as one wants to avoid importing the modules. In source:django/trunk/django/core/management/__init__.py@13489#L31 the code is using imp to find the module. I think this is not suitable, because the variable __path__ of the modules is not respected when traversing the tree.

It would be easy to solve if find_module would return all found modules. May be that it can be simulated by calling find_module for each element of a list of paths and aggregating the results into a tuple. I will try this out and add my results to this ticket.

Changed 3 years ago by joh

Solution which tries to stay with old API

Changed 3 years ago by joh

Some changes in the API

comment:3 Changed 3 years ago by joh

  • Has patch set

I just added two patches which seem to fix this problem:

  • patch_14087_manage.diff includes some if-statements which shall assure that the old API still will work in case the methods are used elsewhere. I marked two places by a "TODO:" because I think there should be some mechanism added to issue a deprecation warning, so that this could be cleaned up in the future.
  • patch_14087_manage_trunk.diff does not include these if-statements

comment:4 Changed 3 years ago by russellm

  • Resolution set to wontfix
  • Status changed from new to closed

The problem with this is that it will mask a bunch of other problems. Django uses the application name as a namespace, and having two applications with the same name will cause problems. This is something that needs a much higher level workaround; it may be addressed by the #3591 app-refactor that was handled as part of the 2010 GSoC.

comment:5 Changed 3 years ago by russellm

  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Ready for checkin

This discussion on django-dev points out that the problem is a bit deeper that my original analysis concluded. This github branch has what appears to be an RFC patch.

The patch needs a final review to make sure that there aren't any dragons hidden in changing the module lookup code.

Changed 3 years ago by nfg

find_managment_module with support for namespaced packages, with tests.

comment:6 Changed 3 years ago by DavidReynolds

  • Cc DavidReynolds added

comment:7 Changed 3 years ago by lnielsen@…

  • Cc lnielsen@… added

Hi,

I've tested out the namespaced_managment_commands.diff patch and it works for me when my namespace packages are installed in develop mode or as eggs (with either easy_install or a python setup.py install), however if install the package with pip then pip automatically removes the namespacepackage/init.py file and above method no longer works.

Cheers,
Lars

Output from installing my package:

(virtualenv) <package>$ pip install .
Unpacking .../<pacakge>
  Running setup.py egg_info for package from file:///.../<pacakge>
    
    no previously-included directories found matching 'docs/build'
    warning: no files found matching '*.css' under directory 'src'
    warning: no files found matching '*.js' under directory 'src'
Installing collected packages: <pacakge>
  Running setup.py install for <pacakge>
    
    no previously-included directories found matching 'docs/build'
    warning: no files found matching '*.css' under directory 'src'
    warning: no files found matching '*.js' under directory 'src'
    Skipping installation of ../virtualenv/lib/python2.7/site-packages/<pacakge>/__init__.py (namespace package)   <-------
    Installing ...virtualenv/lib/python2.7/site-packages/<pacakge>-0.1.0-py2.7-nspkg.pth
Successfully installed <pacakge>
Cleaning up...

comment:8 Changed 3 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

When I looked at this patch 2 nights ago, I was afraid that there would be a problem (either with eggs, or with the namespace packaging usage), but I hadn't got as far as confirming the problem. Thanks to Lars for finding the case where it all falls apart.

comment:9 Changed 3 years ago by anonymous

Hi,

I did a bit more investigation why the init file is being skipped. The skipping of the init file happens when you use the "--single-version-externally-managed" option during install (e.g. python setup.py install --single-version-externally-managed). Pip automatically adds this option by default and as far as I can read it's also used by system package builders (like e.g. Debian or rpms). What setuptools does with this option is to add a .pth file for each part of a namespace module - e.g.:

site-packages/
  <namespace package>_app1_<version>-nspkg.pth
  <namespace package>_app2_<version>-nspkg.pth
...
  <namespace package>/app1
  <namespace package>/app2

The pth files contains a bit of python which will be loaded by the site module when you start python. This piece of code in the pth file will add <namespace pacakge> as a builtin module and set the modules path variable. So while find_module will not be able to locate the module, the module will already be loaded and available in sys.modules.

I haven't gotten around to fix it yet :-)

Cheers,
Lars

Last edited 2 years ago by ramiro (previous) (diff)

comment:10 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

Changed 3 years ago by bhuztez

a dirty fix for pth

Changed 3 years ago by bhuztez

Zip-archived eggs are broken

comment:11 Changed 3 years ago by bhuztez

  • Easy pickings unset
  • UI/UX unset

I tried to improve, but could not get it work with zip-archived eggs.

comment:12 Changed 3 years ago by bhuztez

  • Cc bhuztez added

Changed 2 years ago by bhuztez

fixed for zip-archived eggs

Changed 2 years ago by bhuztez

improved fix for zip-archived eggs

comment:13 Changed 2 years ago by bhuztez

  • Needs documentation set

with patch zip_egg_fixed.2.diff, django will be able to find all commands in a package, if importer for that package implements interfaces explained below and is not in sys.meta_path.

  • importer.iter_modules(prefix='')

importer.iter_modules is not defined in PEP 302, however pkgutil.ImpImporter does implement this method. Django would use this interface to list all modules in the commands package.

  • loader.get_filename(fullname)

loader.get_filename is defined in PEP 302. But this patch use it to figure out search path of packages.

filename = loader.get_filename(fullname)
path_item = os.path.dirname(filename)

Just assume os.path.dirname would work out the search path.

  • loader.is_package(fullname)

This is the same as loader.is_package defined in PEP 302.

Changed 2 years ago by bhuztez

if management has already been imported and is not a package

Changed 2 years ago by bhuztez

update patch for revision 17517

comment:14 Changed 2 years ago by bhuztez

update patch for revision 17517

Changed 2 years ago by bhuztez

update patch for revision 17517

comment:15 Changed 2 years ago by bhuztez

  • Needs documentation unset

namespace_package_pth.2.diff will not look up importer in sys.path_importer_cache, since django do not support PEP 302 importer.

should find commands in modules loaded by PEP 302 importer be in another ticket?

Changed 2 years ago by bhuztez

add test from zip_egg_fixed.3.diff

comment:16 follow-up: Changed 2 years ago by carljm

The zip_egg_fixed patch should also fix #12206.

I can't seem to find a separate bug for the management-commands-in-eggs issue, though I thought there was one. Yes, there should be a separate bug for that (or more broadly, PEP 302 importers). It's confusing to have two patches under development on a single ticket.

comment:17 in reply to: ↑ 16 Changed 2 years ago by bhuztez

I thought there was one.

They are #8280 and #13587. and add support PEP 302 importer should also help improve the error message of load_backend, which is mentioned in #8238.

It's confusing to have two patches under development on a single ticket.

the namespace_package_pth patch is cleaned up to address the problem of finding command in namespace packages only, so that it could be applied without further investigation. However, in practice, namespaced packages are often being zip archived. So, we need a general solution for PEP 302 importers.

Last edited 2 years ago by bhuztez (previous) (diff)

Changed 2 years ago by bhuztez

update patch for revision 17777

Changed 2 years ago by bhuztez

update patch for revision 17777

comment:18 Changed 2 years ago by bhuztez

update namespace_package_pth and zip_egg_fixed for revision 17777.

comment:19 Changed 2 years ago by bhuztez

  • Patch needs improvement unset
  • Version changed from 1.2 to SVN

I found #596 is a better place for patch zip_egg_fixed. I will move it there.

comment:20 Changed 2 years ago by alex@…

  • Cc alex@… added

Changed 2 years ago by bhuztez

remove trailing whitespaces

comment:21 Changed 2 years ago by bhuztez

the zip_egg_fixed patch is now maintained under #8280.

comment:22 Changed 22 months ago by bhuztez

my pull request for the namespace_package_pth patch: https://github.com/django/django/pull/178

comment:23 Changed 19 months ago by Natim

My patch for Python 1.4.1 : https://code.djangoproject.com/ticket/19048

comment:24 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

comment:25 Changed 12 months ago by DavidReynolds

bhuztez' pull request certainly addresses the problem I am seeing, any way we can move this forward?

comment:26 Changed 11 months ago by kmtracey

Alternate fix proposed in #20344.

comment:27 Changed 11 months ago by alexkoshelev

  • Cc daevaorn@… added

comment:29 Changed 7 months ago by timo

  • Patch needs improvement set

From apollo13 on the pull request:

As I noted on the previous PR (#866) I'd like to see:

* Support for PEP 420
* Check other locations in Django for the same issues (app template loaders etc come to mind)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.