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

Implement PEP 513 #3446

Closed
wants to merge 7 commits into from
Closed

Implement PEP 513 #3446

wants to merge 7 commits into from

Conversation

rmcgibbo
Copy link
Member

@rmcgibbo rmcgibbo commented Feb 2, 2016

This PR implements PEP 513 (the manylinux1 PEP approved on distutils-sig last week). The code was written mostly in the PEP by @njsmith. It's implemented as an extra part of pep425tags.get_supported that, when running on Linux, checks if the current machine is manylinux1 compatible if so, adds it to the returned list of supported tags.

Review on Reviewable

@njsmith
Copy link
Member

njsmith commented Feb 3, 2016

@rmcgibbo: It would be good to also add a check to get_platform like

      platform = distutils.util.get_platform().replace('.', '_').replace('-', '_')
      # for 32-bit python running on a 64-bit kernel, get_platform incorrectly returns linux_x86_64
      # see https://mail.python.org/pipermail/distutils-sig/2016-January/028215.html
      if platform == "linux_x86_64" and sys.maxsize == 2147483647:
          platform = "linux_i686"
      return platform

@rmcgibbo
Copy link
Member Author

rmcgibbo commented Feb 3, 2016

@njsmith do you think pip is really the right place for that?

(It's also worth mentioning as a stopgap, that you can get the linux32 behavior by prefixing commands with linux32, for example running linux32 pip ...)

# Check for presence of _manylinux module
try:
import _manylinux
return bool(_manylinux.manylinux1_compatible)

Choose a reason for hiding this comment

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

Maybe safer as:

    try:
         import _manylinux
    except (ImportError, AttributeError):
          # Fall through to heuristic check below
          pass
    else:
          return bool(_manylinux.manylinux1_compatible)

Copy link
Member

Choose a reason for hiding this comment

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

No, this changes the semantics -- the reason it's written that way in the PEP is that it's legal to have a _manylinux module that doesn't define a manylinux1_compatible attribute. (Hence the AttributeError check.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason _manylinux is not vendored?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, _manylinux is basically a config file that may be added to python by a distributor, as a way of providing information about a particular python install to pip. It's not an actual module with, like, code in it. So vendoring it would defeat the purpose :-). See PEP 513 for details.

@njsmith
Copy link
Member

njsmith commented Feb 4, 2016

The right place is distutils, but distutils is stuck on the cpython release
cycle. And linux32 is great if you know you want it, but the problem with
this bug is that if you ever forget and run pip on an affected system, then
you'll silently get broken libraries installed.
On Feb 3, 2016 3:39 PM, "Robert T. McGibbon" [email protected]
wrote:

@njsmith https://github.com/njsmith do you think pip is really the
right place for that?

(It's also worth mentioning as a stopgap, that you can get the linux32
behavior by prefixing commands with linux32, for example running linux32
pip install...)


Reply to this email directly or view it on GitHub
#3446 (comment).

@njsmith
Copy link
Member

njsmith commented Feb 4, 2016

@rmcgibbo: Looks like you have some pep8 failures

@njsmith
Copy link
Member

njsmith commented Feb 4, 2016

Remaining Travis failures appear to just be PyPI being flaky.

@pfmoore
Copy link
Member

pfmoore commented Feb 4, 2016

I restarted the ones that had failed.

@rmcgibbo
Copy link
Member Author

rmcgibbo commented Feb 4, 2016

Okay, now that the travis checkmark is green, is there anything special I have to do for the "reviewable" hook?

"""
Test that manylinux1 is enabled on wide unicode linux_x86_64
"""
assert pep425tags.is_manylinux1_compatible() is True
Copy link
Member

Choose a reason for hiding this comment

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

why the is True ?


# "wide" Unicode mode is mandatory (always true on CPython 3.3+)
if sys.maxunicode <= 0xFFFF:
return False
Copy link
Member

Choose a reason for hiding this comment

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

I think the only change needed to handle this last round of PEP updates is to delete this unicode check

@njsmith
Copy link
Member

njsmith commented Feb 10, 2016

My bad -- you also need to remove the maxunicode test :-)

@xavfernandez xavfernandez added this to the 8.1 milestone Feb 11, 2016
@njsmith
Copy link
Member

njsmith commented Feb 11, 2016

@rmcgibbo: pep8 failures (and some spurious failures due to pypi flakiness)

@ogrisel
Copy link
Contributor

ogrisel commented Feb 21, 2016

I addressed the remaining CI failures in #3497.

@rmcgibbo
Copy link
Member Author

Thanks @ogrisel for picking this up while I finished my Ph.D. thesis. I'll close this and we can continue with your PR.

@rmcgibbo rmcgibbo closed this Feb 22, 2016
@rmcgibbo rmcgibbo deleted the pep-513 branch February 22, 2016 10:08
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants