Code

Opened 3 years ago

Closed 6 months ago

Last modified 6 months ago

#17304 closed Cleanup/optimization (fixed)

Document conditions under which a namespace package may or may not be an app

Reported by: rwmcfa1@… Owned by: carljm
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: jholloway7 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

when INSTALLED_APPS includes a module that does not have file attribute an AttributeError is raised. this occurs with built-in modules (piston being an example of a app with this problem.)

...

File "/home/rm/lib/example/api/env/lib/python2.7/site-packages/django/utils/translation/trans_real.py", line 163, in _fetch

apppath = os.path.join(os.path.dirname(app.file), 'locale')

AttributeError: 'module' object has no attribute 'file'

to reproduce create a django project with piston in INSTALLED_APPS and run ./manage.py syncdb

Attachments (1)

trans_real.patch (869 bytes) - added by rwmcfa1@… 3 years ago.
patch that addresses the problem by using hasattr

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by rwmcfa1@…

patch that addresses the problem by using hasattr

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

I also have a project that uses piston and syncdb works fine, so I tend to think that something is wrong with your setup.

If the bug you describe really exists, it means that piston doesn't work at all. Its authors would have noticed, and it would be a bug in piston, not in Django.

Python's docs say:

__file__ is the pathname of the file from which the module was loaded, if it was loaded from a file. The __file__ attribute is not present for C modules that are statically linked into the interpreter; for extension modules loaded dynamically from a shared library, it is the pathname of the shared library file.

Unless you've linked piston statically into the interpreter (I hope you haven't), something else is going on.

comment:2 Changed 3 years ago by anonymous

It also doesn't exist for modules created using new.module(), however I don't think that's a sufficiently common usecase to warrant this (we look for __file__ in many places).

comment:3 Changed 3 years ago by rwmcfa1@…

I'll check it out on another box or two in a bit, but piston has no file attribute when i install it with pip in a virtualenv:

animal$ cd /tmp/
animal$ virtualenv env
New python executable in env/bin/python
Installing distribute.................................................................................................................................................................................done.
animal$ source env/bin/activate
(env)animal$ pip install django-piston
Downloading/unpacking django-piston
  Downloading django-piston-0.2.3.tar.gz
  Running setup.py egg_info for package django-piston
    
Installing collected packages: django-piston
  Running setup.py install for django-piston
    
    Skipping installation of /tmp/env/lib/python2.7/site-packages/piston/__init__.py (namespace package)
    Installing /tmp/env/lib/python2.7/site-packages/django_piston-0.2.3-py2.7-nspkg.pth
Successfully installed django-piston
Cleaning up...
(env)animal$ python
Python 2.7.1+ (r271:86832, Apr 11 2011, 18:13:53) 
[GCC 4.5.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import piston
>>> piston.__file__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute '__file__'
>>>

I later realized there's other places file is being used on INSTALLED_APPS so this patch isn't sufficient (to fix the problem.)

best,
-rm

comment:4 Changed 3 years ago by rwmcfa1@…

piston doesn't seem to have an init.py file in it's top-level directory, causing it not to have a file attribute.

(env)animal$ python
Python 2.7.1+ (r271:86832, Apr 11 2011, 18:13:53) 
[GCC 4.5.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import piston
>>> piston
<module 'piston' (built-in)>

best,
-rm

comment:5 Changed 3 years ago by aaugustin

Yes, I confirm it's a issue with piston's packaging.

In my project, we add the __init__.py file after installing it.

comment:6 Changed 2 years ago by carljm

  • Resolution needsinfo deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Design decision needed

Technically, this is a bug in Django; Python modules are not required to have the __file__ attribute, so we shouldn't assume it is present.

Practically speaking, it's unclear how Django should handle an app that is itself a namespace package, given that that means the package could be spread across several different directories. Should Django look for "locale", "templates", "static" etc in all of those directories? Or just arbitrarily pick one? (e.g. module.__path__[0])? Or just document the fact that apps cannot be namespace packages, and provide a clearer error message?

I think I lean towards the latter option; I haven't seen any good use case for an app itself being a namespace package (as opposed to being a package _within_ a namespace package, which works fine), and I'm worried that allowing for that would open up a whole range of new problematic edge cases.

comment:7 Changed 2 years ago by rwmcfa1@…

django's use of file seemed to be relatively prevalent when i looked around (been a while now.) better error message would be fine as it would of saved me time digging and tracking down what was going on. downside would be that it would prevent the use of such packages when it doesn't need to make use of locale, template, static, ... i don't have any solid use cases that make me lean one way or another. it seemed like it would be some work to get to rework the various places relying on file.

comment:8 Changed 2 years ago by jag@…

To explain why the init.py isn't there, since that seems to be a point of confusion, there isn't a flaw with piston's packaging. When you use pip to install piston, because of its use of namespace packaging, pip actually ignores the __init__.py in the package and uses an nspkg pth file to handle the namespacing:

$ pip install django-piston
Downloading/unpacking django-piston
  Downloading django-piston-0.2.3.tar.gz
  Running setup.py egg_info for package django-piston
Installing collected packages: django-piston
  Running setup.py install for django-piston
    Skipping installation of $VIRTUAL_ENV/lib/python2.6/site-packages/piston/__init__.py (namespace package)
    Installing $VIRTUAL_ENV/lib/python2.6/site-packages/django_piston-0.2.3-py2.6-nspkg.pth
Successfully installed django-piston
Cleaning up...

This does not happen when installing using other methods. But as a consequence, when you try to access __file__ on the piston module, the attribute does not exist.

As carljm said, the bug is that Django should *not* be relying upon file being there for its i18n functions. I'll evaluate the backward-incompatible b0rkage that would happen by making piston not a namespaced package, but Django should also do its part to DTRT.

comment:9 Changed 2 years ago by jholloway7

  • Cc jholloway7 added

I don't think the problem is entirely the use of __file__, per se, but in particular this pattern of locating __file__ that seems problematic with namespace packages:

    if settings.SETTINGS_MODULE is not None:
        parts = settings.SETTINGS_MODULE.split('.')
        project = import_module(parts[0])
        projectpath = os.path.join(os.path.dirname(project.__file__), 'locale')

By using parts[0], it seems to be making an assumption that the project is always in a top-level package. This certainly wouldn't hold true for namespace packages, but seems like it would be fragile for other scenarios too (i.e. someone simply wants to nest a couple different projects under a common top-level package without using a true namespace package).

Perhaps it could walk backwards from the settings module until it finds the highest package that has a __file__ attribute and then look for the locale folder there.

Or, perhaps, one day "locale loaders" could be configurable/extensible like template loaders and folks that must leave the beaten path would be on their own to handle the loading.

comment:10 Changed 2 years ago by jholloway7

I just realized my problem is different, albeit somewhat related, to the original topic. In my case, it's happening higher up in the locale loading where I have my Django project under a namespace package, not the application(s). My comments may not totally apply to the original problem in this bug report.

comment:11 Changed 22 months ago by zllak

I confirm this issue. My django project is in a namespace package, and I face the same problem.
As said before, a fix could be to walk backwards from the settings module to find the locales.
Is there any progress on this issue ? Or an easy workaround ?
Because I can't use my django application, *AT* all.

comment:12 Changed 16 months ago by aaugustin

  • Status changed from reopened to new

comment:13 Changed 16 months ago by aaugustin

  • Component changed from Uncategorized to Documentation
  • Has patch unset
  • Triage Stage changed from Design decision needed to Accepted
  • Type changed from Bug to Cleanup/optimization

I agree with Carl -- given Django's pattern of looking for various files in well known locations within application, the best we can do is document applications cannot be namespace packages.

comment:14 Changed 10 months ago by wim@…

For piston users, I am referring to: https://bitbucket.org/jespern/django-piston/issue/173/attributeerror-module-object-has-no for some possible solutions: either use piston 0.2.2, use easy_install or manually add an __init__.py file

comment:15 Changed 10 months ago by timo

  • Summary changed from trans_real AttributeError __file__ to Document that apps cannot be namespace packages

comment:16 Changed 6 months ago by carljm

#21862 was a duplicate, and the comments there include some details about how the manifestation of this changed with the merge of the new app-loading stuff for 1.7.

Although this is labeled as just a documentation fix, I think we should also change the code to detect namespace packages at app-loading time (that is, len(app_module.__path__) > 1 and raise a clearer error. The current errors that you get as a result of doing this (in both 1.6 and 1.7/master) are extremely obscure.

comment:17 Changed 6 months ago by carljm

Actually, one nice side effect of coding the check as len(app_module.__path__) > 1 is that we would still support the naive case of someone just leaving out __init__.py in Python 3.3+. So technically namespace packages would be supported, as long as they don't actually have multiple locations (and thus there is no ambiguity about where to look for templates etc).

comment:18 Changed 6 months ago by aaugustin

Sure, you can put that in AppConfig.

Last edited 6 months ago by aaugustin (previous) (diff)

comment:19 Changed 6 months ago by carljm

  • Component changed from Documentation to Core (Other)
  • Has patch set
  • Owner changed from nobody to carljm
  • Status changed from new to assigned

Pull request for proposed fix: https://github.com/django/django/pull/2212

Review welcome. In particular, Aymeric, I noted that providing an explicit "path" class attribute on an AppConfig subclass was not previously documented (it's even documented as "read only" rather than "configurable"), although the app-loading code does go out of its way to avoid overwriting it if it is set on the class. Was that an oversight in the documentation, or intentional? The docs I add here sort of document it in-passing in the new section about namespace packages; if that's ok, we should probably move it up into the "configurable" section and document the effect of configuring it more clearly.

comment:20 Changed 6 months ago by aaugustin

It's intentional. I chose to make the implementation as powerful as possible, but to document only features with a clear use case.

Since you've found a use case for AppConfig.path, yes, you should move it to the "configurable" section.

I left a few suggestions on the pull request but I'm very happy with the overall approach.

comment:21 Changed 6 months ago by Carl Meyer <carl@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 966b186981d619d964152ebcda1bd844ec5f6c8c:

Fixed #17304 -- Allow single-path and configured-path namespace packages as apps.

Also document the conditions under which a namespace package may or may not be
a Django app, and raise a clearer error message in those cases where it may not
be.

Thanks Aymeric for review and consultation.

comment:22 Changed 6 months ago by carljm

  • Summary changed from Document that apps cannot be namespace packages to Document conditions under which a namespace package may or may not be an app

Changing ticket name to avoid confusion in the future - could give the impression that namespace packages cannot be apps.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.