-
Notifications
You must be signed in to change notification settings - Fork 264
fix: native ios includes device #2453
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
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances support for iOS device builds by treating the arm64_iphoneos
architecture as part of the native
set, updates documentation to reflect this change, and enables footnotes in the site build.
- Added
footnotes
extension in mkdocs configuration - Updated
options.md
to include iOS arm64 footnote and a warning about future default changes - Extended
parse_config
inarchitecture.py
to includearm64_iphoneos
whennative_arch
is the simulator
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
mkdocs.yml | Enabled footnotes extension for new documentation references |
docs/options.md | Clarified native /auto options, added iOS footnote and warning |
cibuildwheel/architecture.py | Ensures arm64_iphoneos is added for native when on Apple silicon |
Comments suppressed due to low confidence (2)
docs/options.md:197
- The bullet for
native
is not correctly indented under theauto
option, which may render the nesting incorrectly. Consider indenting it to match the level ofauto64
andauto32
.
- `native`: the native arch of the build machine - Matches [`platform.machine()`](https://docs.python.org/3/library/platform.html#platform.machine).[^iosarch]
cibuildwheel/architecture.py:84
- Add a unit test to verify that when
native_arch
isarm64_iphonesimulator
, the device architecturearm64_iphoneos
is also included in the result to prevent regressions.
if native_arch == Architecture.arm64_iphonesimulator:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change to native
on iOS makes sense, and if it opens up options for the future, all the better. I've a couple of notes on the docs changes.
Linux riscv64 platform support is experimental and requires an explicit opt-in through [`enable`](#enable). | ||
|
||
Default: `auto` | ||
|
||
!!! warning | ||
The `auto` option is the current default, but 32-bit Linux builds are deprecated and not available on modern manylinux images. Modern Windows (Windows 11) does not come in a 32-bit version either, so we may change the default to `native` in a future version. Please explicitly specify `auto` if you want to keep the 32-bit builds, or specify `auto64` or `native`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still struggle with this logic... I think that setting an option to 'auto' means you're telling the tool 'do what you normally do'. So explicitly setting an option to 'auto' feels redundant. And, if we change the default to 'native' then a user setting an option to 'auto' doesn't self-describe what the intention is - they'd always want to add a comment to that config to say 'include i686 builds' or such.
I'd like to spend a bit more time thinking about the design. Maybe we can make the recommendation a little more explicit, to keep our design options open...
The `auto` option is the current default, but 32-bit Linux builds are deprecated and not available on modern manylinux images. Modern Windows (Windows 11) does not come in a 32-bit version either, so we may change the default to `native` in a future version. Please explicitly specify `auto` if you want to keep the 32-bit builds, or specify `auto64` or `native`. | |
The default includes Linux `i686` and Windows `x86`, but 32-bit Linux builds are deprecated and not available on modern manylinux images, and modern Windows (Windows 11) does not come in a 32-bit version either. So, we may stop building these archs by default in a future release. Please explicitly specify `x86_64 i686` on Linux Intel, or `AMD64 x86` on Windows if you don't want to be affected by this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that setting an option to 'auto' means you're telling the tool 'do what you normally do'. So explicitly setting an option to 'auto' feels redundant.
My understanding of auto
is not the same and has nothing to do with the default setting of the tool itself.
I understand auto
as "build & test everything you can on this system that does not require emulation", especially since auto32
& auto64
do exist and I thus expect auto
to be the union of those as is the case for now.
"do what you normally do" is the default we configure and can be different from auto
. Changing the default to native
makes sense to me. Once this becomes the default, we might want to deprecate auto*
and we could then drop auto*
altogether in a future version IMHO (in case of an override, it will be clearer to request 2 explicit architectures rather than auto
, as mentioned in the previous comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tools don't always set the "auto" setting as the default. Tools often have "off" as default, but allow an "auto" to be enabled. I think backward compatibility for the thousands of users of cibuildwheel is more important than trying to avoid having a default that is not "auto". That doesn't mean we must go that way, but we should consider it as an option.
I'll write more in the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly prefer this wording (below), for two reasons. One, is I don't think users need to bother with the weird naming schemes for architectures, and be forced to use platform-specific names if they don't need to. And second, I want auto64
and auto32
to be immortalized as immutable, those work perfectly and aren't a problem at all. It's just "auto"
that we are debating. :)
The `auto` option is the current default, but 32-bit Linux builds are deprecated and not available on modern manylinux images. Modern Windows (Windows 11) does not come in a 32-bit version either, so we may change the default to `native` in a future version. Please explicitly specify `auto` if you want to keep the 32-bit builds, or specify `auto64` or `native`. | |
The default includes Linux `i686` and Windows `x86`, but 32-bit Linux builds are deprecated and not available on modern manylinux images, and modern Windows (Windows 11) does not come in a 32-bit version either. So, we may stop building these archs by default in a future release. Please explicitly specify `auto64 auto32` if you don't want to be affected by this change. |
The original intention of interpreting
Following on from @mayeut's comment above: my read would be that if building 32 bit manylinux is an issue, then the solution would be to remove 32 bit builds from However, that's a strong opinion, weakly held; if the decision is make this change for "iOS", the implementation here makes sense, and I don't think it will have much practical impact. |
If you do want to built the simulator and device separately, you can split them up, but you'd always want to build both somewhere, so you'd be stuck with jobs |
If we don't change to "native" (option 2 in the issue), then this is a bit less important, but it's still odd that there's a way to spell the simulator arch without a way to spell the device arch. Though, to be fair, there's only one device arch, and two simulator archs, so one could argue that's why there's a special way to spell it. |
To be clear - I have zero practical application for this. I can't see any reason, other than maybe an optimised quick check of a build in CI, that you'd target
Makes perfect sense. Like I said, my position is a strong opinion, weakly held; I don't think what you're describing is at all inconsistent or unreasonable, and the implementation makes sense as well. I'm happy if you are. |
If we go with option (2) from the issue, I think (although the applications are minuscule), we can rollback to |
See #2451.
This adds the arm64 device "arch" to
native
. This isn't truly anarch
, it just fit into this framework better than it did intoplatform
, and it is arm64. This meansnative
is now is identical toauto64
when running on a 64-bit system, andauto32
when running on a 32-bit system (like it was before iOS was added). Practically, you almost never want just built a simulator wheel (which is for testing) but not the device wheel. If you did want to split it up, there's no special term for the device wheel, so you could just use the exact arch name for the simulator wheel too.This makes
native
a much nicer default in the future, as it's a logical default (don't build 32-bit on 64-bit), and keeps "auto" meaning the same thing (build as much as you can). I've just added a warning for 3.0 about it possibly changing in the future. I can weaken or strengthen the language, or we could even change the default now if everyone wants that.I've also noted that pyodide ignores the arch and builds regardless of the setting.
CC @freakboy3742