Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
gh-114099: Add configure and Makefile targets to support iOS compilation. #115390
gh-114099: Add configure and Makefile targets to support iOS compilation. #115390
Changes from 3 commits
dfd97f8
838713d
5b54336
9a5f851
8c763af
95275c1
9f851b0
302a5b0
ba946fb
24b3770
3a1a6fb
6247052
ee41015
49778fe
8a3cbeb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
When building outside of the source directory, the copy operation of the stub scripts to the bin directory fails because they are only in the source directory.
for file in $(srcdir)/$(RESSRCDIR)/bin/* ; do \
Large diffs are not rendered by default.
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.
One further thought here: perhaps we should be a bit more exact about what is meant by
the *exact* same version
. A builder could rightfully question whether that means that the build Python has to be built from the exact same source checkout as the iOS version(s) being cross-built. Or is a build Python from the same branch OK, e.g. a Python 3.13.x? This is more of an issue once 3.13, say, is released and we promise things like ABI compatibility, etc.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.
Once a release is finalised, it's generally safe to use any 3.X build as a host Python for a cross platform build of 3.X.Y. However, there were at least 2 commits in January (both threading related) that caused an issue with iOS cross-platform builds of 3.13 if the host Python wasn't the same commit hash. That's easy enough to put down to "main branch is a moving target"; but I believe this could manifest with 3.X.Y as a host for 3.X.Y+1 because there's no guarantee that the bytecode magic number will remain consistent in a 3.X series.
So - the safest answer here is "the same commit hash"; but there's often a bit of leeway. I'll clarify the language here to a "hope for the best, expect the worst" kind of framing.
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.
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
toxcrun
overrides anySDKROOT
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 anSDKROOT
env variable and, if defined, not specify--sdk iphoneos
?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.
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 forSDKROOT
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 theCC
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 overridingSDKROOT
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.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 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 aDEVELOPER_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 defaultiphoneos
?, and mentioning settingDEVELOPER_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.
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.
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 getiphoneosX.Y
andiphonesimulatorX.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 andDEVELOPER_DIR
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.
Sorry, I didn't notice these earlier: all of the stubs should just use a shebang line of
/bin/sh
rather thanbash
sincebash
is officially deprecated on macOS, and there are no bash-isms used.Unchanged files with check annotations Beta
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h
Check warning on line 32 in Include/internal/pycore_runtime_init.h