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

Use cibuildwheel to build wheels. #491

Merged
merged 3 commits into from Dec 14, 2021

Conversation

bwoodsend
Copy link
Collaborator

Fixes #456.

Changes proposed in this pull request:

  • Jettison our build wheels on CI infrastructure in favour of cibuildwheel.

We should have done this sooner. Look at all that code we get to delete!

I've rejiggled the work distribution. Now Windows, macOS and normal (x86_64 and i686) Linux get one job each. Then the really slow qemu emulation (Linux on aarch64) is split per Python version. Overall runtime is 13 minutes with Windows being the slowest (despite have half as much work to do as the native Linux runner 🙄).

@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2021

Codecov Report

Merging #491 (104e852) into main (6833495) will not change coverage.
The diff coverage is n/a.

❗ Current head 104e852 differs from pull request most recent head 04286a6. Consider uploading reports for the commit 04286a6 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #491   +/-   ##
=======================================
  Coverage   88.96%   88.96%           
=======================================
  Files           6        6           
  Lines        1685     1685           
=======================================
  Hits         1499     1499           
  Misses        186      186           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6833495...04286a6. Read the comment docs.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

We should have done this sooner. Look at all that code we get to delete!

image

👏


I've rejiggled the work distribution. Now Windows, macOS and normal (x86_64 and i686) Linux get one job each. Then the really slow qemu emulation (Linux on aarch64) is split per Python version. Overall runtime is 13 minutes with Windows being the slowest (despite have half as much work to do as the native Linux runner 🙄).

What do you think about also splitting the deploy-wheels-native.yml matrix into Python versions? Then each one would be maybe 90 seconds or less.

Then the bottleneck would be a QEMU job at around 9 minutes, but all the others should have finished by then.

Another nice benefit of separate jobs is each wheel is uploaded to PyPI a few minutes earlier, and so narrowing the window someone ends up getting and building the sdist.


Finally, I think it's worth bundling this in with the 3.6 drop = major version bump. Might be safer just in case of incompatibilities. What do you think?

.github/workflows/deploy-wheels-native.yml Outdated Show resolved Hide resolved
.github/workflows/deploy-wheels-qemu.yml Outdated Show resolved Hide resolved
.github/workflows/deploy-wheels-qemu.yml Outdated Show resolved Hide resolved
.github/workflows/deploy-wheels-native.yml Outdated Show resolved Hide resolved
@hugovk hugovk added the changelog: Added For new features label Nov 28, 2021
@bwoodsend
Copy link
Collaborator Author

What do you think about also splitting the deploy-wheels-native.yml matrix into Python versions? Then each one would be maybe 90 seconds or less.

Honestly, I find firing off 58 different jobs to save a few minutes thumb twiddling time slightly excessive but, sure, your call...

Then the bottleneck would be a QEMU job at around 9 minutes, but all the others should have finished by then.

Those qemu jobs are doing two jobs each (one glibc and one musl) so we could slice them in half too to bring it down to about 5 minutes.

Another nice benefit of separate jobs is each wheel is uploaded to PyPI a few minutes earlier, and so narrowing the window someone ends up getting and building the sdist.

Surely if the timing is that critical then it would be best to maket the sdist be uploaded last? i.e. Stick it in its own job to be launched only when the wheel building jobs succeed?

Finally, I think it's worth bundling this in with the 3.6 drop = major version bump. Might be safer just in case of incompatibilities. What do you think?

The Windows and macOS wheels are now being built with https://python.org/downloads Pythons instead of those on https://github.com/actions/python-versions so any difference in the compiler flags used building Python will now be affecting those wheels too. Without rummaging through either's build systems, the only difference I know of is that python.org sets the macOS deployment target to 10.9 whereas Github's is unset and defaults to 10.14 (the macOS VM version they build on). So these new wheels should be more compatible in that they also support macOS 10.9 and up. I'd be quite surprised if this is a breaking change but if you're already about to do a major version bump then we might as well assume the worst.

This will enable the building of macOS ARM64 compatible wheels, fixing ultrajson#456 (and
also lets us delete lots of code!!!).
@hugovk
Copy link
Member

hugovk commented Nov 28, 2021

Surely if the timing is that critical then it would be best to make the sdist be uploaded last? i.e. Stick it in its own job to be launched only when the wheel building jobs succeed?

Ah, let's go for that :) Can we have deploy.yml depend on these two wheel workflows or something like that?

So these new wheels should be more compatible in that they also support macOS 10.9 and up. I'd be quite surprised if this is a breaking change but if you're already about to do a major version bump then we might as well assume the worst.

Yep, let's ditch 3.6 from this one too. The new macosx_11_0_arm64 ones are only for 3.8+ so they won't really be losing out on anything.

@bwoodsend
Copy link
Collaborator Author

Can we have deploy.yml depend on these two wheel workflows or something like that?

You can indeed but it looks like they must be defined all in one file so I've merged all 3 deploy*.yml files into one and added the magic line that ensures that the sdist job doesn't happen until the other two succeed. If you look at the run you should see that Github has even drawn a cute little dependency graph to make the point.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Because deploy.yml can now be triggered by PR, we'll need a guard for the Test PyPI upload, like for the other two:

      if: |
        github.repository == 'ultrajson/ultrajson' &&
        github.ref == 'refs/heads/main'

.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you!

Will merge this and #490 in December, to coincide with 3.6 EOL month.

Will likely release before the actual EOL date, the previous 3.6-compatible versions will still be available on PyPI and python_requires will help out.

@hugovk hugovk mentioned this pull request Nov 29, 2021
2 tasks
@hugovk hugovk merged commit b55049f into ultrajson:main Dec 14, 2021
@hugovk
Copy link
Member

hugovk commented Dec 14, 2021

Thank you!

@bwoodsend bwoodsend deleted the switch-to-ci-build-wheels branch December 14, 2021 20:55
@hugovk
Copy link
Member

hugovk commented Dec 16, 2021

@bwoodsend
Copy link
Collaborator Author

You're welcome 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ujson could not be found on Apple Silicon
3 participants