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

refactor _generic_api to use EXT_SUFFIX #607

Merged
merged 7 commits into from
Dec 23, 2022
Merged

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Nov 2, 2022

Fixes #606 where PyPy would like to change its non-compliant SOABI tag (pypy36-pp73) to a compliant one (pypy39-pp73-x86_64-linux-gnu).

The basic code is taken from wheel.bdist_wheel's version.
changed to use EXT_SUFFIX instead of SOABI.

A few other changes:

  • return a list instead of a iterator, since all uses of the function converted to a list anyway
    - add a fallback for older cpython to call _cpython_abis, which means passing in PythonVersion
  • clarify the typical value of interpreter_name, since it actually is the 'cp' or 'pp' short name.

If I was starting over, PyPy's ABI tag would be pp39_73 rather than pypy39_73 to save two letters. Maybe someday ...

Edit (11-Nov-2022): refactored to use EXT_SUFFIX

@uranusjr
Copy link
Member

uranusjr commented Nov 2, 2022

Does this not need to still support the old format so currently existing wheels can still work?

@mattip
Copy link
Contributor Author

mattip commented Nov 2, 2022

Does this not need to still support the old format so currently existing wheels can still work?

Yes. The code should be backward compatible: the names should not change.

@mattip
Copy link
Contributor Author

mattip commented Nov 2, 2022

Does this not need to still support the old format so currently existing wheels can still work?

I added tests to make sure that on PyPy the current SOABI and the future SOABI give the same value as they currently do. There should be no change here for CPython.

@henryiii
Copy link
Contributor

henryiii commented Nov 2, 2022

According to PEP 3149, EXT_SUFFIX and SOABI are equivalent, minus the extension. If they are not, it's a bug - and historically, EXT_SUFFIX is much more likely to be right:

For future reference going forward, sysconfig.get_config_var('EXT_SUFFIX') is the recommended way to spell this for CPython3.8+ and all current versions of PyPy. See CPython issues https://bugs.python.org/issue39825 (for windows) and https://bugs.python.org/issue42604 (for FreeBSD and AIX).

There's also a chance SOABI will be blank on Windows, while EXT_SUFFIX is safe. Why not just use EXT_SUFFIX, then always strip the extension and the platform part? Then you don't need the conditional logic to see if there is a platform part.

@mattip
Copy link
Contributor Author

mattip commented Nov 2, 2022

Where is the quote from? I don't see it in the linked PEP Whoops - you were quoting me on the cmake issue https://gitlab.kitware.com/cmake/cmake/-/issues/21070#note_876947.

@henryiii
Copy link
Contributor

henryiii commented Nov 2, 2022

Yes, sorry, there's a link just below to the same issue, one comment down.

@mattip
Copy link
Contributor Author

mattip commented Nov 2, 2022

I wonder where I got that information (that EXT_SUFFIX is to be preferred over SOABI). It seems that in the posix configure script SOABI comes first. I do agree that if they are not equivalent (plus-minus a suffix) it is a bug, so it would seem for the purposes of this PR that SOABI is easier, no need to remove the suffix.

@henryiii
Copy link
Contributor

henryiii commented Nov 2, 2022

I would assume it's because EXT_SUFFIX is used when loading the module, so it has to be correct. While SOABI is misused here, so any implementation (PyPy, and maybe all others?) had to have incorrect SOABI's in the past to make packaging tags work. Are there other implementations to check? Pyodide is correct, but it doesn't use Pip, and it counts as cpython, so there's no issue. I tried to install IronPython 3.4, but have no idea where it went after installing it. ¯\_(ツ)_/¯

Also the warning for using SO is:

DeprecationWarning: SO is deprecated, use EXT_SUFFIX

So it's pointing to EXT_SUFFIX as the replacement, not SOABI.

@mattip
Copy link
Contributor Author

mattip commented Nov 7, 2022

@henryiii is there something to change in this PR to answer your comment?

@mattip
Copy link
Contributor Author

mattip commented Nov 7, 2022

I wouldn't want to switch SOABI for EXT_SUFFIX without tacit approval from the package maintainers.

@henryiii
Copy link
Contributor

henryiii commented Nov 7, 2022

My question was why not use EXT_SUFFIX? It is much more likely to be right, and it's suggested as the replacement for SO anyway, even though SOABI is a closer match. Then you don't have to do special processing to check to see if it's a malformed SOABI, you can just strip the extension suffix & leading dot and now you know it's a valid SOABI.

@henryiii
Copy link
Contributor

henryiii commented Nov 7, 2022

Without approval from packaging maintainers, the current version is better than the status quo, so no unless packaging maintainers chime in. Setuptools/distutils rely on EXT_SUFFIX, like in https://github.com/pypa/setuptools/blob/a2c4c578e4921be7670448fc1cbf3f81792d86e4/setuptools/_distutils/command/build_ext.py#L710.

@brettcannon
Copy link
Member

I wouldn't want to switch SOABI for EXT_SUFFIX without tacit approval from the package maintainers.

If it leads to better results I'm up for trying it out. Is there anyone else to loop in here and ask how they might be affected?

@henryiii
Copy link
Contributor

henryiii commented Nov 9, 2022

Are there any other Python implementations that might be using this? I couldn't figure out how to find IronPython 3.4 after installing it (and besides, that currently maxes out at Python 3.4, this would only affect 3.7+), and pyodide doesn't use this AFAICT (it "is" CPython in a sense).

@mattip mattip changed the title refactor _generic_api to accept PEP 3149 SOABI tags refactor _generic_api to use EXT_SUFFIX Nov 10, 2022
@mattip
Copy link
Contributor Author

mattip commented Nov 10, 2022

I am refactoring this to use EXT_SUFFIX. I changed the description and title to match.


ext_suffix = _get_config_var("EXT_SUFFIX", warn=True)
if not isinstance(ext_suffix, str) or ext_suffix[0] != ".":
raise SystemError("invalid sysconfig.get_config_var('EXT_SUFFIX')")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this correct or would the reviewers prefer the function return [] for an invalid EXT_SUFFIX (which should never happen)? I prefer exceptions when reality clashes with my expectations, but maybe that is too optimistic?

Copy link
Member

Choose a reason for hiding this comment

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

If you can't function w/o the information then an exception makes sense (have not thought about which one).

@mattip mattip force-pushed the generic_abi branch 4 times, most recently from e6cd1cc to e606666 Compare November 11, 2022 08:35
@mattip
Copy link
Contributor Author

mattip commented Nov 12, 2022

CI is passing.

@isuruf
Copy link

isuruf commented Nov 12, 2022

cc @timfel from GraalPy

@brettcannon
Copy link
Member

Looks like @timfel from GrallPy is the only person anyone could think of, so let's give them a chance to comment and then we can go from there.

@timfel
Copy link

timfel commented Nov 15, 2022

I'm happy with this for GraalPy.

@brettcannon
Copy link
Member

Maybe someday sysconfig will could grow new functions python_425_tag, abi_425_tag and platform_425_tag to report the appropriate PEP 425 tags. Then the interpreter would be self-reporting and this would all become redundant.

Please feel free to open an issue at python/cpython if we want to generalize some things, but moving all tag code upstream might not be doable as I can see some folks getting upset with embedding that much packaging knowledge into the stdlib (but we won't know until we ask 🤷).

@brettcannon
Copy link
Member

So, are we all in agreement on this PR? Last chance to speak up before I begin reviewing for technical/code reasons and not on results reasons.

@dnicolodi
Copy link

A version of meson-python has been released incorporating changes in the spirit of this PR. I don't think we will go back on these changes. Note that this means that SciPy (and other less prominent packages) is now uninstallable on GraalPy, unless meson-python is pinned to a previous version or other tricks are applied.

@brettcannon
Copy link
Member

@dnicolodi

Note that this means that SciPy (and other less prominent packages) is now uninstallable on GraalPy

... until the PR is merged and released, correct?

@dnicolodi
Copy link

Note that this means that SciPy (and other less prominent packages) is now uninstallable on GraalPy

... until the PR is merged and released, correct?

Correct. But that's going to be a while.

@brettcannon brettcannon mentioned this pull request Dec 9, 2022
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
@mattip
Copy link
Contributor Author

mattip commented Dec 10, 2022

Squashed review commits and rebase off main to clear merge conflict

@pradyunsg pradyunsg added this to the 22.1 / 23.0 milestone Dec 13, 2022
@pradyunsg
Copy link
Member

@brettcannon I'm gonna defer to you on this one completely. :)

@brettcannon brettcannon self-requested a review December 13, 2022 22:02
@brettcannon brettcannon merged commit 30554f5 into pypa:main Dec 23, 2022
@pradyunsg
Copy link
Member

@mattip @brettcannon Any suggestions on what would be a good changelog entry for this?

@mattip
Copy link
Contributor Author

mattip commented Dec 24, 2022

"Refactor _generic_api to use EXT_SUFFIX instead of SOABI"

@pradyunsg
Copy link
Member

Hmm... I'm not sure that means much for end users, and mentioning an internal function name in the changelog isn't particularly meaningful either...

What's the user-facing behaviour change here?

@mattip
Copy link
Contributor Author

mattip commented Dec 24, 2022

Hopefully this change has absolutely no user facing changes. It is a code refactor to make the code more robust to the single-source of truth (EXT_SUFFIX) instead of using aliases (SOABI) that may be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tags._generic_abi should use only the first part of the SOABI
8 participants