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 issues with distribution location from eggs #629

Merged
merged 3 commits into from Oct 16, 2016

Conversation

Projects
None yet
3 participants
@kata198
Contributor

kata198 commented Jul 5, 2016

Please consider the following pull request, which addresses real-world issues (and took a while to debug).

  1. "Skip empty egg directories, which may be leftover from previous installation" 1b228fa

This patch will ignore empty egg directories, which I think is intended.
Some background: When using setup.py install --record option, only the FILES are listed, not the directories. It is common practice, especially from third-party distributions of RPMS to use this "record" option to generate a file list. This creates the side-effect that when the package is upgraded, the old .egg-info directory is left around, and empty.

Thus, when something like pkg_resources.get_distribution('packageName') is called, it returns a distribution based off the OLD empty egg info, which causes issues and/or complete failure.

I'm sure there are many other circumstances that could leave behind an empty egg directory.

This patch causes empty egg-info directories to be skipped.


\2. "Scan for distributions in reverse order, so we find the newest version of a distrubtion (instead of the oldest)" -- fb1867e

Since os.listdir returns (at least on POSIX) directories in lexicographical order, old versions of eggs will be found and associated with a distribution, even when newer versions are present.

This patch searches the reverse of os.listdir, which will find and associate the NEWEST version of a package instead of the OLDEST (for calls like pkg_resources.get_distribution)

If you don't want to apply number 1 (because, I understand that while nothing in practice does generate empty egg directories, the name itself can identify version, etc), this patch in practice usually will result in the same goal, since during upgrades the newer package will be found and the empty ignored anyway. However, this patch alone does not cover downgrades, etc.


Please seriously consider these two patches. I know there are workarounds in the sense of deleting old empty egg directories, but in real-world one often has no control over third-party RPMs etc which by template use the "record" option of setup.py. Also, it could be the case that someone manually installs an old egg on one server, and then an rpm comes along later and installs a newer version, and you have an issue that 1/4 times the application fails because it identifies the wrong egg, etc. All very hard to debug, and in the case of using third-party distributions require things like pruning empty directories through cron, etc. These patches could alleviate these issues, and I think overall harden the setuptools codebase.

Thanks,
Tim

@kata198

This comment has been minimized.

Contributor

kata198 commented Jul 5, 2016

Steps to reproduce:

  1. Install cryptography (currently at 1.4)
  2. Create a directory in site-packages, named cryptography-1.3-py2.7.egg-info (replacing 2.7 with whatever py version you're using) to simulate an artifact from prior install
  3. python -c "import pkg_resources; print ( repr(pkg_resources.get_distribution('cryptography')))"

Before:
You'd get cryptography 1.3 (based on the old, empty egg)

After / Desired:
With either patch or both you'll get the correct cryptography 1.4

@fukanchik

This comment has been minimized.

fukanchik commented on fb1867e Jul 5, 2016

From here: https://docs.python.org/2/library/os.html#os.listdir

... The list is in arbitrary order....

@fukanchik

This comment has been minimized.

fukanchik commented Jul 5, 2016

This should also fix pyca/cryptography/issues/2907

@kata198

This comment has been minimized.

Contributor

kata198 commented Jul 5, 2016

Thanks, you're right about listdir not being sorted by default. I've updated to add a .sort()

@jaraco

This comment has been minimized.

Member

jaraco commented Oct 14, 2016

I apologize for letting this issue linger for so long. After reviewing, the changes seem reasonable. It would be nice if the changes had tests, but I'm not going to insist on that. At the same time, I disavow responsibility if the functionality gets optimized out because it's not protected by tests.

One concern I have is that lexicographic sorting is not the same as package version order. The simple example is that lexicographically, 2.7.10 < 2.7.2. How about use package version order to perform the sort? Or is that condition not important for this optimization?

@kata198

This comment has been minimized.

Contributor

kata198 commented Oct 16, 2016

It would be better to extract and sort by the version. In practice, this
issue has not come up, but in theory it should. I personally think if
lexicographical sorting resolves the observed issue 99% of the time, it is
still a functional improvement, but could still be further improved upon.

On Fri, Oct 14, 2016 at 1:27 PM, Jason R. Coombs notifications@github.com
wrote:

I apologize for letting this issue linger for so long. After reviewing,
the changes seem reasonable. It would be nice if the changes had tests, but
I'm not going to insist on that. At the same time, I disavow responsibility
if the functionality gets optimized out because it's not protected by tests.

One concern I have is that lexicographic sorting is not the same as
package version order. The simple example is that lexicographically, 2.7.10
< 2.7.2. How about use package version order to perform the sort? Or is
that condition not important for this optimization?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#629 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIEbO726L9y0Sdl0o0dKdxGtev5Q869Qks5qz7tkgaJpZM4JFhVN
.

@jaraco

This comment has been minimized.

Member

jaraco commented Oct 16, 2016

Works for me.

@jaraco jaraco merged commit 5273dba into pypa:master Oct 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kata198 added a commit to kata198/setuptools that referenced this pull request Oct 25, 2016

Update per comment on PR pypa#629 . Sort in a version-aware manner, s…
…uch that 2.7.10 comes AFTER 2.7.2, not before.

jaraco added a commit that referenced this pull request Nov 4, 2016

Use packaging.version.Version to sort filenames by the version of the…
… package they represent. Alternate implementation of that proposed in #829. Also ref #629.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment