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

Package C extension to wheel #225

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

Conversation

theirix
Copy link

@theirix theirix commented Apr 28, 2024

Existing published wheels do not include the native C extension inside_polygon_ext. The default distribution could be quite slow if the library consumer doesn't install Clang, GCC, and ffi-dev to build dependencies. To clarify, it doesn't relate to Numba support.

Added an auditwheel check to the GitHub Actions check extension compatibility with manylinux specification. It's probably needed to adjust it based on auditwheel check.

How to verify check C extension support: assert(TimezoneFinder.using_clang_pip()). Checked it on a local project with a private PyPi repository.

@jannikmi
Copy link
Owner

Thanks for your contribution.

I have some questions:

I see some remaining TODOs:

  • separate build test job (independent of the publish/deploy job) in the GHA workflow
  • add Changelog entry
  • bump package version number

@theirix
Copy link
Author

theirix commented Apr 29, 2024

Thank you for the review.

  1. The binary files are packaged to the resulting binary wheel during poetry build --no-interaction in the Deploy workflow. If the proposed string is omitted, only source files will be included.

  2. Speaking of auditwheel, it's not enough to rely only on exit code since the diagnostics are not machine readable. So the repair step is usually needed.

  3. Since current binary build is based on ubuntu-latest, the result build won't be manylinux-compatible. I've built my version against quay.io/pypa/manylinux2014_x86_64 docker image which contains many prebuilt python interpreters, so I was able to produce per-Python version wheels like timezonefinder-6.5.0.post100-cp311-cp311-manylinux_2_17_x86_64.whl for 3.11.

  4. Test and deployment workflows should be separated. To isolate complexity of auditwheel, repairing wheels, multiple python versions and auto-testing them, I suggest to use a PyPa-provided solution cibuildwheel which allows to add a GHA build step wrapped in build matrix to automatically produce multiple binary wheels. A good example is here. I can draft such workflow in this PR.

@jannikmi
Copy link
Owner

@adamchainz @ringsaturn @joshuadavidthomas Can anyone jump in an help me out here? I am currently too busy to read into this, but I don't want to leave this valid PR pending.
Thanks a lot

@ringsaturn
Copy link
Contributor

@adamchainz @ringsaturn @joshuadavidthomas Can anyone jump in an help me out here? I am currently too busy to read into this, but I don't want to leave this valid PR pending. Thanks a lot

Thanks for reaching out, however I'm too busy to handle this. I suggest to get in touch with https://github.com/hugovk who help ujson's development a lot ultrajson/ultrajson#343.

@theirix
Copy link
Author

theirix commented May 29, 2024

I can make an initial implementation of cibuildwheel next week, but it will be complicated to test it properly.

@joshuadavidthomas
Copy link
Contributor

@adamchainz @ringsaturn @joshuadavidthomas Can anyone jump in an help me out here? I am currently too busy to read into this, but I don't want to leave this valid PR pending. Thanks a lot

@jannikmi Appreciate the ask, and I wish I could lend a hand, but I have only ever worked with pure Python packages and have never written a line of C code in my life. I'm not sure how much help I can be. 🫤

@jannikmi
Copy link
Owner

I can make an initial implementation of cibuildwheel next week, but it will be complicated to test it properly.

Thanks. I am sorry this topic is too far from my area of expertise that I cannot be of much help.

What would you need to test the changes?

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.

None yet

4 participants