-
Notifications
You must be signed in to change notification settings - Fork 76
Add support for targeting iOS. #731
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
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.
Thanks for working on this. I don't know anything about building for iOS: the use of environment variables to configure the build goes in the opposite direction of where I would like the ecosystem to go. I can be convinced that this is the only sane way, but the handling of the CFLAGS
and BLDSHARED
environment variables is IMO wrong.
mesonpy/__init__.py
Outdated
for flag in shlex.split(sysconfig.get_config_var('CFLAGS')): | ||
# Ensure the iOS minimum version reflects the current build environment | ||
if flag.startswith('-mios-version-min='): | ||
cflags.append(f"-mios-version-min={min_ios_version}") |
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 this should be an error: in presence of two contradicting settings, guessing is not the right thing to do.
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.
This isn't guessing or contradicting anything - it's expressing specified preferences.
CPython can be (and is) built with a minimum iOS version (13.0 by default), and this is reflected in the compiler flags that are contained in sysconfig data (in this case, CFLAGS).
However, an individual project can then be compiled with a different minimum iOS version. For example, if you want to build NumPy with full accelerated math, you need a min iOS version of 16.4. This obviously results in a binary that can only be used on iOS 16.4 - but that can be (and is) reflected in the iOS wheel tag for the resulting binaries.
So - this code is ensuring that the minimum iOS version used for compilation is the one specified as a part of this Meson build, not the one specified at the CPython level.
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 I understand.
In your picture, the $CFLAGS
env var is set to what Python uses for its compilation taken from sysconfig
data and Python has a -mios-version-min=
flag in there, while the user intent for a minimum iOS version is reflected in the $IPHONEOS_DEPLOYMENT_TARGET
env var. Therefore, you make sure that the former is adjusted to the latter.
In my picture, both $CFLAGS
and $IPHONEOS_DEPLOYMENT_TARGET
come from the user. The guessing I was referring to is relative to deciding which environment variable should take precedence in defining the min iOS version: if you assume that the user is responsible for the value of both environment variables, you can only guess which one is the one the user intended to be correct and the best thing to do is to error out to indicate that the user specified an inconsistent configuration.
With this sorted, I have a few questions regarding the handling of $IPHONEOS_DEPLOYMENT_TARGET
and -mios-version-min=
.
Why does CPython adds -mios-version-min=
to the compilation flags required to link with it? IIUC how this is works at the OS level, it does not seem necessary. What does the compiler do when the $IPHONEOS_DEPLOYMENT_TARGET
env var is set and -mios-version-min=
is specified? What happens when -mios-version-min=
appears multiple times on the command line? Having -mios-version-min=
in the compilation flags in sysconfig (and thus exposed by python-config
, I guess) does not only seem unnecessary, but a potential source of issues.
When compiling for macOS we do not fiddle with -mmacosx-version-min=
compilation flag and rely entirely on the $MACOSX_DEPLOYMENT_TARGET
env var, also for deriving the platform tag. This is sub optimal as the user has many ways to pass -mmacosx-version-min=
to the build and that would not be reflected in the wheel platform tag. I'm not sure about the best way forward here. Maybe continuing to rely on the env vars alone here is the best thimg, at least for continuity. However, if as expected the command line flag takes precedence over the env var, Meson should not include the -mios-version-min=
when pickling up the compilation flags from Python.
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.
These are all good and reasonable questions; at this point, we're hitting up against "things Apple doesn't document well because Why Aren't You Just Using Xcode", so I'll need to do some experimentation to confirm what I think is true is actually the behaviour in practice.
tl;dr is that "just use environment variables" might be a viable approach, but I've got at least one case that doesn't appear to be behaving quite right in my testing - I need to confirm that's not operator error.
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.
After some experimentation (and a lot of hair-pulling), I've been able to get to the bottom of things here:
clang
has three ways to control the minimum SDK:- the
IPHONEOS_DEPLOYMENT_TARGET
environment variable - the
-mios-version-min
compiler argument, and - a version number encoded into the
-target
compiler triple (e.g.,-target arm64-apple-ios13.0
)
- the
- If
-mios-version-min
will override the environment variable without any warning - If multiple
-mios-version-min
arguments are specified, the last one on the command line takes priority - If
-mios-version-min
and-target
are specified,-target
takes priority; and if the version number doesn't match, a warning is raised. - If a "version unspecified" target is provided (e.g.,
-target arm64-apple-ios
), this is interpreted as "no minimum version". A-mios-version-min
will override this - butIPHONEOS_DEPLOYMENT_TARGET
will not - an environment variable is ignored in this situation. - The option doesn't appear go be needed on link; that looks like I was being overzealous in flag usage.
This actually revealed a bug in CPython's compiler shims (see python/cpython#133183) - the compiler shims were effectively overriding the IPHONEOS_DEPLOYMENT_TARGET
provided in the environment. This is essentially why I included the -mios-version-min
argument in the cross build file - without the explicit -mios-version-min
argument, numpy
would fail to build when it hit a language feature that wasn't available before iOS 7, even if IPHONEOS_DEPLOYMENT_TARGET
was set in the environment.
So - the tl;dr is that with a fix in CPython's shims to address the environment variable issue, and promoting the Python library linking argument upstream to meson, the iOS cross-file can be reduced to the pure compiler definitions (just the binary names), and the platform identifier section that will be common to all iOS builds.
I'll push an update as soon as I've got all the pieces sorted out.
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.
Thanks for investigating this. I'm not sure I understand all the details:
5. If a "version unspecified" target is provided (e.g.,
-target arm64-apple-ios
), this is interpreted as "no minimum version". A-mios-version-min
will override this - butIPHONEOS_DEPLOYMENT_TARGET
will not - an environment variable is ignored in this situation.
Are you saying that when the -taget arm64-apple-ios
build flag is specified and no -mios-version-min
is specified, the IPHONEOS_DEPLOYMENT_TARGET
env var is ignored? Given points 2 and 4 above, that would mean that the environment variable is always ignored. I think I'm misunderstanding something.
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.
Yes, that is the consequence.
The other piece of the puzzle is that the -target
option isn't the only way to drive clang
to build for iOS. The Apple-endorsed way to invoke clang
is xcrun --sdk iphoneos clang
; Apple then provides 2 ways to drive clang to produce an iOS binary for a specific architecture:
xcrun --sdk iphoneos clang -target arm64-apple-ios
.xcrun --sdk iphoneos clang -arch arm64
In the second usage, the IPHONEOS_DEPLOYMENT_TARGET
environment variable would be honoured; to target the simulator, you'd need to pass in --sdk iphonesimulator
.
This isn't well documented, but it appears that -target
is Apple's preferred option (to the extent they express preferences). Aside from being more explicit; when compiling for visionOS, there is no -mvisionos-version-min
(or equivalent) argument to clang; the version specifier must be provided with the -target
triple, or with XROS_DEPLOYMENT_TARGET
. There's only 1 architecture for visionOS, so the -target
is strictly unnecessary... I think? It's difficult to prove categorically, and Apple doesn't document this stuff well - again, "Why Aren't You Using Xcode?")
tl;dr - Apple has far too many options for the same thing :-)
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.
This is very useful content for some future debugging I'm sure, so let's leave this comment thread unresolved.
I've just pushed an update that (I think) addresses the concerns raised about the previous version; in summary:
This means the cross-build file is now fairly minimal - it requires no environment lookups, and only specifies things that are "core" to iOS - the compiler names, and the platform identification tags. There's a lot of moving pieces here to be able to test; I've tested it with my numpy PR, but it requires patched versions of numpy, CPython, and meson, in addition to these changes. |
The test failures here seem unrelated; the fedora failure is a CI credits issue; the macOS issue looks like it might be related to the recent meson 1.8 release? |
The two upstream PRs (python/cpython#133184 and mesonbuild/meson#14541) have now been merged. |
The macOS issue resolved itself, Fedora failure can be ignored. There are two other test failures that are due to this PR @freakboy3742, could you resolve those? |
Will do first thing tomorrow. Initial appearances look like some straightforward typing and platform-specific test stuff. |
Done.
|
Grumble... looks like another Windows test failure. Looks like we just skip that test on Windows, then. |
If you want to submit a separate PR for a typo fix that I can merge straight away, then GHA CI jobs won't need manual approval anymore. There's one on line 763 of |
Done - see #748. |
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.
Can you please squash together the commits that make sense to be squashed, and edit the commit messages to comply with the style used in this repository?
I've lost track of things: are the patches needed for iOS support merged into Meson and CPython already?
Those are in, see this comment above. I also started looking at this and then lost track of it - it's about ready to merge I think. |
I can't find any contribution guide to confirm what the required style is - but I've squashed everything into a single commit with a message that looks like it might be consistent with local style.
Yes - the CPython patches were all included in 3.14.0b1; the Meson patches have all landed, and are waiting on the 1.9 release. |
Then I think a note should be added here stating that meson 1.9.0 or later is required for iOS support. |
Indeed, we don't have such a guide. We use the NumPy commit message style https://numpy.org/devdocs/dev/development_workflow.html#writing-the-commit-message |
Done. For complete transparency, 1.9.0 hasn't been released yet, but the required code is all in the main branch, so as soon as a new feature release is made, it should be included. |
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.
Thanks. Just a couple of minor things to fix in the doc.
Thanks. I tweaked up a couple of nits myself. Merging. |
Adds the modifications required to support targeting iOS.
Assumes meson is running in a cross-platform virtual environment, where
sys.platform == "ios"
, and other platform identification tags are "mocked" versions of an iOS environment (matching what cibuildwheel's iOS support does).Functionally, it's very similar to macOS build support - it writes a cross-build configuration file, based on the contents of sysconfigdata.
The bump in the version of Packaging is required so that iOS platform tag handling is available.