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.

Sign up for GitHub

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

Automatically set compiler flags to target PEP 384 Python limited API #26

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Automatically set compiler flags to target PEP 384 Python limited API #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6547724
Automatically set compiler flags to target PEP 384 Python limited API
lpsinger Feb 9, 2021
cdfeeac
Update extension_helpers/_setup_helpers.py
lpsinger Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions docs/using.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,20 @@ To use this, you should modify your ``setup.py`` file to use
Note that if you use this, extension-helpers will also we create a
``packagename.compiler_version`` submodule that contain information about the
compilers used.

Python limited API
------------------

Copy link
Member

Choose a reason for hiding this comment

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

In a similar vein to my other comment, it would be nice to explain when using this is appropriate and when it isn't as I expect most people don't even know this exists (I didn't) and why they would want to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right! It's not a well-known feature, although it seems to be well supported by pip. Part of the aim of this PR is to raise awareness by making it easy for Astropy affiliated packages to opt in.

Your package may opt in to the :pep:`384` Python Limited API so that a single
binary wheel works with many different versions of Python on the same platform.
To opt in to the Python Limited API, add the following standard setuptools
option to your project's ``setup.cfg`` file::

[bdist_wheel]
py_limited_api = cp36

Here, ``36`` denotes API compatibility with Python >= 3.6. Replace with the
lowest major and minor version number that you wish to support.

The ``get_extensions()`` functions will automatically detect this option and
add the necessary compiler flags to build your extension modules.
46 changes: 45 additions & 1 deletion extension_helpers/_setup_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"""

import os
import re
import sys
import shutil
import subprocess
Expand All @@ -15,11 +16,36 @@
from setuptools import find_packages
from setuptools.config import read_configuration

from ._distutils_helpers import get_compiler
from ._distutils_helpers import get_compiler, get_distutils_option
from ._utils import import_file, walk_skip_hidden

__all__ = ['get_extensions', 'pkg_config']

_PEP_384_WARNING = """\
Not targeting PEP 384 limited API. You can build one binary wheel for each \
platform to support all Python versions by adding the following to your \
project's setup.cfg file:

[bdist_wheel]
py_limited_api = cp36

Replace '36' with the lowest Python major and minor version that you wish to \
support.
"""


def _abi_to_version_info(abi):
match = re.fullmatch(r'^cp(\d)(\d+)$', abi)
if match is None:
return None
else:
return int(match[1]), int(match[2])


def _version_info_to_version_hex(major=0, minor=0, micro=0,
releaselevel=0, serial=0):
return f'0x{major:0>2}{minor:0>2}{micro:0>2}{releaselevel:0>2}{serial:0>2}'
Comment on lines +45 to +47
Copy link
Member

@ConorMacBride ConorMacBride May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Based on the Python API and ABI Versioning docs I think this would need to be:

Suggested change
def _version_info_to_version_hex(major=0, minor=0, micro=0,
releaselevel=0, serial=0):
return f'0x{major:0>2}{minor:0>2}{micro:0>2}{releaselevel:0>2}{serial:0>2}'
def _version_info_to_version_hex(major=0, minor=0):
"""Returns a PY_VERSION_HEX for {major}.{minor).0"""
return f'0x{major:02x}{minor:02x}0000'

Currently for cp310, {10:0>2} would give 10 instead of 0a, and the release level should be f for a final release. (Edit: according to this python doc setting the release level to 0 should be fine.)



def get_extensions(srcdir='.'):
"""
Expand Down Expand Up @@ -85,6 +111,24 @@ def get_extensions(srcdir='.'):
[os.path.join(main_package_dir, '_compiler.c')])
ext_modules.append(ext)

use_limited_api = False
abi = get_distutils_option('py_limited_api', ['bdist_wheel'])
if abi:
version_info = _abi_to_version_info(abi)
if version_info:
Copy link
Member

Choose a reason for hiding this comment

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

This isn't raising an error/warning if py_limited_api is specified but does not conform to the pattern, however, pypa/wheel will raise an error in this case so this should be fine.

lpsinger marked this conversation as resolved.
Show resolved Hide resolved
use_limited_api = True

if use_limited_api:
log.info(
'Targeting PEP 384 limited API supporting Python >= %d.%d',
*version_info)
version_hex = _version_info_to_version_hex(*version_info)
for ext in ext_modules:
ext.py_limited_api = True
ext.define_macros.append(('Py_LIMITED_API', version_hex))
lpsinger marked this conversation as resolved.
Show resolved Hide resolved
else:
log.warn(_PEP_384_WARNING)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the limitations of the limited API really, but does numpy compile against it? I am just wondering if this warning is too keen, i.e. if most of our users (scientific python packages with numpy C dep) are likely to be able to make use of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Downstream packages that compile against and link with Numpy can use the limited API. I maintain one such package. Here are the wheels: https://pypi.org/project/ligo.skymap/#files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The major limitation is that Cython support for the limited API is very, very new: cython/cython#3223

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you could make astropy core compile with this set 🤔 that would certainly be a good indicator that most things could use it.

Thanks for the extra info.

Copy link
Member

Choose a reason for hiding this comment

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

So just to make sure I understand, people will see the warning if they don't opt-in to this? If so I wonder if we should perhaps not do that, or at least make it clear to people how to get rid of the warning if they don't want to opt in to the limited API?


return ext_modules


Expand Down