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

#141 introduces regression in importing namespace packages #207

Open
bb-migration opened this Issue May 14, 2014 · 4 comments

Comments

Projects
None yet
1 participant
@bb-migration

bb-migration commented May 14, 2014

Originally reported by: embray (Bitbucket: embray, GitHub: embray)


Unfortunately the fix to #141 was basically my code :(

The regression is caused by the changes to pkg_resources._handle_ns. That change was added to support the scenario reported in #141 where the setup_requires package in question has a namespace package. For example:

#!python
setup_requires = ['foo.bar > 1.1']

where foo is a namespace package. The fix ensures that the path to the newer version of foo.bar is added earlier on foo.__path__ while still maintaining the original foo.__path__ entries in their original relative order.

Unfortunately this fix breaks the (likely more common case) where there are two copies of foo.bar on sys.path to begin with. For example, if upon starting the interpreter it has the entries:

#!python
sys.path = [
    ...
    'path/to/virtualenv/site-packages/foo.bar-1.1.egg/'
    ...
    'system/site-packages/foo.bar-1.0.egg/'
]

when one performs import foo, the first entry is found first resulting in

#!python
foo.__path__ == [ 'path/to/virtualenv/site-packages/foo.bar-1.1.egg/']

which is fine. But then as declare_namespace continues its march it gets to the later path entry which ends up reordering foo.__path__ so that

#!python
foo.__path__ == ['system/site-packages/foo.bar-1.0.egg/', 'path/to/virtualenv/site-packages/foo.bar-1.1.egg/']

Thus a call to import foo.bar ends up importing the old foo.bar later on the path.

If that's all confusing, I will attach a regression test in a bit. I think the solution is to handle the __path__ reordering in fixup_namespace_packages which is where it is actually necessary, and not in _handle_ns generally.


@bb-migration

This comment has been minimized.

bb-migration commented May 14, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Thanks Erik for taking the lead on this; it's most appreciated.

@bb-migration

This comment has been minimized.

bb-migration commented May 15, 2014

Original comment by embray (Bitbucket: embray, GitHub: embray):


Namespace packages are a great idea but kind of silly to get just right considering the history surrounding them :)

@bb-migration

This comment has been minimized.

bb-migration commented May 15, 2014

Original comment by embray (Bitbucket: embray, GitHub: embray):


This first change moves some things around in test_easy_install to support a new test "test_setup_requires_override_nspkg" which specifically tests the use case my original changes to _handle_ns was meant to support. This test should pass currently. It's a little ugly but gets the job done.

@bb-migration

This comment has been minimized.

bb-migration commented Jan 6, 2016

Original comment by embray (Bitbucket: embray, GitHub: embray):


Encountered this lovely issue again for the first time in a while, when the software stack used by many of my coworkers upgraded setuptools for the first time in a long time.

The fix to #141 handled namespace packages in such a way that whenever a new distribution is activated, if it contains a member of a namespace package, it inserts that package's path first into the namespace package's __path__. This allows a dependency to be "upgraded" by way of setup_requires.

However, this is at odds the normal behavior, where the first time some member of a namespace package is imported, declare_namespace is called and each sys.path entry is searched for members of the namespace package. Those paths are then added to the namespace package's __path__ in the order they're found on sys.path. Because of the fix in #141, this results in the __path__ being in the reverse order of sys.path.

I have a potential fix for this that ensures that a namespace package's __path__ is always ordered according to sys.path. This also required changing how new eggs are activated on sys.path, by requiring them to be inserted at the beginning of sys.path, rather than appending them to the end. This seems to make more sense in general, I think. But I'm not 100% sure what the impact of this change is overall.

jaraco pushed a commit that referenced this issue Apr 4, 2016

Adds the regression test for distribute issue 323 that I attached to #…
…207.  This is to ensure that any fix to #207 does not introduce another regression.

jaraco added a commit that referenced this issue Apr 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment