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

ENH: delegate computing wheel tags to the packaging module #191

Closed
wants to merge 3 commits into from

Conversation

dnicolodi
Copy link
Member

@dnicolodi dnicolodi commented Nov 1, 2022

Use the wheel contents only to determine whether the wheel contains python ABI dependent modules or other platform dependent code.

Fixes #142, fixes #189, fixes #190.

@dnicolodi dnicolodi force-pushed the packaging branch 12 times, most recently from d9e7993 to 4285be3 Compare November 1, 2022 23:56
@dnicolodi dnicolodi force-pushed the packaging branch 2 times, most recently from 226c02b to 4d788b1 Compare November 2, 2022 09:24
@dnicolodi dnicolodi changed the title Delegate computing wheel tags to the packaging module ENH: delegate computing wheel tags to the packaging module Nov 2, 2022
@dimpase
Copy link

dimpase commented Nov 2, 2022

For testing purposes, it would be great to have this fix as a pip-installable package. Is it too much to ask?

@dnicolodi
Copy link
Member Author

dnicolodi commented Nov 2, 2022

pip install git+https://github.com/dnicolodi/meson-python.git@packaging should work.

You can use something like this:

[build-system]
requires = [
    "wheel",
    "meson >= 0.63.3",
    "meson-python @ git+https://github.com/dnicolodi/meson-python.git@packaging",
]
build-backend = "mesonpy"

in a pyproject.toml file.

git+https://github.com/mesonbuild/meson-python.git@pull/191/head should also work

@dimpase
Copy link

dimpase commented Nov 2, 2022

Thanks. I forgot that pip can install from git repos...

@dimpase
Copy link

dimpase commented Nov 2, 2022

I can confirm that with this fix I am able to install scipy 1.9.3 on an M1 macOS 13. Thanks!

@dimpase
Copy link

dimpase commented Nov 2, 2022

something funny with pypy tags, as far as I can see from failing tests here:

AssertionError: assert 'pp38' == 'pypy38_pp73'

@dnicolodi
Copy link
Member Author

Indeed, although it looks like a bug in wheel. The wheel filename parsing is confused by filenames in the form purelib_and_platlib-1.0.0-pp38-pypy38_pp73-manylinux_2_31_x86_64.whl while packaging is fine with it:

>>> import packaging.utils
>>> name, version, build, tags = packaging.utils.parse_wheel_filename('purelib_and_platlib-1.0.0-pp38-pypy38_pp73-manylinux_2_31_x86_64.whl')
>>> t = next(iter(tags))
>>> t.interpreter
'pp38'
>>> t.abi
'pypy38_pp73'
>>> t.platform
'manylinux_2_31_x86_64'

Although, I am not sure what is the canonical form for pypy tags.

@dnicolodi
Copy link
Member Author

Ah, no. Much simpler. I inverted interpreter and ABI tags in the test. The naming is confusing... Can someone please restart the tests?

@dnicolodi
Copy link
Member Author

Some other tests fail because bugs in the tests, namely they look for the string any anywhere in the wheel filename and that triggers on manylinux. However, this highlights the fact that packaging uses something like manylinux_2_31_x86_64 as platform tags while wheel usually generates something that looks like linux_x86_64. Is there a standard for these platform tags? I don't see why one version or the other should be preferred, except that we are not checking against the manylinux specification, thus putting that in the platform tag may be misleading. Should we simply use linux_$architecture?

@dnicolodi
Copy link
Member Author

Windows test were failing for an actual bug. Fixed. Can someone please kick the tests again?

@dnicolodi
Copy link
Member Author

I'm coming around on some of the objections. I had a better look at the API provided by packaging and, assuming that all functions not explicitly marked as private are to be considered public, not only the ones documented in the manual, we can get most of the things we need without re-implementing too much. I'll give it a try later today.

Looking for the string 'any' in the wheel filenames triggers when the
platform tag contains the string 'manylinux'. The ABI and platform
tags are anyhow verified a few lines above.
@dnicolodi
Copy link
Member Author

I've been looking into this some more and the main piece missing without re-implementing parts of packaging into meson-python is packaging.tags.interpreter_abi() see pypa/packaging#611 Unfortunately the bit and pieces necessary to implement this simple function are private and it may be better to do not rely on them. It really seem that packaging is not meant to provide the functionality we require, but I don't know of a better match. wheel would be the best candidate, but it does not have (yet) a stable API.

I see two possibilities: either continue to abuse the packaging API like in the approach in the current patch, or bite the bullet and start re-implementing the required pieces here, with the risk of the implementations diverging because of bugs or reinterpretations of the PEPs or new platforms appearing. The first approach is not that bad, but the second would be a step into providing meson-python functionality directly from Meson (because of the Meson's "no dependencies outside the standard library" policy).

I'll go with the first approach, for now.

@dnicolodi dnicolodi force-pushed the packaging branch 15 times, most recently from f777740 to a8aa6fe Compare November 11, 2022 09:07
Use the wheel contents only to determine whether the wheel contains
python ABI dependent modules or other platform dependent code.

Fixes mesonbuild#142, fixes mesonbuild#189, fixes mesonbuild#190.
@dnicolodi
Copy link
Member Author

dnicolodi commented Nov 11, 2022

I think the comments have all been addressed. This is ready for another round of review.

Despite there isn't in Meson yet an easy way to generate extension modules targeting the stable Python ABI, I added some synthetic tests checking that the handling of the stable ABI tag works as intended. Unfortunately, it turns out that, as I was fearing, the stable ABI tag in the extension modules filename is not only just advisory, it is not even supported on Windows: a CPython interpreter on windows does not load extension modules with filename suffix .abi3.pyd or anything else with abi3 in the extension filename suffix. Therefore, the meson-python handling of the stable ABI tag is "broken by design". I decided to keep the current logic in place, but a better solution has to be found if we want to support this use case. By the way, does anyone know of any project that builds against the stable ABI?

@dnicolodi dnicolodi requested a review from FFY00 November 11, 2022 09:19
@henryiii
Copy link
Contributor

henryiii commented Nov 11, 2022

That's correct, on Windows you are not supposed to add the .abi3 part of the extension, that's by design (why, I don't know). You also need to link against python3.dll instead of python3.X.dll when building. You can see some stable ABI users here: https://discuss.python.org/t/lets-get-rid-of-the-stable-abi-but-keep-the-limited-api/18458/11?u=henryiii

Most binding frameworks are unable to support ABI3, or have very limited support. pybind11 might start supporting it in Python 3.12, that will be the first version with enough in the Limited API to make it feasible to support. SWIG is slowly trying to support it.

@dnicolodi
Copy link
Member Author

Thanks @henryiii I also found pypa/cibuildwheel#78 with some information. In general there isn't much documentation about how this is to be used. I think the only build tools that supports this is setuptools and its documentation on this is very poor.

Clearly detecting the stable ABI from the extension modules filenames is not a solution, if it cannot be done on all platforms. Also, it seems that one of the goals is to have extension modules compatible with multiple Python versions, thus using the stable ABI wheel tag and the current Python interpreter version for the interpreter tag is not going to be enough. The only solution I see is a configuration parameter in pyproject.toml. Something like

[tool.meson]
python-api = "cp32"

As this is an existing separate issue and Meson does not have yet an easy way to produce stable ABI extensions, I would leave solving this issue to another PR.

@henryiii
Copy link
Contributor

henryiii commented Nov 11, 2022

Given the mention of cryptography on that issue, I'm guessing Maturin supports it? I've got initial support in scikit-build-core, too, though FindPython from CMake doesn't support it so I'm only testing it on Windows. Yes, you can't auto-detect it, and you need to tell the system what your minimum version is for the tagging. In scikit-build-core:

[tool.scikit-build]
tags.py-abi = "cp32-abi3"

Would target ABI3. It can also be used to build a "Pythonless" wheel (one that doesn't use Python, like ninja, cmake, or clang-format, or just has things you load via ctypes):

[tool.scikit-build]
tags.py-abi = "py3-none"

SKBUILD_SOABI is set to "abi3" on non-Windows if this is set to *-abi3.

(I wonder if api-abi would have been better as a name)

@dnicolodi
Copy link
Member Author

I'm a bit fuzzy on the connection between the limited API and the stable ABI. I thought that using the limited API always produces a stable ABI extension module. Doesn't it?

@henryiii
Copy link
Contributor

henryiii commented Nov 11, 2022

Yes. The reason I settled on requesting both API and ABI tags was to have a way to specify binary tags that do not depend on Python at all; currently I think those are just as if not more common than limited API / stable ABI extensions. You could do just the api tag and cp32 would be limited ABI and py3 or py2.py3 or py37 would be a wheels that doesn't depend on the C api. I was a little worried about that being unclear; setting py37 or cp37 here for example means you'd support anything 3.7+, but it looks like you are locking a specific version of Python instead. Maybe it's fine? (scikit-build-core is still experimental and can be changed, if we have similar settings that would be nice)

CPython extensions on Linux are allowed to have just the .so suffix, as well, so I don't think automatic discovery is all that safe, though if Meson makes it harder to produce SOABI-missing .so files, maybe it's safe enough? (CMake makes it very easy to produce, so it's not safe to use in scikit-build-core). PyPy does not accept files without SOABI (and, to make things fun, it reports the wrong SOABI from sysconfig, so you have to get SOABI from EXT_SUFFIX).

@dnicolodi
Copy link
Member Author

I think #202 is a better approach in the long run.

@dnicolodi dnicolodi closed this Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
8 participants