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

Don't append "m" ABI flag in Python 3.8 #6874

Merged
merged 1 commit into from Aug 17, 2019
Merged

Don't append "m" ABI flag in Python 3.8 #6874

merged 1 commit into from Aug 17, 2019

Conversation

@rdb
Copy link
Contributor

rdb commented Aug 14, 2019

In Python 3.8, the m flag is no longer appended to the SOABI, since the ABI is longer influenced by enabling pymalloc. This PR should fix the ability to install Python 3.8 wheels on Windows. In Python 3.8, sys.abiflags is an empty string.

https://docs.python.org/dev/whatsnew/3.8.html#build-and-c-api-changes
https://bugs.python.org/issue36707

See also pypa/wheel#303

@chrahunt

This comment has been minimized.

Copy link
Member

chrahunt commented Aug 15, 2019

Hello, thank you for submitting a PR! We're tracking this activity over in pypa/packaging#172. This fix should target that library and then ideally it would get pulled into both pip and wheel.

IIUC, libraries built in release mode can get used in a debug interpreter in 3.8+, but not vice-versa. @vstinner describes the intended behavior here. So we would probably want to return a list of applicable tags (that is, both dm and m on a debug build), rather than remove them entirely. This would also need tests.

@rdb

This comment has been minimized.

Copy link
Contributor Author

rdb commented Aug 15, 2019

@chrahunt Note that this PR doesn't just cover debug but also the removal of the m ABI flag. This is unrelated to pypa/packaging#172 (and the cited e-mail still mentions the "m" flag because it predates that change). See bpo-36707. That change needs to get into the ensurepip shipped with Python 3.8 or it will be impossible to install properly formed Windows wheels in Python 3.8.

You're right that the addition of the d flag is now dependent on Py_TRACE_REFS instead of Py_DEBUG. I will change the PR accordingly. However, debug/release compatibility is out of scope of this PR, so targeting pypa/packaging#172 is not right.

Thank you for considering this.

@rdb rdb force-pushed the rdb:patch-1 branch Aug 15, 2019
@rdb rdb changed the title Don't append d/m ABI flags in Python 3.8 Don't append "m" ABI flag in Python 3.8 Aug 15, 2019
@rdb

This comment has been minimized.

Copy link
Contributor Author

rdb commented Aug 15, 2019

@chrahunt I have opened issue pypa/packaging#180 to track this in that library, but not before realising that packaging.tags does not suffer from this bug; it returns the correct cp38 ABI instead of cp38m on Windows. This is because packaging.tags uses a different function that does not assume PyMalloc as the default.

I have changed the scope of this PR to only focus on the m tag to ensure that the discussion about debug compatibility does not confuse efforts to get this critical fix in.

I've updated the unit test.

@chrahunt

This comment has been minimized.

Copy link
Member

chrahunt commented Aug 16, 2019

You're right, this is unrelated to the change I linked before, sorry about that.

I made issue #6885 describing the problem in more detail. Can you please update the news file to reflect that issue?

@pradyunsg, assuming this looks OK and given the Python 3.8 release schedule (link), would this need to be in 19.2.3 or 19.3 to get into 3.8.0's ensurepip?

@rdb rdb force-pushed the rdb:patch-1 branch to 48109b1 Aug 16, 2019
@rdb

This comment has been minimized.

Copy link
Contributor Author

rdb commented Aug 16, 2019

Thank you. Updated the bugfix number.

@pfmoore pfmoore merged commit 54eedbd into pypa:master Aug 17, 2019
22 checks passed
22 checks passed
Linux Build #20190816.7 succeeded
Details
Linux (Package) Package succeeded
Details
Linux (Test Primary Python27) Test Primary Python27 succeeded
Details
Linux (Test Primary Python36) Test Primary Python36 succeeded
Details
Linux (Test Secondary Python35) Test Secondary Python35 succeeded
Details
Linux (Test Secondary Python37) Test Secondary Python37 succeeded
Details
Windows Build #20190816.7 succeeded
Details
Windows (Package) Package succeeded
Details
Windows (Test Primary Python27-x86) Test Primary Python27-x86 succeeded
Details
Windows (Test Primary Python37-x64) Test Primary Python37-x64 succeeded
Details
Windows (Test Secondary Python35-x86) Test Secondary Python35-x86 succeeded
Details
Windows (Test Secondary Python36-x86) Test Secondary Python36-x86 succeeded
Details
Windows (Test Secondary Python37-x86) Test Secondary Python37-x86 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
macOS Build #20190816.7 succeeded
Details
macOS (Package) Package succeeded
Details
macOS (Test Primary Python27) Test Primary Python27 succeeded
Details
macOS (Test Primary Python36) Test Primary Python36 succeeded
Details
macOS (Test Secondary Python35) Test Secondary Python35 succeeded
Details
macOS (Test Secondary Python37) Test Secondary Python37 succeeded
Details
news-file/pr News files updated and/or change is trivial.
Details
@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Aug 17, 2019

@rdb Thanks for spotting this and providing a fix!

warn=(impl == 'cp')):
warn=(impl == 'cp' and
sys.version_info < (3, 8))) \
and sys.version_info < (3, 8):

This comment has been minimized.

Copy link
@cjerdonek

cjerdonek Aug 17, 2019

Member

Question: is there a reason the sys.version_info < (3, 8) has to go after the call to get_flag()? It seems like this would be simpler with it before because then the sys.version_info < (3, 8) wouldn't need to be duplicated.

This comment has been minimized.

Copy link
@pfmoore

pfmoore Aug 17, 2019

Member

At a quick glance, that sounds like a reasonable tidy-up. A follow-up PR (as I just merged this as it stands) to clean this up would seem reasonable to me.

This comment has been minimized.

Copy link
@cjerdonek

cjerdonek Aug 17, 2019

Member

Sounds good. (And the same type of change can be made in the following conditional as well.)

This comment has been minimized.

Copy link
@rdb

rdb Aug 17, 2019

Author Contributor

I agree, I was just following the example of the other version check below.

This comment has been minimized.

Copy link
@cjerdonek

cjerdonek Aug 17, 2019

Member

I just filed this as PR #6889.

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Aug 17, 2019

@rdb Thanks a lot for this PR!


@chrahunt Good idea!

Let's do a 19.2.3 with just this change added and update the ensurepip module.

I see that the beta is on 26th. I'll try to get to both of those tasks by ~24,25 then.

@rdb

This comment has been minimized.

Copy link
Contributor Author

rdb commented Aug 17, 2019

Thanks for merging!

@pradyunsg Though it didn't appear to be important in my testing, setuptools.pep425tags also has the bugged version of get_abi_tag(), so I've replicated this PR there as well. In case it is used somewhere important after all, it might be good to get an updated setuptools into ensurepip as well.

rdb added a commit to panda3d/panda3d that referenced this pull request Aug 17, 2019
A proper fix for this has been merged in pip already (pypa/pip#6874), so this hack will only exist until pip 19.2.3 is out.
pradyunsg added a commit to pradyunsg/pip that referenced this pull request Aug 25, 2019
Don't append "m" ABI flag in Python 3.8
@lock lock bot added the S: auto-locked label Sep 16, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.