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

python - build abi3 wheels in cibuildwheel #1206

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

K0lb3
Copy link

@K0lb3 K0lb3 commented Oct 5, 2024

This PR makes it possible to build abi3 wheels within a cibuildwheel environment.
To not effect the performance of version specific builds the C code changes are all guarded within preprocessor conditions.

This PR also modifies the setup.py, making it:

  • abi3 compatible
  • python 2 compatible again
  • simplifies the extension build code a bit

TODO:

  • add type stubs (currently don't get included in wheel)
  • fix abi3 setup.py build issue (C code works, setup.py still has some issues)

setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
MANIFEST.in Show resolved Hide resolved
@K0lb3 K0lb3 marked this pull request as ready for review October 5, 2024 23:32
@K0lb3
Copy link
Author

K0lb3 commented Oct 6, 2024

It might be worth discussing if the abi3 wheels should start at 3.6 as it's now, or 3.10, which would then allow using the Buffer interface in the abi3 wheels as well.

@eustas
Copy link
Collaborator

eustas commented Nov 21, 2024

Awesome, thanks. Will review soon.

setup.py Outdated
@@ -291,7 +266,8 @@ def build_extension(self, ext):
platforms=PLATFORMS,
classifiers=CLASSIFIERS,
package_dir=PACKAGE_DIR,
py_modules=PY_MODULES,
packages=[''],
package_data={'': ['__init__.py', 'brotli.py', 'brotli.pyi', 'py.typed']},
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 think .py files should be put in package_data which is for data files only, they should already be included when you list the packages. Also the empty package '' looks suspicious. And in fact setuptools throws a warning

WARNING: '' not a valid package name; please use only .-separated package names in setup.py

If you want to turn brotli from a single module into a package with a directory containing the extra data files, you need to create a new "brotli" sub-directory under ./python representing the package itself (with its own __init__.py).
Then inside the python/brotli/__init__.py, you would import all the symbols from the _brotli extension module so that when one does import brotli like usual the package's __init__ gets executed and the things defined in the extension module are available like before. The extension module then will be brotli._brotli under the new brotli package.

Does that make sense?

Copy link
Member

@anthrotype anthrotype Nov 21, 2024

Choose a reason for hiding this comment

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

actually, the file currently named "brotli.py" which wraps the extension module should be renamed __init__.py and placed under the new brotli package directory

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 don't fix this, then all the files get installed in the lib/pythonX.Y/site-packages folder directly without an enclosing package directory as it should be for valid python packages; even the empty __init__.py ends in there, or the bro.py script which we don't want to install at all.

Copy link
Author

Choose a reason for hiding this comment

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

You're right.
I was only trying to keep the structure as it is. I will make the relevant changes later today.

Assuming a normal directory for the .py files,
then what you said is absolutely correct.

In that case bro.py could actually be shipped as __main__ entry point of the module, so that it could be used as cli.
```python -m brotli {bro args}`` could then be used. It might not be that relevant for users, but since it's 0 effort and might be nice for tests.....

Copy link
Member

Choose a reason for hiding this comment

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

In that case bro.py could actually be shipped as main

sounds good to me.

Copy link
Author

Choose a reason for hiding this comment

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

Whoops, something got in between.
I made the relevant changes now.

The cli interface via python -m brotli might still require a little fix.

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'd love to not have to worry about building brotli wheels every year that a new python minor version gets out :)

This looks good, except for the issue I raised in my previous comments about the unnamed package without its own folder, which must be addressed before merge.

@K0lb3
Copy link
Author

K0lb3 commented Nov 26, 2024

Thanks for this! I'd love to not have to worry about building brotli wheels every year that a new python minor version gets out :)

The repo for building and publishing the wheels needs a PR as well to make this work in the end,
as pypi might otherwise block the upload due to already existing wheels of the same name.
Alternatively the cibuildwheel config can also be done within the pyproject.toml, and added to this PR.

e.g. like following which I use in one of my projects

[tool.cibuildwheel.linux]
archs = ["x86_64", "i686", "aarch64", "ppc64le", "s390x", "armv7l"]
build = "cp37-* pp3*"

[tool.cibuildwheel.macos]
archs = ["x86_64", "arm64"]
build = "cp37-macosx_x86_64 cp38-macosx_arm64 pp3*"

[tool.cibuildwheel.windows]
archs = ["AMD64", "x86", "ARM64"]
build = "cp37-win_amd64 cp37-win32 cp39-win_arm64 pp3*"

The archs are all written separately to ensure that cibuildwheel covers all possible ones instead of just the default ones.
pypy dosn't support abi wheels, so wheels for all version of it still have to be build.
Arm OSX earliest release python build is 3.8, therefore the version to be build for it is cp38.
Arm Windows starts at 3.9, so it's configured as cp39.

As brotli could also support 3.6 abi, the config above would have to be slightly changed, as armv7l linux support is only official for cp37...as far as I can recall.

Edit:
It's on purpose that the config has no universal(2) build, as pip will use the more specific one.
e.g. if pip finds universal2 and x86_64, then it will pick x86_64. Therefore there isn't really a point in universal builds imo.

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

Successfully merging this pull request may close these issues.

3 participants