Skip to content

Loading…

wheel finder priority based on the order of the pep425 supported tags #882

Merged
merged 2 commits into from

4 participants

@qwcode
Python Packaging Authority member

contains the logic to prefer e.g. "simple-0.1-cp26-none-linux_x86_64.whl'" over "simple-0.1-py2-none-any.whl"

contains some refactor for testability.

I was tempted to do more refactor, but better to isolate that in a separate pull.

@qwcode qwcode was assigned
@hltbra hltbra commented on an outdated diff
tests/test_wheel.py
((5 lines not shown))
+class TestWheelFile(object):
+
+ @patch('pip.wheel.supported_tags', [('py2', 'none', 'any')])
+ def test_supported_single_version(self):
+ """
+ Test single-version wheel is known to be supported
+ """
+ w = wheel.Wheel('simple-0.1-py2-none-any.whl')
+ assert w.supported()
+
+ @patch('pip.wheel.supported_tags', [('py3', 'none', 'any')])
+ def test_supported_multi_version(self):
+ """
+ Test multi-version wheel is known to be supported
+ """
+ mock_supported_tags = [('py3', 'none', 'any')]
@hltbra Python Packaging Authority member
hltbra added a note

dead code?

@qwcode Python Packaging Authority member
qwcode added a note

yes, good catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@qwcode
Python Packaging Authority member

review request to @dholth , @pfmoore (the pypa wheel experts)

@pfmoore
Python Packaging Authority member

I've read through the patch and from that point of view it looks good. I don't have any examples to check it against right now (although I might be able to set something up - if I do, I'll add a further comment)

@dholth dholth commented on the diff
pip/pep425tags.py
@@ -73,25 +73,26 @@ def get_supported(versions=None):
abis.append('none')
arch = get_platform()
-
@dholth Python Packaging Authority member
dholth added a note

Solid edits.

@qwcode Python Packaging Authority member
qwcode added a note

auto-save stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dholth dholth commented on the diff
pip/index.py
@@ -655,6 +658,11 @@ def __init__(self, url, comes_from=None):
self.url = url
self.comes_from = comes_from
+ # Set whether it's a wheel
+ self.wheel = None
+ if url != Inf and self.splitext()[1] == wheel_ext:
@dholth Python Packaging Authority member
dholth added a note

I always thought Inf / InfLink needed better comments, IIRC they weren't explained much in the source?

@qwcode Python Packaging Authority member
qwcode added a note

fyi, see this #703

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dholth dholth commented on the diff
pip/wheel.py
@@ -188,6 +191,43 @@ def uninstallation_paths(dist):
yield path
+class Wheel(object):
+ """A wheel file"""
+
+ # TODO: maybe move the install code into this class
+
+ wheel_file_re = re.compile(
+ r"""^(?P<namever>(?P<name>.+?)(-(?P<ver>\d.+?))?)
+ ((-(?P<build>\d.*?))?-(?P<pyver>.+?)-(?P<abi>.+?)-(?P<plat>.+?)
+ \.whl|\.dist-info)$""",
@dholth Python Packaging Authority member
dholth added a note

The optional .dist-info extension is no longer needed. We have a separate regex (probably only needed in pkg_resources) for that.

@qwcode Python Packaging Authority member
qwcode added a note

I'll take it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dholth dholth commented on the diff
pip/wheel.py
((11 lines not shown))
+ ((-(?P<build>\d.*?))?-(?P<pyver>.+?)-(?P<abi>.+?)-(?P<plat>.+?)
+ \.whl|\.dist-info)$""",
+ re.VERBOSE)
+
+ def __init__(self, filename):
+ wheel_info = self.wheel_file_re.match(filename)
+ self.filename = filename
+ self.name = wheel_info.group('name').replace('_', '-')
+ self.version = wheel_info.group('ver')
+ self.pyversions = wheel_info.group('pyver').split('.')
+ self.abis = wheel_info.group('abi').split('.')
+ self.plats = wheel_info.group('plat').split('.')
+
+ # All the tag combinations from this file
+ self.file_tags = set((x, y, z) for x in self.pyversions for y
+ in self.abis for z in self.plats)
@dholth Python Packaging Authority member
dholth added a note

Cool, a threefor.

@qwcode Python Packaging Authority member
qwcode added a note

I don't know what you mean.

@dholth Python Packaging Authority member
dholth added a note

Three for's in a comprehension.

@qwcode Python Packaging Authority member
qwcode added a note

you wrote that btw. I just copied over here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dholth
Python Packaging Authority member

lgtm.

@qwcode
Python Packaging Authority member

that's 2 votes. will resolve the conflict and merge away.

@qwcode qwcode merged commit 3e7c94e into pypa:develop

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 193 additions and 58 deletions.
  1. +41 −35 pip/index.py
  2. +12 −11 pip/pep425tags.py
  3. +43 −1 pip/wheel.py
  4. +46 −10 tests/test_finder.py
  5. +1 −1 tests/test_index.py
  6. +50 −0 tests/test_wheel.py
View
76 pip/index.py
@@ -29,8 +29,8 @@
Empty as QueueEmpty)
from pip.backwardcompat import CertificateError
from pip.download import urlopen, path_to_url2, url_to_path, geturl, Urllib2HeadRequest
-import pip.pep425tags
-import pip.wheel
+from pip.wheel import Wheel, wheel_ext, wheel_distribute_support, distribute_requirement
+from pip.pep425tags import supported_tags
__all__ = ['PackageFinder']
@@ -68,9 +68,8 @@ def use_wheel(self):
@use_wheel.setter
def use_wheel(self, value):
self._use_wheel = value
- if self._use_wheel:
- if not pip.wheel.wheel_distribute_support():
- raise InstallationError("pip's wheel support requires %s." % pip.wheel.distribute_requirement)
+ if self._use_wheel and not wheel_distribute_support():
+ raise InstallationError("pip's wheel support requires %s." % distribute_requirement)
def add_dependency_links(self, links):
## FIXME: this shouldn't be global list this, it should only
@@ -122,22 +121,38 @@ def sort_path(path):
def _link_sort_key(self, link_tuple):
"""
Function used to generate link sort key for link tuples.
- If finding wheels, wheels are preferred over sdists
+ The greater the return value, the more preferred it is.
+ If not finding wheels, then sorted by version only.
+ If finding wheels, then the sort order is by version, then:
+ 1. existing installs
+ 2. wheels ordered via Wheel.support_index_min()
+ 3. source archives
+ Note: it was considered to embed this logic into the Link
+ comparison operators, but then different sdist links
+ with the same version, would have to be considered equal
"""
- parsed_version = link_tuple[0]
- link = link_tuple[1]
+ parsed_version, link, _ = link_tuple
if self.use_wheel:
- if link == InfLink: #existing install
- pri = 2
- else:
+ support_num = len(supported_tags)
+ if link == InfLink: # existing install
pri = 1
- link_ext = link.splitext()[1]
- if link_ext == '.whl':
- pri = 2
+ elif link.wheel:
+ # all wheel links are known to be supported at this stage
+ pri = -(link.wheel.support_index_min())
+ else: # sdist
+ pri = -(support_num)
return (parsed_version, pri)
else:
return parsed_version
+ def _sort_versions(self, applicable_versions):
+ """
+ Bring the latest version (and wheels) to the front, but maintain the existing ordering as secondary.
+ See the docstring for `_link_sort_key` for details.
+ This function is isolated for easier unit testing.
+ """
+ return sorted(applicable_versions, key=self._link_sort_key, reverse=True)
+
def find_requirement(self, req, upgrade):
def mkurl_pypi_url(url):
@@ -222,8 +237,7 @@ def mkurl_pypi_url(url):
logger.info("Ignoring link %s, version %s is a pre-release (use --pre to allow)." % (link, version))
continue
applicable_versions.append((parsed_version, link, version))
- #bring the latest version (and wheels) to the front, but maintain the existing ordering as secondary
- applicable_versions = sorted(applicable_versions, key=self._link_sort_key, reverse=True)
+ applicable_versions = self._sort_versions(applicable_versions)
existing_applicable = bool([link for parsed_version, link, version in applicable_versions if link is InfLink])
if not upgrade and existing_applicable:
if applicable_versions[0][1] is InfLink:
@@ -306,11 +320,6 @@ def _get_queued_page(self, req, pending_queue, done, seen):
_egg_fragment_re = re.compile(r'#egg=([^&]*)')
_egg_info_re = re.compile(r'([a-z0-9_.]+)-([a-z0-9_.-]+)', re.I)
_py_version_re = re.compile(r'-py([123]\.?[0-9]?)$')
- _wheel_info_re = re.compile(
- r"""^(?P<namever>(?P<name>.+?)(-(?P<ver>\d.+?))?)
- ((-(?P<build>\d.*?))?-(?P<pyver>.+?)-(?P<abi>.+?)-(?P<plat>.+?)
- \.whl|\.dist-info)$""",
- re.VERBOSE)
def _sort_links(self, links):
"Returns elements of links in order, non-egg links first, egg links second, while eliminating duplicates"
@@ -333,7 +342,7 @@ def _package_versions(self, links, search_name):
def _known_extensions(self):
extensions = ('.tar.gz', '.tar.bz2', '.tar', '.tgz', '.zip')
if self.use_wheel:
- return extensions + ('.whl',)
+ return extensions + (wheel_ext,)
return extensions
def _link_package_versions(self, link, search_name):
@@ -368,19 +377,11 @@ def _link_package_versions(self, link, search_name):
logger.debug('Skipping link %s; macosx10 one' % (link))
self.logged_links.add(link)
return []
- if ext == '.whl':
- wheel_info = self._wheel_info_re.match(link.filename)
- if wheel_info.group('name').replace('_', '-').lower() == search_name.lower():
- version = wheel_info.group('ver')
- pyversions = wheel_info.group('pyver').split('.')
- abis = wheel_info.group('abi').split('.')
- plats = wheel_info.group('plat').split('.')
- wheel_supports = set((x, y, z) for x in pyversions for y
- in abis for z in plats)
- supported = set(pip.pep425tags.get_supported())
- if not supported.intersection(wheel_supports):
- logger.debug('Skipping %s because it is not compatible with this Python' % link)
- return []
+ if link.wheel and link.wheel.name.lower() == search_name.lower():
+ version = link.wheel.version
+ if not link.wheel.supported():
+ logger.debug('Skipping %s because it is not compatible with this Python' % link)
+ return []
if not version:
version = self._egg_info_matches(egg_info, search_name, link)
if version is None:
@@ -668,6 +669,11 @@ def __init__(self, url, comes_from=None):
self.url = url
self.comes_from = comes_from
+ # Set whether it's a wheel
+ self.wheel = None
+ if url != Inf and self.splitext()[1] == wheel_ext:
@dholth Python Packaging Authority member
dholth added a note

I always thought Inf / InfLink needed better comments, IIRC they weren't explained much in the source?

@qwcode Python Packaging Authority member
qwcode added a note

fyi, see this #703

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.wheel = Wheel(self.filename)
+
def __str__(self):
if self.comes_from:
return '%s (from %s)' % (self.url, self.comes_from)
View
23 pip/pep425tags.py
@@ -41,11 +41,11 @@ def get_supported(versions=None):
"""Return a list of supported tags for each version specified in
`versions`.
- :param versions: a list of string versions, of the form ["33", "32"],
+ :param versions: a list of string versions, of the form ["33", "32"],
or None. The first version will be assumed to support our ABI.
"""
supported = []
-
+
# Versions must be given with respect to the preference
if versions is None:
versions = []
@@ -53,15 +53,15 @@ def get_supported(versions=None):
# Support all previous minor Python versions.
for minor in range(sys.version_info[1], -1, -1):
versions.append(''.join(map(str, (major, minor))))
-
+
impl = get_abbr_impl()
-
+
abis = []
soabi = sysconfig.get_config_var('SOABI')
if soabi and soabi.startswith('cpython-'):
abis[0:0] = ['cp' + soabi.split('-', 1)[-1]]
-
+
abi3s = set()
import imp
for suffix in imp.get_suffixes():
@@ -73,25 +73,26 @@ def get_supported(versions=None):
abis.append('none')
arch = get_platform()
-
@dholth Python Packaging Authority member
dholth added a note

Solid edits.

@qwcode Python Packaging Authority member
qwcode added a note

auto-save stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
# Current version, current API (built specifically for our Python):
for abi in abis:
supported.append(('%s%s' % (impl, versions[0]), abi, arch))
-
+
# No abi / arch, but requires our implementation:
for i, version in enumerate(versions):
supported.append(('%s%s' % (impl, version), 'none', 'any'))
if i == 0:
- # Tagged specifically as being cross-version compatible
+ # Tagged specifically as being cross-version compatible
# (with just the major version specified)
- supported.append(('%s%s' % (impl, versions[0][0]), 'none', 'any'))
-
+ supported.append(('%s%s' % (impl, versions[0][0]), 'none', 'any'))
+
# No abi / arch, generic Python
for i, version in enumerate(versions):
supported.append(('py%s' % (version,), 'none', 'any'))
if i == 0:
supported.append(('py%s' % (version[0]), 'none', 'any'))
-
+
return supported
+supported_tags = get_supported()
View
44 pip/wheel.py
@@ -1,5 +1,5 @@
"""
-Support functions for installing and building the "wheel" binary package format.
+Support for installing and building the "wheel" binary package format.
"""
from __future__ import with_statement
@@ -8,14 +8,17 @@
import hashlib
import os
import pkg_resources
+import re
import shutil
import sys
from base64 import urlsafe_b64encode
from pip.locations import distutils_scheme
from pip.log import logger
+from pip.pep425tags import supported_tags
from pip.util import call_subprocess, normalize_path, make_path_relative
+wheel_ext = '.whl'
distribute_requirement = pkg_resources.Requirement.parse("distribute>=0.6.34")
def wheel_distribute_support(distribute_req=distribute_requirement):
@@ -33,6 +36,7 @@ def wheel_distribute_support(distribute_req=distribute_requirement):
logger.warn("%s is required for wheel installs.", distribute_req)
return supported
+
def rehash(path, algo='sha256', blocksize=1<<20):
"""Return (hash, length) for path using hashlib.new(algo)"""
h = hashlib.new(algo)
@@ -182,6 +186,7 @@ def unique(*args, **kw):
yield item
return unique
+# TODO: this goes somewhere besides the wheel module
@_unique
def uninstallation_paths(dist):
"""
@@ -204,6 +209,43 @@ def uninstallation_paths(dist):
yield path
+class Wheel(object):
+ """A wheel file"""
+
+ # TODO: maybe move the install code into this class
+
+ wheel_file_re = re.compile(
+ r"""^(?P<namever>(?P<name>.+?)(-(?P<ver>\d.+?))?)
+ ((-(?P<build>\d.*?))?-(?P<pyver>.+?)-(?P<abi>.+?)-(?P<plat>.+?)
+ \.whl|\.dist-info)$""",
@dholth Python Packaging Authority member
dholth added a note

The optional .dist-info extension is no longer needed. We have a separate regex (probably only needed in pkg_resources) for that.

@qwcode Python Packaging Authority member
qwcode added a note

I'll take it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ re.VERBOSE)
+
+ def __init__(self, filename):
+ wheel_info = self.wheel_file_re.match(filename)
+ self.filename = filename
+ self.name = wheel_info.group('name').replace('_', '-')
+ self.version = wheel_info.group('ver')
+ self.pyversions = wheel_info.group('pyver').split('.')
+ self.abis = wheel_info.group('abi').split('.')
+ self.plats = wheel_info.group('plat').split('.')
+
+ # All the tag combinations from this file
+ self.file_tags = set((x, y, z) for x in self.pyversions for y
+ in self.abis for z in self.plats)
@dholth Python Packaging Authority member
dholth added a note

Cool, a threefor.

@qwcode Python Packaging Authority member
qwcode added a note

I don't know what you mean.

@dholth Python Packaging Authority member
dholth added a note

Three for's in a comprehension.

@qwcode Python Packaging Authority member
qwcode added a note

you wrote that btw. I just copied over here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ def support_index_min(self):
+ """
+ Return the lowest index that a file_tag achieves in the supported_tags list
+ e.g. if there are 8 supported tags, and one of the file tags is first in the
+ list, then return 0.
+ """
+ indexes = [supported_tags.index(c) for c in self.file_tags if c in supported_tags]
+ return min(indexes) if indexes else None
+
+ def supported(self):
+ """Is this wheel supported on this system?"""
+ return bool(set(supported_tags).intersection(self.file_tags))
+
class WheelBuilder(object):
"""Build wheels from a RequirementSet."""
View
56 tests/test_finder.py
@@ -2,13 +2,13 @@
from pkg_resources import parse_version
from pip.backwardcompat import urllib
from pip.req import InstallRequirement
-from pip.index import PackageFinder
+from pip.index import PackageFinder, Link
from pip.exceptions import BestVersionAlreadyInstalled, DistributionNotFound
+from pip.util import Inf
from tests.path import Path
from tests.test_pip import here, path_to_url
from nose.tools import assert_raises
from mock import Mock, patch
-import os
find_links = path_to_url(os.path.join(here, 'packages'))
find_links2 = path_to_url(os.path.join(here, 'packages2'))
@@ -78,24 +78,31 @@ def test_finder_detects_latest_already_satisfied_pypi_links():
finder = PackageFinder([], ["http://pypi.python.org/simple"])
assert_raises(BestVersionAlreadyInstalled, finder.find_requirement, req, True)
-@patch('pip.pep425tags.get_supported')
-def test_find_wheel(mock_get_supported):
+@patch('pip.wheel.supported_tags', [('py1', 'none', 'any')])
+def test_not_find_wheel_not_supported():
"""
- Test finding wheels.
+ Test not finding an unsupported wheel.
"""
find_links_url = 'file://' + os.path.join(here, 'packages')
find_links = [find_links_url]
req = InstallRequirement.from_line("simple.dist")
finder = PackageFinder(find_links, [], use_wheel=True)
- mock_get_supported.return_value = [('py1', 'none', 'any')]
assert_raises(DistributionNotFound, finder.find_requirement, req, True)
- mock_get_supported.return_value = [('py2', 'none', 'any')]
- found = finder.find_requirement(req, True)
- assert found.url.endswith("simple.dist-0.1-py2.py3-none-any.whl"), found
- mock_get_supported.return_value = [('py3', 'none', 'any')]
+
+
+@patch('pip.wheel.supported_tags', [('py2', 'none', 'any')])
+def test_find_wheel_supported():
+ """
+ Test finding supported wheel.
+ """
+ find_links_url = 'file://' + os.path.join(here, 'packages')
+ find_links = [find_links_url]
+ req = InstallRequirement.from_line("simple.dist")
+ finder = PackageFinder(find_links, [], use_wheel=True)
found = finder.find_requirement(req, True)
assert found.url.endswith("simple.dist-0.1-py2.py3-none-any.whl"), found
+
def test_finder_priority_file_over_page():
"""Test PackageFinder prefers file links over equivalent page links"""
req = InstallRequirement.from_line('gmpy==1.15', None)
@@ -131,6 +138,7 @@ def test_finder_priority_nonegg_over_eggfragments():
def test_wheel_over_sdist_priority():
"""
Test wheels have priority over sdists.
+ `test_link_sorting` also covers this at lower level
"""
req = InstallRequirement.from_line("priority")
finder = PackageFinder([find_links], [], use_wheel=True)
@@ -140,6 +148,7 @@ def test_wheel_over_sdist_priority():
def test_existing_over_wheel_priority():
"""
Test existing install has priority over wheels.
+ `test_link_sorting` also covers this at a lower level
"""
req = InstallRequirement.from_line('priority', None)
latest_version = "1.0"
@@ -230,3 +239,30 @@ def test_finder_installs_pre_releases_with_version_spec():
finder = PackageFinder(links, [])
link = finder.find_requirement(req, False)
assert link.url == "https://foo/bar-2.0b1.tar.gz"
+
+
+@patch('pip.wheel.supported_tags', [
+ ('pyT', 'none', 'TEST'),
+ ('pyT', 'TEST', 'any'),
+ ('pyT', 'none', 'any'),
+ ])
+def test_link_sorting():
+ """
+ Test link sorting
+ """
+ links = [
+ (parse_version('2.0'), Link(Inf), '2.0'),
+ (parse_version('2.0'), Link('simple-2.0.tar.gz'), '2.0'),
+ (parse_version('1.0'), Link('simple-1.0-pyT-none-TEST.whl'), '1.0'),
+ (parse_version('1.0'), Link('simple-1.0-pyT-TEST-any.whl'), '1.0'),
+ (parse_version('1.0'), Link('simple-1.0-pyT-none-any.whl'), '1.0'),
+ (parse_version('1.0'), Link('simple-1.0.tar.gz'), '1.0'),
+ ]
+
+ finder = PackageFinder([], [])
+ finder.use_wheel = True
+
+ results = finder._sort_versions(links)
+ results2 = finder._sort_versions(sorted(links, reverse=True))
+
+ assert links == results == results2, results2
View
2 tests/test_index.py
@@ -104,7 +104,7 @@ def test_file_index_url_quoting():
def test_inflink_greater():
"""Test InfLink compares greater."""
- assert InfLink > Link(object())
+ assert InfLink > Link("some link")
def test_mirror_url_formats():
View
50 tests/test_wheel.py
@@ -3,6 +3,7 @@
import pkg_resources
import sys
import textwrap
+from mock import patch
from mock import patch
from nose import SkipTest
@@ -187,3 +188,52 @@ def test_finder_no_raises_error(self, mock_get_distribution):
p = PackageFinder([], [])
p.use_wheel = False
+
+class TestWheelFile(object):
+
+ @patch('pip.wheel.supported_tags', [('py2', 'none', 'any')])
+ def test_supported_single_version(self):
+ """
+ Test single-version wheel is known to be supported
+ """
+ w = wheel.Wheel('simple-0.1-py2-none-any.whl')
+ assert w.supported()
+
+ @patch('pip.wheel.supported_tags', [('py3', 'none', 'any')])
+ def test_supported_multi_version(self):
+ """
+ Test multi-version wheel is known to be supported
+ """
+ w = wheel.Wheel('simple-0.1-py2.py3-none-any.whl')
+ assert w.supported()
+
+ @patch('pip.wheel.supported_tags', [('py1', 'none', 'any')])
+ def test_not_supported_version(self):
+ """
+ Test unsupported wheel is known to be unsupported
+ """
+ w = wheel.Wheel('simple-0.1-py2-none-any.whl')
+ assert not w.supported()
+
+ @patch('pip.wheel.supported_tags', [
+ ('py2', 'none', 'TEST'),
+ ('py2', 'TEST', 'any'),
+ ('py2', 'none', 'any'),
+ ])
+ def test_support_index_min(self):
+ """
+ Test results from `support_index_min`
+ """
+ w = wheel.Wheel('simple-0.1-py2-none-any.whl')
+ assert w.support_index_min() == 2
+ w = wheel.Wheel('simple-0.1-py2-none-TEST.whl')
+ assert w.support_index_min() == 0
+
+ @patch('pip.wheel.supported_tags', [])
+ def test_support_index_min_none(self):
+ """
+ Test `support_index_min` returns None, when wheel not supported
+ """
+ w = wheel.Wheel('simple-0.1-py2-none-any.whl')
+ assert w.support_index_min() == None
+
Something went wrong with that request. Please try again.