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

fix namespace packages handling of wheels #1215

Merged

Conversation

benoit-pierre
Copy link
Member

I'd like some feedback on this, not use if there's a better approach...

Fix #1214.

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Welcome to the wonderful challenges of namespace packages, challenges which always seem to push the limits of our abilities to manage. It looks like once again, we're caught between the design disparity between namespace packages as implemented by pip versus how they were implemented by setuptools/pkg_resources.

I'd like to harmonize the behavior as much as possible, such that if Setuptools were to use pip rather than its own installer, the technique will continue to work. For that reason, I'd like to avoid injecting package modules.

When I think about why these packages install under pip, they work because setuptools creates the -nspkg.pth file as part of the install, and that .pth file is executed implicitly when the interpreter starts up.

In the case of easy_install installing wheels, I think it should follow the same model as pip and ensure those .pth files are also installed. That will cause those namespace packages to work properly in subsequent interpreter invocations.

I note that .pth files are only invoked implicitly when found in the site-packages directory, which is why tools like rwt need additional handling to ensure those pth files are executed.

For packages that are installed for this interpreter invocation (such as setup_requires or tests_require might expect), I recommend still extracting the -nspkg.pth file and then executing it explicitly, at the same place that fetch_build_eggs adds the installed packages to the working set.

All that said, I'm not sure my recommendation is viable or what other implications it may have, and this issue is pretty pressing for a fix. I'm comfortable proceeding with this patch, with the understanding that it's running in conflict with the namespace handling as pip would have installed it... and that the project will deal with that difference at another time.

@benoit-pierre
Copy link
Member Author

OK, 2 issues I can think of when using a pth file:

  • we cannot use the one included in the wheel: modules are loaded relative to it
  • the pth file won't be uninstalled by pip

@benoit-pierre
Copy link
Member Author

Additionally, what will happen if multiple versions of the same package are installed?

@jaraco
Copy link
Member

jaraco commented Nov 27, 2017

Oh, gosh. I see now that easy_installs wheels by converting them to eggs. And since eggs do have these __init__ files, it seems perfectly reasonable for them to retain them as part of the install process. So this PR seems like the right fix, as the behavior falls under the existing deprecation of eggs themselves.

@benoit-pierre benoit-pierre merged commit b9df5fd into pypa:master Nov 27, 2017
@benoit-pierre benoit-pierre deleted the fix_wheels_namespace_packages branch November 27, 2017 22:21
@@ -124,3 +131,14 @@ def raw_req(req):
os.rmdir(subdir)
if os.path.exists(dist_data):
os.rmdir(dist_data)
# Fix namespace packages.
namespace_packages = os.path.join(egg_info, 'namespace_packages.txt')
Copy link
Contributor

@leorochael leorochael Dec 1, 2017

Choose a reason for hiding this comment

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

Apropos @reinout's comment on #1214, shouldn't the line above be like this instead?

try:
    namespace_packages = zf.read(
        '%s/namespace_packages.txt' % (dist_info,)
    ).splitlines()
except KeyError:
    pass
else:
    for mod in namespace_packages:
        [...]

Copy link
Contributor

Choose a reason for hiding this comment

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

Your code isn't completely clear. The %s, what is that?

The problem is that egg_info instead of *dist-info/ is used (I believe).

Copy link
Member Author

Choose a reason for hiding this comment

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

The dist-info directory is renamed to EGG-INFO as part of the conversion to an unzipped-egg.

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally, buildout lets setuptools download the release and then calls setuptools.archive_util.unpack_archive() when it gets an .egg file from setuptools download step.

Now suddenly setuptools starts feeding buildout wheels :-) I'm calling unpack_archive() on those, too. It sounds like I should use a different function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Your code isn't completely clear. The %s, what is that?

@reinout, good catch. I've fixed my comment. that part should be:

'%s/namespace_packages.txt' % (dist_info,)

The gist of it is that we should read namespace_packages.txt from the original zipped wheel metadata, instead of the converted metadata.

Copy link
Contributor

Choose a reason for hiding this comment

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

@leorochael: yes, that ought to help get the __init__.py generated, even when I'm using the regular .unpack_archive() function.
(I don't know if there's more that we're missing if we don't let setuptools convert it to an egg).

Copy link
Member Author

@benoit-pierre benoit-pierre Dec 1, 2017

Choose a reason for hiding this comment

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

@leorochael: Why? This particular metadata is unchanged during the conversion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like I just said in #1214, I was simply calling the wrong function. I'm letting setuptools extract the .whl now and it does the right thing.

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.

None yet

4 participants