Add PEP 513 support (manylinux1 platform tag) #3497

ogrisel · 2016-02-19T21:52:46Z

This is a rebased and squashed version of #3446 with fixes for the failed flake8 checks. The goal is to add support for the manylinux1 platform tag.

ogrisel · 2016-02-19T22:11:57Z

The TOXENV=docs failure is unrelated to this PR: caused by an outage on https://www.pypa.io/en/latest/

ogrisel · 2016-02-21T16:26:07Z

pip/pep425tags.py

@@ -189,6 +236,11 @@ def get_supported(versions=None, noarch=False):
            else:
                # arch pattern didn't match (?!)
                arches = [arch]
+        elif sys.platform == 'linux':
+            if is_manylinux1_compatible():
+                arches = [arch, arch.replace('linux', 'manylinux1')]


@rmcgibbo @njsmith is the order important here? For instance in case both the linux_x86_64 and the manylinux1_x86_64 wheel files are available in a wheel index, would pip choose the first or the later?

I don't know, either what the effect is or what the preferred behavior is... To check what the effect is I guess it would be easy to check (you can trivially run pip out of a source checkout by running python -m pipwhile in the root of the checkout). And in terms of what behavior we want, I think it's pretty arbitrary really -- the case where it comes up is very unlikely, and in this unlikely case, I can't think of any strong reason to prefer one outcome or another.

I did the test, and the current ordering make pip install select the linux arch over the manylinux1 arch when both wheels are available on the index (in my case a local folder). Switching the ordering at that line yield the opposite behavior.

pro: this is backward compatible with the behavior of pip 8.0 (and earlier)

con: this is less specific, one could expect pip to select the most platform specific wheel instead.

Note that pip wheel does not seem to be impacted by this change as it calls python setup.py bdist_wheel in a subprocess (I think) and therefore always name the generated file with the linux arch. I think this is good as to generate a valid manylinux1 wheel file extract care is required (e.g. using auditwheel repair to embed system shared libraries not provided a manylinux1 compatible system).

@dstufft any opinion on this ordering issue?

pip should always select the most specific available.

ogrisel · 2016-02-22T18:22:21Z

@dstufft I amended the change to make sure the most specific supported tag comes first and added a test for this.

ogrisel · 2016-02-22T18:22:25Z

pip/pep425tags.py

@@ -198,7 +247,8 @@ def get_supported(versions=None, noarch=False):
                supported.append(('%s%s' % (impl, versions[0]), abi, arch))

        # Has binaries, does not use the Python API:
-        supported.append(('py%s' % (versions[0][0]), 'none', arch))
+        for arch in arches:
+            supported.append(('py%s' % (versions[0][0]), 'none', arch))


@dstufft this change also impacts macosx: previously it would use the arch from get_platform() instead of all possible arches expanded in this function. I think this was a bug but I am not 100% sure.

I also think it is a bug fix, and for a bug that was never noticed before because it only affects wheels that combine a none ABI tag + a non-trivial platform tag, which is a rare (but totally valid) configuration.

This is a partial fix to an issue described in #3202; I have a PR open with a full fix at #3357. The full fix generates an order of magnitude more tags on my machine than this PR (198 vs 2196, though these both include impossible tags as described in #3403).

I think ~2000 should not cause any performance issue for pip but it would be worth auditing the codebase to check that this list of tags is not used by any algorithm with quadratic complexity or more.

get_supported() is used directly in 3 places:

index.py#L245: len(tags): O(1)

wheel.py#L656: [tags.index(c) for c in self.file_tags if c in tags]: self.file_tags is the set of tag combinations in a wheel filename so won't be very large. c in tags complexity is O(len(tags)) = O(n). Not sure what the complexity of tags.index(c) is but I assume it's implemented the same way as c in tags and should then be O(n), so this whole statement is O(n).

wheel.py#L663: bool(set(tags).intersection(self.file_tags)): Set construction is O(len(tags)) = O(n) and set intersection is O(len(tags)) = O(n) so the whole thing is also O(n).

I don't see any loops or recursion in the callers that would cause any of these to be called len(tags) times since they're used on lists of wheels discovered e.g. from the index or in a cache directory. So I suppose the danger would be if an index had thousands of wheels.

ogrisel · 2016-02-22T18:24:48Z

Also do we need a functional test for this new feature or are the unit tests enough?

Ivoz · 2016-02-23T06:15:19Z

@ogrisel I think pep425tags unit tests should be moved to a new file, rather than shoved under wheel.

ogrisel · 2016-02-23T07:46:56Z

@Ivoz alright, I will do that.

ogrisel · 2016-02-23T08:21:24Z

Done.

ogrisel · 2016-02-23T15:24:35Z

@rmcgibbo sorry I lost authorship information in the process or splitting the commit into 2.

ogrisel · 2016-02-24T09:23:15Z

I fixed the authorship of the last commit. Let me know if there are any other problem with this PR.

dstufft · 2016-03-03T11:56:50Z

pip/pep425tags.py

@@ -124,7 +124,54 @@ def get_platform():
        split_ver = release.split('.')
        return 'macosx_{0}_{1}_{2}'.format(split_ver[0], split_ver[1], machine)
    # XXX remove distutils dependency
-    return distutils.util.get_platform().replace('.', '_').replace('-', '_')
+    result = distutils.util.get_platform().replace('.', '_').replace('-', '_')
+    if result == "linux_x86_64" and sys.maxsize == 2147483647:


What's up with this check? Can we add a comment as to why it exists?

BTW, is it somewhat related to this quite old PR #1377 ?

If you run 32 bit Python on a 64 bit linux (e.g. via anaconda or even if you debootstrap a i686 ubuntu inside a 64 bit ubuntu), then distutils.util.get_platform() returns "linux-x86_64" even though Python itself is 32 bit.

I don't know if this is a bug of distutils or not.

This occurs because distutils.util.get_platform() relies on os.uname() (and thus POSIX uname()) for the architecture of the platform. It does not make any attempt to determine the "platform" of the interpreter itself.

Yeah, this fixes the same bug that PR #1377 was trying to address; see also this mailing list message. The bug has been around a long time, but it becomes much more urgent to fix as soon manylinux support lands; in the past it only caused a problem if you specifically did pip install -f <directory with 64 bit wheels> from a 32-bit userland, but once 64-bit linux wheels are on pypi [edit: and people are using manylinux-enabled pip], then it will become possible to hit by just doing pip install. So it's important to land this fix at the same time we enable manylinux support.

Just a note, that it's likely that the wheel project has this bug too.

Fixed in wheel: https://bitbucket.org/pypa/wheel/commits/4da780b849affbff98a0defb21ad1f30a94d6f57

ogrisel · 2016-03-03T14:07:30Z

pip/pep425tags.py

+    result = distutils.util.get_platform().replace('.', '_').replace('-', '_')
+    if result == "linux_x86_64" and sys.maxsize == 2147483647:
+        # 32 bit Python program (running on a 64 bit Linux): pip should only
+        # install and run 32 bit compiled extensions in that case.


@dstufft I amended the commit to add this comment to explain this particular case. Let me know if would prefer another phrasing.

ogrisel · 2016-03-03T14:10:16Z

I amended the last commit to move the ctypes import. Github has folded the unrelated #3497 (comment) comment as the result even though this remark has not been addressed yet.

xavfernandez · 2016-03-03T22:57:51Z

pip/pep425tags.py

+
+    # Parse string and check against requested version.
+    version = [int(piece) for piece in version_str.split(".")]
+    assert len(version) == 2


I don't think we want an assert here: we'd rather return False and not crash pip.

ogrisel · 2016-03-04T08:48:12Z

Thanks @xavfernandez, I addressed your comments.

Add PEP 513 support (manylinux1 platform tag)

xavfernandez added this to the 8.1 milestone Feb 19, 2016

ogrisel reviewed Feb 21, 2016
View reviewed changes

ogrisel mentioned this pull request Feb 21, 2016

Implement PEP 513 #3446

Closed

ogrisel force-pushed the pr-3446-followup branch from dd8cb40 to 624066e Compare February 22, 2016 17:59

ogrisel reviewed Feb 22, 2016
View reviewed changes

ogrisel force-pushed the pr-3446-followup branch 2 times, most recently from aa27185 to 309d403 Compare February 23, 2016 08:20

Move pep425tags tests to their own file

09b99e8

ogrisel force-pushed the pr-3446-followup branch from 309d403 to 8fe9302 Compare February 23, 2016 08:36

ogrisel mentioned this pull request Feb 23, 2016

Handle wheels that are only OS specific #3202

Closed

Ivoz mentioned this pull request Feb 24, 2016

Verify whether a given OS X version supports a given arch (fixes #3403) #3411

Closed

ogrisel force-pushed the pr-3446-followup branch from 8fe9302 to 3e67145 Compare February 24, 2016 09:18

ogrisel changed the title ~~Add PEP 513 support~~ Add PEP 513 support (manylinux1 platform tag) Feb 25, 2016

dstufft reviewed Mar 3, 2016
View reviewed changes

ogrisel force-pushed the pr-3446-followup branch from 3e67145 to 99e8493 Compare March 3, 2016 14:05

ogrisel reviewed Mar 3, 2016
View reviewed changes

xavfernandez reviewed Mar 3, 2016
View reviewed changes

ogrisel force-pushed the pr-3446-followup branch from 99e8493 to d83957b Compare March 4, 2016 08:43

Implement PEP 513: manylinux1 platform tags

b0351b3

ogrisel force-pushed the pr-3446-followup branch from d83957b to b0351b3 Compare March 4, 2016 08:55

dstufft added a commit that referenced this pull request Mar 4, 2016

Merge pull request #3497 from ogrisel/pr-3446-followup

05baa9f

Add PEP 513 support (manylinux1 platform tag)

dstufft merged commit 05baa9f into pypa:develop Mar 4, 2016

xavfernandez mentioned this pull request Mar 4, 2016

On a 32-bit Linux Python installation install 32-bit binary packages #1377

Closed

natefoo mentioned this pull request Mar 4, 2016

Switch to standard pip galaxyproject/galaxy#1864

Closed

rouge8 mentioned this pull request Jul 7, 2016

Support manylinux1 wheels pex-tool/pex#281

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PEP 513 support (manylinux1 platform tag) #3497

Add PEP 513 support (manylinux1 platform tag) #3497

ogrisel commented Feb 19, 2016

ogrisel commented Feb 19, 2016

ogrisel Feb 21, 2016

njsmith Feb 21, 2016

ogrisel Feb 22, 2016

dstufft Feb 22, 2016

ogrisel commented Feb 22, 2016

ogrisel Feb 22, 2016

njsmith Feb 22, 2016

carloslbello Feb 23, 2016

ogrisel Feb 24, 2016

natefoo Mar 2, 2016

ogrisel commented Feb 22, 2016

Ivoz commented Feb 23, 2016

ogrisel commented Feb 23, 2016

ogrisel commented Feb 23, 2016

ogrisel commented Feb 23, 2016

ogrisel commented Feb 24, 2016

dstufft Mar 3, 2016

xavfernandez Mar 3, 2016

ogrisel Mar 3, 2016

natefoo Mar 3, 2016

njsmith Mar 3, 2016

dstufft Mar 4, 2016

natefoo Mar 4, 2016

ogrisel Mar 3, 2016

ogrisel commented Mar 3, 2016

xavfernandez Mar 3, 2016

ogrisel commented Mar 4, 2016

Add PEP 513 support (manylinux1 platform tag) #3497

Add PEP 513 support (manylinux1 platform tag) #3497

Conversation

ogrisel commented Feb 19, 2016

ogrisel commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel commented Feb 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel commented Feb 22, 2016

Ivoz commented Feb 23, 2016

ogrisel commented Feb 23, 2016

ogrisel commented Feb 23, 2016

ogrisel commented Feb 23, 2016

ogrisel commented Feb 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel commented Mar 3, 2016

Choose a reason for hiding this comment

ogrisel commented Mar 4, 2016