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

Copy package metadata from wheels and from modules #161

Merged
merged 6 commits into from Sep 30, 2018

Conversation

Projects
None yet
2 participants
@adferrand
Contributor

adferrand commented Sep 25, 2018

Following #160.

This PR modify the wheel and modules copy process to ensure that the package metadata (either *.dist-info or *.egg-info) are inserted in the pkgs directory before the installer is built.

This allows to use the plugins discovery mechanism inside a python application installed through the generated NSIS installer.

Regards,
Adrien Ferrand

Adrien Ferrand and others added some commits Sep 25, 2018

Adrien Ferrand
@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 29, 2018

Thanks! I'd actually prefer to do this only from wheels, though, and leave it out for packages found by their import name. Sorry to tell you this after you've implemented that; allow me to explain a bit more.

pkg_resources.get_distribution() expects a distribution name, not a module name - they're the same for many packages, but definitely not for all (e.g. pyzmq is imported as zmq). So trying to use the import name as a distribution name will lead to confusing, inconsistent behaviour. And even if we did implement correctly finding the distribution metadata, it may not accurately describe the current code (e.g. if you have an editable install).

Conceptually, this makes sense: a wheel is a distribution, containing one or more packages and some metadata. If you specify an importable name, you'll get just the importable package, not a distribution. Specifying packages with importable names is largely a legacy option (except for including the app's own code), and I encourage people to use pypi_wheels wherever possible.

@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 29, 2018

As so often, I thought of a more concise summary just after posting my comment:

The .dist-info metadata is a property of a distribution, such as a wheel, not an importable module. Most distributions consist of one top-level module named after the distribution, but enough don't that I would rather not assume it.

@adferrand

This comment has been minimized.

Contributor

adferrand commented Sep 29, 2018

In fact I tested the situation for an editable install, metedata are corrected resolved from the source code, and will create the correct folder.

But your are conceptually right, one can not expect to have a fully instantiated distribution from a unique module import, and using the plugins discovery mechanism when you explicitly import modules is overkill.

I will update the PR accordingly.

@adferrand

This comment has been minimized.

Contributor

adferrand commented Sep 29, 2018

By the way your project itself has its distribution named pynsist, but the module included in it is nsist ^^

@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 30, 2018

Thanks!

Yup, I was considering using Pynsist as an example, but it's not something you'd normally bundle into an application. :-)

I was a bit too brief about editable installs. The issue is that the metadata is generated when you make the editable install, and if you later update the code (e.g. git pull), the metadata is not regenerated. So you could have metadata saying it's version 4, but the code is actually version 5. Though that's probably overshadowed by the possibility of bundling up a development version which doesn't correspond to any release, which is a general problem of copying in packages found in your development environment.

@adferrand

This comment has been minimized.

Contributor

adferrand commented Sep 30, 2018

By the way, about including development modules.

I was trying to include the module that corresponds to the application itself, what you specify in the entry_point, as a wheel by compiling it first before invoking pynsist. I found this more elegant to have everything packaged the same way, the application and its dependencies, and do not use at all the import module mechanism in pynsist.

But in this case pynsist will fails, as it will try to import the module specified in entry_point from the python environment.

However at runtime it would work, as the module would be installed from the wheel, its path would be added to sys.path, and so the module would be found.

As a workaround I was thinking to use script instead of an entry_point, and make this script able to load from pkgs folder (basically, the same code that the generated script one when entry_point is used). Is it the best way to achieve this?

@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 30, 2018

Hmm, can you open a new issue for that (with whatever error message you get)?

The mechanism to automatically include the package for entry points was added before it was possible to specify wheels. I think it still makes sense in many cases, but I can see that it might make sense to get the application from a wheel instead.

@adferrand

This comment has been minimized.

Contributor

adferrand commented Sep 30, 2018

Nevermind, I was certainly doing something wrong: I tried with a simple project to test. In this case, pynsist was able to find the entrypoint from the packages exposed in the included wheels.

I updated my PR to integrate package metadata only from wheels.

Show resolved Hide resolved nsist/copymodules.py Outdated
@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 30, 2018

I'd love to have a simple check for this in the existing nsist.test_pypi.test_download. I know that doing so will produce a merge conflict, unfortunately, but we can resolve that.

Adrien Ferrand added some commits Sep 30, 2018

Adrien Ferrand
@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 30, 2018

Thanks, that's great. And it doesn't even seem to have produced a merge conflict. :-)

@takluyver takluyver merged commit 973a8af into takluyver:master Sep 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 30, 2018

I've done a few bits with Pynsist today, with the aim of making a 2.2 release soon. So it shouldn't be too long before this is in a release.

@adferrand

This comment has been minimized.

Contributor

adferrand commented Oct 1, 2018

Great !

@adferrand adferrand deleted the adferrand:include-dist-info branch Oct 1, 2018

@adferrand adferrand referenced this pull request Oct 3, 2018

Closed

Setuptools integration #22

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