GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||
GeezerVison is showing github.com — gh-114099: Add configure and Makefile targets to support iOS compilation. by freakboy3742 · Pull Request #115390 · python/cpython Conversation
| ||||||||||||||||||||
|
I don't have access to resolve conversations in this repository, so I've marked them with a thumbs up instead. The only outstanding one, which we need guidance from the core team on, is whether the iOS files should go in a top-level directory as this PR currently does, or a subdirectory of |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I personally would prefer one top-level directory, like the current |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
That's good enough for me. I've modified the docs to use a folder inside iOS. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks to @mhsmith for the thorough reviews. Waiting thumbs-up from @ned-deily before landing.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is a very nice PR and a pleasure to review. I don't have time left tonight to actually test it; I will do so tomorrow. But I do have a few, relatively minor review comments here. Let me know what you think.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,2 @@ | |||
| #!/bin/bash | |||
| xcrun --sdk iphoneos ar $@ | |||
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The use of these stub scripts for executables is a clever solution. As they stand, I think there is one potential shortcoming if I understand the xcrun documentation correctly (I haven't yet verified this): according to the man page, specifying --sdk to xcrun overrides any SDKROOT environment variable, something which I've had occasion to use when building and testing for multiple macOS versions. It's probably less of a deal on iOS but I think it would be nice to allow for this. If the behavior is as documented, perhaps the stub scripts could do a test for the presence of an SDKROOT env variable and, if defined, not specify --sdk iphoneos?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
That's an easy enough change to make; however, it does open a potential class of bugs.
As currently defined, arm64-apple-ios-clang will always be a clang install for ARM64 iOS devices. If we allow for SDKROOT as you describe, a user could easily think they're compiling an iOS build, but actually be using an iOS simulator SDK (or worse - a macOS or watchOS SDK).
This is especially problematic when the values are embedded in sysconfig, as the CC value embedded in sysconfig will become dependent on the end-user's environment.
You can still switch between different Xcode installs with xcode-select for testing purposes; I'd argue this is likely more useful for testing purposes, as Xcode leans heavily to having a single iOS SDK per Xcode install.
My inclination is that being explicit with --sdk in these scripts and overriding SDKROOT is preferable on balance, but I'll defer to your judgement if you feel strongly otherwise. If we do make this change, I'll add some docs clarifying how SDKROOT is interpreted, and clarifying that it's the user's responsibility to make sure SDKROOT matches their intended compilation target.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I see two drawbacks with relying solely on xcode-select. One, xcode-select -s has effect system-wide, and so would affect all other uses and users of Xcode on that system. Of course, there is another way to effect this on a process basis: set a DEVELOPER_DIR env variable. But, two, even with that, AFAICT there still would be no way to select a non-default iOS SDK within a particular Xcode instance and, while it may not be the default case at the moment, it would be better to not preclude supporting more than one SDK available in an Xcode instance. I think the primary value of supporting all this would be during development and testing of Python iOS frameworks themselves and, as such, you likely wouldn't want to use them in an actual release of a Python iOS framework but there might be cases where you would. (And, the end-user of the framework, presumably someone developing an iOS app, would need to ensure they had a compatible developer environment anyway.)
So perhaps a slightly simpler way to address these concerns would be to define a new env variable that would allow overriding the SDK name in the stubs, something like IOSSDK='iphoneos17.1' (I'm not hung up on the name) instead of the default iphoneos ?, and mentioning setting DEVELOPER_DIR to use a non-default Xcode installation:
xcrun --sdk ${IOSSDK:-iphoneos} clang -target arm64-apple-ios $@
Or we could just defer this until a demonstrated need arises but I think it is worth considering.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Passing in a specific SDK (or even better, a specific SDK version) with a custom environment variable makes sense to me. I'll push an update shortly that uses IOS_SDK_VERSION so you can get iphoneosX.Y and iphonesimulatorX.Y from a single environment variable (since the base names of the SDKs are stable, and I can't think of a use case where you'd want a different version for the device vs simulator SDK in a single build), along with docs covering the new variable and DEVELOPER_DIR
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,2 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't notice these earlier: all of the stubs should just use a shebang line of /bin/sh rather than bash since bash is officially deprecated on macOS, and there are no bash-isms used.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
This change is part of the work on PEP-738: Adding Android as a supported platform. * Remove the "1.0" suffix from libpython's filename on Android, which would prevent Gradle from packaging it into an app. * Simplify the build command in the Makefile so that libpython always gets given an SONAME with the `-Wl-h` argument, even if the SONAME is identical to the actual filename. * Disable a number of functions on Android which can be compiled and linked against, but always fail at runtime. As a result, the native _multiprocessing module is no longer built for Android. * gh-115390 (bee7bb3) added some pre-determined results to the configure script for things that can't be autodetected when cross-compiling; this change adds Android to these where appropriate. * Add a couple more pre-determined results for Android, and making them cover iOS as well. This means the --enable-ipv6 configure option will no longer be required on either platform.
This change is part of the work on PEP-738: Adding Android as a supported platform. * Remove the "1.0" suffix from libpython's filename on Android, which would prevent Gradle from packaging it into an app. * Simplify the build command in the Makefile so that libpython always gets given an SONAME with the `-Wl-h` argument, even if the SONAME is identical to the actual filename. * Disable a number of functions on Android which can be compiled and linked against, but always fail at runtime. As a result, the native _multiprocessing module is no longer built for Android. * pythongh-115390 (bee7bb3) added some pre-determined results to the configure script for things that can't be autodetected when cross-compiling; this change adds Android to these where appropriate. * Add a couple more pre-determined results for Android, and making them cover iOS as well. This means the --enable-ipv6 configure option will no longer be required on either platform.
This change is part of the work on PEP-738: Adding Android as a supported platform. * Remove the "1.0" suffix from libpython's filename on Android, which would prevent Gradle from packaging it into an app. * Simplify the build command in the Makefile so that libpython always gets given an SONAME with the `-Wl-h` argument, even if the SONAME is identical to the actual filename. * Disable a number of functions on Android which can be compiled and linked against, but always fail at runtime. As a result, the native _multiprocessing module is no longer built for Android. * pythongh-115390 (bee7bb3) added some pre-determined results to the configure script for things that can't be autodetected when cross-compiling; this change adds Android to these where appropriate. * Add a couple more pre-determined results for Android, and making them cover iOS as well. This means the --enable-ipv6 configure option will no longer be required on either platform.
This change is part of the work on PEP-738: Adding Android as a supported platform. * Remove the "1.0" suffix from libpython's filename on Android, which would prevent Gradle from packaging it into an app. * Simplify the build command in the Makefile so that libpython always gets given an SONAME with the `-Wl-h` argument, even if the SONAME is identical to the actual filename. * Disable a number of functions on Android which can be compiled and linked against, but always fail at runtime. As a result, the native _multiprocessing module is no longer built for Android. * pythongh-115390 (bee7bb3) added some pre-determined results to the configure script for things that can't be autodetected when cross-compiling; this change adds Android to these where appropriate. * Add a couple more pre-determined results for Android, and making them cover iOS as well. This means the --enable-ipv6 configure option will no longer be required on either platform.
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
…iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
…iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
…iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
… iOS compilation. (pythonGH-115390)
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.