Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct implementation of pkgutil style namespace #215

Merged
merged 5 commits into from
Sep 7, 2018

Conversation

Gadgetoid
Copy link
Collaborator

See corresponding PR to luma.core - rm-hull/luma.core#145
Original issue and explanation - rm-hull/luma.core#144

Copy link
Collaborator

@thijstriemstra thijstriemstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merging this I'd like to see the luma.core adjustment merged and released first (rm-hull/luma.core#145).

@Gadgetoid
Copy link
Collaborator Author

Agreed- I expected somewhat more discussion around this since it's a change to packaging technique and may have quite a far-reaching impact and it seems to be a heck of a rabbit hole.

Incidentally the currently shipped luma.core-1.7.2-py2.py3-none-any.whl does not include the __init__.py in luma/ but when I install the whl with sudo pip install luma.core and run import luma.core Python finds the package happily:

>>> import luma.core
import luma.core # directory /usr/local/lib/python2.7/dist-packages/luma/core
# trying /usr/local/lib/python2.7/dist-packages/luma/core/__init__.arm-linux-gnueabihf.so
# trying /usr/local/lib/python2.7/dist-packages/luma/core/__init__.so
# trying /usr/local/lib/python2.7/dist-packages/luma/core/__init__module.so
# trying /usr/local/lib/python2.7/dist-packages/luma/core/__init__.py
# /usr/local/lib/python2.7/dist-packages/luma/core/__init__.pyc matches /usr/local/lib/python2.7/dist-packages/luma/core/__init__.py
import luma.core # precompiled from /usr/local/lib/python2.7/dist-packages/luma/core/__init__.pyc

However when I install locally using sudo python setup.py install it wont import.

I can only attribute this difference to the luma.core-1.7.2-py2.7.egg based install that setup.py install produces.

If I build the new luma.core (as of my changes) with python setup.py bdist_wheel and install the resulting .whl file with pip, then it works.

If I do the same for luma.oled then my luma directory in dist-packages looks like this:

pi@raspberrypi:~ $ ls /usr/local/lib/python2.7/dist-packages/luma/
core  __init__.py  __init__.pyc  oled

And both import luma.core and import luma.oled continue to work.

However if I then uninstall luma.oled and reinstall it using python setup.py install (with my fix) then luma.core will be broken. Fun!

Again, TLDR

  • Installing both luma.core and luma.oled from .whl files (whether local or via pip) = FINE
  • Installing both luma.core and luma.oled with sudo python setup.py install (and my fix) = FINE
  • Installing each package with a different one of the methods above = BREAKS

And:

Installing both packages from .whl files results in them both living in /usr/local/lib/python2.7/dist-packages/luma/

pi@raspberrypi:~ $ ls /usr/local/lib/python2.7/dist-packages/luma/
core  __init__.py  __init__.pyc  oled

But when I sudo pip uninstall luma.oled

pi@raspberrypi:~ $ ls /usr/local/lib/python2.7/dist-packages/luma/
core

facepalm

This is with pip 9.0.1. And apparently isn't a new problem: pypa/packaging.python.org#314

This seems to suggest that a namespace_packages is required, but that results in:

pi@raspberrypi:~/luma.oled $ python setup.py build
running build
running build_py
error: Namespace package problem: luma is a namespace package, but its
__init__.py does not call declare_namespace()! Please fix it.
(See the setuptools manual under "Namespace Packages" for details.)

So delving into this yields the oh-so-common-of-Python pattern for luma/__init__.py of:

# -*- coding: utf-8 -*-
# Copyright (c) 2014-18 Richard Hull and contributors
# See LICENSE.rst for details.

try:
    from pkg_resources import declare_namespace
    declare_namespace(__name__)
except ImportError:
    from pkgutil import extend_path
    __path__ = extend_path(__path__, __name__)

Which results in a /usr/local/lib/python2.7/dist-packages/luma/ of:

pi@raspberrypi:~/ $ ls /usr/local/lib/python2.7/dist-packages/luma/
core  oled

Dang, that surely wont work, right!??

Python 2.7.13 (default, Nov 24 2017, 17:33:09)
[GCC 6.3.0 20170516] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import luma.oled
>>> import luma.core
>>>

Wut!?

Okay let's mess with this: sudo pip uninstall luma.core ... sudo python setup.py install

Python 2.7.13 (default, Nov 24 2017, 17:33:09)
[GCC 6.3.0 20170516] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import luma.core
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named core
>>>

!?!?

>>> import luma.core
# trying /usr/local/lib/python2.7/dist-packages/luma/core.arm-linux-gnueabihf.so

# trying /usr/local/lib/python2.7/dist-packages/luma/core.so
# trying /usr/local/lib/python2.7/dist-packages/luma/coremodule.so
# trying /usr/local/lib/python2.7/dist-packages/luma/core.py
# trying /usr/local/lib/python2.7/dist-packages/luma/core.pyc
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named core

Hmm, okay, it never sees the luma.core-1.7.2-py2.7.egg because it stops looking after luma.

Summary

The working pattern seems to be to include namespace_packages=["luma"] and use an __init__.py that looks like:

# -*- coding: utf-8 -*-
# Copyright (c) 2014-18 Richard Hull and contributors
# See LICENSE.rst for details.

try:
    from pkg_resources import declare_namespace
    declare_namespace(__name__)
except ImportError:
    from pkgutil import extend_path
    __path__ = extend_path(__path__, __name__)

This prevents the namespace from being yanked away from installed packages when any namespaced package is uninstalled.

The key caveat of this solution- and it appears any solution involving namespacing- is that the packages must be installed in the same manner. You can't mix and match .whl installs with .egg installs.

Someone actually went to the trouble of enumerating the working install permutations here: pypa/packaging.python.org#265 (comment)

And seems to have a setup for testing this in the repository I was consulting beforehand: https://github.com/pypa/sample-namespace-packages

@thijstriemstra
Copy link
Collaborator

Agreed- I expected somewhat more discussion around this since it's a change to packaging technique and may have quite a far-reaching impact and it seems to be a heck of a rabbit hole.

thanks for pointing this out, i fully agree. could you also take a look at the other luma.* libraries and check if they need to be updated as well? once a new luma.core is released it's always a little bit of fingers crossed and hope the other libraries don't break (even though we have travis builds etc).

@rm-hull
Copy link
Owner

rm-hull commented Sep 4, 2018

I think we took the decision to fully support pip-installed versioned releases from PyPi only rather than someone arbitrarily installing from a git clone (it's hard enough supporting this library as it is). This might explain why it doesn't really hang together properly when installing with python setup.py install (at least thats what I assume).

Before merging this I'd like to see the luma.core adjustment merged and released first (rm-hull/luma.core#145).

Anyway, I just pushed luma.core v1.8.0 with the package fix.

@rm-hull
Copy link
Owner

rm-hull commented Sep 4, 2018

Also: I'm not familiar enough with the nuances of python packaging to know whether it will break, so let's just push it out and fix forward.

@Gadgetoid
Copy link
Collaborator Author

Honestly- despite combing through all the packaging documentation with a fine-toothed comb several times over, I'm still not sure I'm familiar enough with its nuisances.

I'm all for pushing it out and fixing forward, though. If I don't see them, please tag me on any issues which may be related.

@rm-hull rm-hull dismissed thijstriemstra’s stale review September 7, 2018 18:29

rm-hull/luma.core#145 was released, so review comments now satisfied

@rm-hull rm-hull merged commit 3358c0e into rm-hull:master Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants