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

feat: add PyPy macOS arm64 #1372

Merged
merged 6 commits into from
Jan 14, 2023
Merged

feat: add PyPy macOS arm64 #1372

merged 6 commits into from
Jan 14, 2023

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Dec 11, 2022

Add PyPy macOS arm64
Cross compilation is not supported.

fixes #1369

@mayeut
Copy link
Member Author

mayeut commented Dec 11, 2022

Let's see what fails with a simple update.

@mayeut mayeut force-pushed the pypy-macos-arm64 branch 2 times, most recently from e91ab61 to 9779375 Compare December 11, 2022 12:28
@mattip
Copy link
Contributor

mattip commented Jan 1, 2023

Could you update the pypy tarballs on macos to 7.3.11, it hopefully will solve the appveyor failure (macOS x86_64)

@mattip
Copy link
Contributor

mattip commented Jan 2, 2023

It seems the appveyor 10.14 image falls afoul of PyPy issue 3880. There is a 10.15 Catalina appveyor image. Would it be acceptable to update or are there reasons to stay with 10.14 Mojave? Github Actions has deprecated all 10.x images. Note this is just for the platform to build the wheels, delocate still makes wheels that are compatible with the macosx_10_9_x86_64 tag.

@henryiii
Copy link
Contributor

henryiii commented Jan 3, 2023

That would just be for our tests; downstream projects will stop working unless they also update to a 10.15 image too if we make this change. IMO, that's fine, but I don't feel strongly about it if someone else does. (Also IMO, PyPy should have a way to release patch releases that are separate from minor releases for these sorts of fixes ;) )

@henryiii
Copy link
Contributor

henryiii commented Jan 3, 2023

I'd also be okay to update to (at least) MSVC 2017+ for all examples - we've been dropping 2015 support elsewhere (pybind11, scikit-build) as GHA dropped support a while ago (And Python 3.7+ really should use 2017+ anyway, and it works for older ones too)

@mayeut
Copy link
Member Author

mayeut commented Jan 3, 2023

downstream projects will stop working unless they also update to a 10.15 image too if we make this change. IMO, that's fine, but I don't feel strongly about it if someone else does.

I also think that's fine.

@mattip
Copy link
Contributor

mattip commented Jan 3, 2023

CI is passing

@henryiii henryiii marked this pull request as ready for review January 3, 2023 20:08
@henryiii henryiii requested a review from joerick January 4, 2023 04:14
joerick
joerick previously approved these changes Jan 6, 2023
README.md Outdated Show resolved Hide resolved
@@ -73,6 +73,7 @@ jobs:
output-dir: wheelhouse
env:
CIBW_ARCHS_MACOS: x86_64 universal2 arm64
CIBW_SKIP: pp*-macosx_arm64
Copy link
Contributor

@joerick joerick Jan 6, 2023

Choose a reason for hiding this comment

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

Hmmm. on further consideration this does seem a little like a footgun - many people will have cross-compilation written into their configs, which will fail when this PR is added. Can we perhaps provide a softer failure on macOS when pp*_arm64 is erroneously selected on x86_64?

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. when we attempt to build pp-arm64 on x86_64, we print-

warning: pp38-macosx_arm64 was selected but can't be built on this architecture (x86_64). Deselect these builds using CIBW_SKIP=pp*-macosx_arm64 to remove this warning.

I realise this is a special case, but I expect many people to hit a failed build otherwise.

On this other hand, we could say that we'd rather error out for consistency. But since our example configs have included this since arm64 support was added, it's gonna hit a lot of people.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could even downgrade that from a warning to a notice, since there's no real problem with not having skipped pp*_arm64.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, since we don't support pypy cross compilation, I don't even know if we need a notice - it should simply not be a selectable option on a non-native platfrom, IMO, unless we add cross compilation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with @henryiii proposal to filter out this configuration. Tests were updated to check this, let's see CI results.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still be in favour of a notice, since it's the only build identifier that's not selectable in this way.

@joerick joerick dismissed their stale review January 6, 2023 12:04

want to discuss the common failure that this is going to introduce when users have enabled cross-compilation

@joerick
Copy link
Contributor

joerick commented Jan 6, 2023

another thing that occurs to me is that we should update the example configs to add CIBW_SKIP=pp*-macosx_arm64 when we're showing arm64 cross-compilation for cpython. no longer relevant since it's not a misconfiguration anymore

Co-authored-by: Joe Rickerby <[email protected]>
cibuildwheel/macos.py Outdated Show resolved Hide resolved
@joerick
Copy link
Contributor

joerick commented Jan 10, 2023

I added a quiet notice about the auto-skipping of these builds - hope that works for you @mayeut and @henryiii

image

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.

Update PyPy versions, add macos arm64 PyPy
4 participants