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: fix support for Python limited API / stable ABI wheels #451

Merged
merged 6 commits into from
Aug 25, 2023

Conversation

dnicolodi
Copy link
Member

Meson gained support for building Python extension modules targeting the Python limited API, therefore it is time for meson-python to properly support tagging the build wheels as targeting the stable ABI.

Unfortunately there isn't a reliable and cross-platform way to detect when extension modules are build for the limited API. Therefore, we need to add an explicit "limited-api" configuration option in the [tool.meson-python] section in pyproject.toml.

@dnicolodi
Copy link
Member Author

Documentation is still missing.

@dnicolodi dnicolodi force-pushed the limited-api branch 2 times, most recently from 19e40fd to 48e87a0 Compare August 15, 2023 20:47
@dnicolodi dnicolodi changed the title Fix support for Python limited API / stable ABI wheels ENH: fix support for Python limited API / stable ABI wheels Aug 15, 2023
@dnicolodi dnicolodi force-pushed the limited-api branch 8 times, most recently from ab0d8b3 to 4700a3f Compare August 15, 2023 23:52
@dnicolodi
Copy link
Member Author

I've added the documentation.

@rgommers rgommers added the enhancement New feature or request label Aug 23, 2023
@rgommers rgommers added this to the v0.14.0 milestone Aug 23, 2023
Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @dnicolodi, this looks quite good and this is a very nice new feature.

My main question is about the added test case: it seems to demonstrate that the limited_api setting in pyproject.toml cannot be set correctly if there is a build option which has the effect of toggling between the limited API and the regular C API. This should be rare, but it is conceivable.

In such a case, I think the answer is that the setting in pyproject.toml should be overridden via a CLI flag in order to not have the build fail, so something like:

pip install . -C-Dextra=true -C--no-limited-api

However, we don't have a flag like --no-limited-api that meson-python will use to override the pyproject.toml setting. Do you think that's needed, or can it be left for later because no one needs this, and we may never need it? Or is the answer to just patch pyproject.toml?

tests/packages/limited-api/meson.build Outdated Show resolved Hide resolved
tests/packages/limited-api/meson.build Outdated Show resolved Hide resolved
@dnicolodi
Copy link
Member Author

The test is somehow a contrived example that I clobbered together to do not have to replicate the package for the two test cases. I don't think doing something like that in a real package is realistic. I also think that the use of the limited API is still relatively rare, thus I'll favor keeping this simple for now and add features if the need arise.

I have the impression that having Python packages with configurable options and components is a rare exception, as the tooling for Python packaging does not make that easy (for example, there is no way to distinguish between versions of the same package built with different options). I think, in the context of Python packages, Meson options are almost exclusively used for enabling the build (ie, pointing at external dependencies, or switching between alternative implementations used on different platforms) more than enabling or disabling features.

I think that using or not the limited API is a choice of the package developers. However, Meson has a configuration options to disable the use of the limited API. One thing we could do is to check whether the option is passed to Meson, and override the tool.meson-python.limited-api setting if it is. But I would implement it only if someone asks for it.

@rgommers
Copy link
Contributor

I also think that the use of the limited API is still relatively rare, thus I'll favor keeping this simple for now and add features if the need arise.
...
But I would implement it only if someone asks for it.

+1, good to keep it simple.

@eli-schwartz
Copy link
Member

For a bit of context, during the development of the limited API support in meson, o recommended the addition of that option toggle based purely on the notion that it would be interesting to ISVs like a Linux distro that don't use wheels except as an unavoidable side effect of build backends requiring them. They manually construct their own distro package manager archive with fully resolved contents pointing to a exact python version and rebuild from source regardless of "limited API or regular extensions", so disabling the limited API define is a free optimization.

What are the downsides of an inaccurate wheel tag for purely local builds with an immediate install?

@rgommers
Copy link
Contributor

Ah, I also noticed for the first time that meson_options.txt has been renamed to meson.options (since Meson 1.1, as noted in https://mesonbuild.com/Build-options.html#build-options).

What are the downsides of an inaccurate wheel tag for purely local builds with an immediate install?

As long as they are guaranteed to stay local, nothing I guess. But producing -abi3 tags when the contents don't match that seems fishy. Sooner or later someone is going to upload them anyway.

@eli-schwartz
Copy link
Member

Hmm, then maybe you can just check the value of that setting from the json introspection files created in the builddir?

@dnicolodi
Copy link
Member Author

dnicolodi commented Aug 23, 2023

When building wheel for the stable API, meson-python checks that the extension modules filenames suffixes actually match what is expected for the stable ABI. Therefore not setting tool.meson-python.stable-api when using the stable API results in wheel with too specific tags, but the other way around results in an error (this is what one of the tests I added checks).

The value of the option is easy enough to check. The hard part is the UI. How should the thing be presented to the user? In other words, if tool.meson-python.stable-api is true, and -Dpython.allow_limited_api=false, what should we do? We can silently fall back to the regular ABI tag, but it may be surprising.

@eli-schwartz
Copy link
Member

I guess that depends on how you view the setting. Is it:

  • a requirement that meson-python had better give you one "or else"
  • an informational hint that says "it's difficult to tell whether abi3 was actually used, so in case you can't tell -- be aware this package does use that"

What I understood from:

Unfortunately there isn't a reliable and cross-platform way to detect when extension modules are build for the limited API. Therefore, we need to add an explicit "limited-api" configuration option

is that you want the meson-python option to act as a hint, not a requirement. Therefore if the meson option -Dpython.allow_limited_api=false is set, you'd want it to take priority, and silently fall back.

@dnicolodi dnicolodi force-pushed the limited-api branch 4 times, most recently from 2fa1b53 to c36d14c Compare August 23, 2023 14:29
@dnicolodi
Copy link
Member Author

I've implemented it exactly like that now, and updated the documentation.

This mostly serves the purpose of testing the limited API support.
The cast was anyhow to an incorrect type.
Meson gained support for building Python extension modules targeting
the Python limited API, therefore it is time for meson-python to
properly support tagging the build wheels as targeting the stable ABI.

Unfortunately there isn't a reliable and cross-platform way to detect
when extension modules are build for the limited API. Therefore, we
need to add an explicit "limited-api" configuration option in the
[tool.meson-python] section in pyproject.toml.
Group similar tests together and use similar constructs to skip tests
on platforms not supporting the tested features.  This made it
evident that we are skipping a valid test on macOS.  Fix that.
@rgommers
Copy link
Contributor

The Cygwin CI job isn't happy with the new LIB_SUFFIX:

    LIB_SUFFIX = {
E   KeyError: 'CYGWIN_NT-10.0-20348'

@dnicolodi
Copy link
Member Author

Cygwin is always funny, and given this result, I don't know what platform.system() is actually good for. It seems that we should standardize on using sys.platform() instead, which since a few Python releases actually has a well specified value for all major platforms.

Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

All green now and looks great, in it goes. Thanks @dnicolodi!

@rgommers rgommers merged commit b94ddbd into mesonbuild:main Aug 25, 2023
34 checks passed
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
Development

Successfully merging this pull request may close these issues.

3 participants