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

move to Cibuildwheel workflow #274

Merged
merged 109 commits into from
Jan 27, 2024

Conversation

hendrikmuhs
Copy link
Contributor

switch the build system for python to cibuildwheel. cibuildwheel makes it easier to build python wheels on different platforms and python versions. The switch hopefully makes our life a little easier and lets us concentrate on the more important things.

With the switch we get:

  • builds for mac on arm
  • a new target: musllinux
  • more pypy targets

In addition to the change I implemented the following additions in this PR:

  • use of ccache per platform
  • publishing of artefacts as part of the build (for testing PRs by installing the wheel locally)
  • various toolchain updates, including enabling dependabot

superseds #269

@hendrikmuhs
Copy link
Contributor Author

The 1st run was really slow, over 3 hours for linux on arm builds. Those run on QEMU (emulator). I've disabled testing on all but the python 3.12 image. The bottleneck seems to be compiling. CCache wasn't active for the 1st run by design, lets see if the 2nd is faster. Unfortunately it only helps for musllinux as manylinux images miss ccache support.

It is possible to speed up CI by splitting the workflow into more tasks, e.g. per python version or maybe <3.10, >= 3.10 or many and musl (but that only exists on linux).

The other problem with this PR: The mac on arm binaries don't work yet, it seems that the C++ part needs to respect cross compilation, similar to scikit which fixed this problem in scikit-build/scikit-build#530, scikit-build/scikit-build#555, scikit-build/scikit-build#556

@hendrikmuhs hendrikmuhs marked this pull request as ready for review January 21, 2024 09:50
Copy link
Member

@narekgharibyan narekgharibyan left a comment

Choose a reason for hiding this comment

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

LGTM, just had a few suggestions for setup.py.

Btw, it seems you are trying to enforce 80 chars line limit ?
I guess it is somewhat short nowadays, but if you think we should have it, then maybe better we have something like .editorconfig ?
Also seems we have lot of other code, that doesn't comply with it.

fwiw, I had no chance to work with Cibuildwheel before, but am glad we are using something more modern, that should make our life better.

python/setup.py Outdated Show resolved Hide resolved
python/setup.py Outdated Show resolved Hide resolved
@hendrikmuhs
Copy link
Contributor Author

@narekgharibyan No intention to enforce a character limit. I just switched to a new box, used vscode and let it format the file. The limit must be the default as we are missing a configuration for all of the python code. I happily add a config and reformat accordingly. Do you have a config that you consider modern/standard these days?

@hendrikmuhs
Copy link
Contributor Author

What about adding a pyproject.toml and configuring a line length of 100?

@narekgharibyan
Copy link
Member

What about adding a pyproject.toml and configuring a line length of 100?

Yup sounds good ... fwiw, I didn't play with pyproject.toml to have an opinion on it, but I think it doesn't really matter.

@hendrikmuhs hendrikmuhs force-pushed the cibuildwheel-workflow branch from 07d42ce to 16f7a92 Compare January 27, 2024 10:00
@hendrikmuhs
Copy link
Contributor Author

hendrikmuhs commented Jan 27, 2024

I reformatted setup.py with ruff. Unfortunately I had to revert my checkin of pyproject.toml as cibuildwhell jumped on it and ignored the requirements.txt file

I think we need a renovation of the python code base according to the newest standards, however not as part of this PR. Created #275

@hendrikmuhs hendrikmuhs merged commit 6b7800f into KeyviDev:master Jan 27, 2024
18 checks passed
@hendrikmuhs hendrikmuhs deleted the cibuildwheel-workflow branch January 28, 2024 10:46
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.

2 participants