-
-
Notifications
You must be signed in to change notification settings - Fork 8
Add support for maccatalyst #43
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
Add support for maccatalyst #43
Conversation
Add maccatalyst support
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 the PR!
The core of this PR looks really good; I've left a couple of broad comments inline about the specifics. Those comments are mostly related to the format of the additional patches; the one big issue is how the MacCatalyst build is configured.
My immediate inclination is that even thought the compiler triple used by MacCatalyst is arm64-apple-ios-macabi
, we might not want to handle this strictly as an "iOS" build. A separate TARGETS-MacCatalyst
et al would give much more flexibility to define catalyst-specific flags (which seems necessary if only for the VERSION_MIN
). It also gives the flexibility to build only the MacCatalyst artefacts (or not build them if you're working on iOS).
Three final notes:
- The ideal end state is that we won't need patches at all. As the libFFI patch is a substantial change in behavior, it should be submitted upstream to libFFI for inclusion there.
mpdecimal
andxz
are a little harder, as the actual patch is toconfig.sub
, which is part ofautoconf
, and theautoconf
maintainers have been very resistant to adding the-simulator
part of the patch. I've tried multiple times, and had no response - not even a "acknowledgement of receipt". To that end, our current approach is to keep the patch to the bare minimum needed on top of the version of config.sub in the project. I'd encourage you to try to submit the patch (although even the submission process is... baroque), but don't be surprised if it goes nowhere.- I've just added a CI workflow to this repo so that changes in the build can be easily evaluated. To test that this PR actually works, you'll need to merge that update into this PR, and add the extra configuration to build MacCatalyst as part of the build matrix.
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 mostly makes sense; a couple of minor patch cleanups flagged, and one request for more details about the specifics of macCatalyst itself.
patch/libffi.patch
Outdated
+ ;; | ||
*-eabi*- | *-gnueabi*-) | ||
+ ;; | ||
+ ios*-macabi- ) |
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.
From a consistency point of view - why not include this as part of the the simulator group on L16?
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.
Relocate to L16
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.
... it has been? I'm not seeing any change here. Have you pushed this update?
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.
Updated now
|
||
class Platform(object): | ||
- pass | ||
+ abi = None |
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.
Based on the usage in this PR - what's the difference between the sdk
and the abi
? Why can't MacCatalyst builds be referenced as sdk="macabi"
?
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 posted a comment above
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.
... where is this comment? I'm not sure I follow what you're referring to.
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 macosx is the sdk and the macabi instructs the compiler to use the Mac Catalyst ABI. It allows the iOS(iPad) version of the app to run on Mac. The Mac Catalyst version only supports the iOS APIs (no access to the MacOSX APIs) and is therefore closely aligned with iOS and for all intents and purposes behaves as such.
The purpose of Mac Catalyst is to allow taking an iPad app and using all the iPad apis in a Mac app.
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 understand what Mac Catalyst is conceptually - I'm asking why it's necessary to separate sdk
and abi
in this configuration. What functional purpose is met by adding an entirely new configuration flag? It's a flag that is only used in the macabi situation, and is literally blank everywhere else. It's mutually exclusive with sdk="iphoneos"
. You're not making any other use of abi
as a logic differentiator... so why make the distinction? Or, if you do want to keep the two separate - why isn't that separation followed through to the rest of the SDK/ABI handling (i.e., every platform has an SDK and an ABI - so why not define them an exploit that to simplify other logic)?
patch/xz.patch
Outdated
none--*) | ||
# None (no kernel, i.e. freestanding / bare metal), | ||
# can be paired with an machine code file format |
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.
Again - same weird addition...
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 appear to be contextual diff lines. I can clean them up manually but it was just generated through diff -Naur on the original vs patched version of the directories.
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.
Update the patch in line with the above
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.
Looks like you may have missed pushing some updates - your comments are referencing changes that don't appear to have been made.
Also - this still requires updates to the CI configuration to generate catalyst builds.
|
||
class Platform(object): | ||
- pass | ||
+ abi = None |
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 understand what Mac Catalyst is conceptually - I'm asking why it's necessary to separate sdk
and abi
in this configuration. What functional purpose is met by adding an entirely new configuration flag? It's a flag that is only used in the macabi situation, and is literally blank everywhere else. It's mutually exclusive with sdk="iphoneos"
. You're not making any other use of abi
as a logic differentiator... so why make the distinction? Or, if you do want to keep the two separate - why isn't that separation followed through to the rest of the SDK/ABI handling (i.e., every platform has an SDK and an ABI - so why not define them an exploit that to simplify other logic)?
@@ -16,7 +16,7 @@ jobs: | |||
BUILD_NUMBER: ${{ steps.build-vars.outputs.BUILD_NUMBER }} | |||
strategy: | |||
matrix: | |||
target: [ "iOS", "tvOS", "watchOS" ] | |||
target: [ "iOS", "tvOS", "watchOS", "MacCatalyst" ] |
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 the release workflow - it doesn't address the CI workflow that I added a couple of days ago to make it possible to test this PR. You may need to merge with main to get that update.
@asavva-2016 Can I ask why you've closed this PR (and the related PRs)? It felt like it was on the right track and getting fairly close to being merged - there were just a few details that needed to be clarified. |
@freakboy3742 I was doing this in my spare time and it was just consuming too much of my time. |
Re: this @freakboy3742. It is required for Mac Catalyst builds because the SDK is
Without the abi flag the Maybe there was a different way to effect that change but that was the simplest that I could determine in my time constraints. In case I wasn't clear elsewhere the Mac Catalyst build uses the macosx SDK with a different ABI. Therefore this check for the |
Totally understood. Thanks for the time you did spend; at the very least, this would be a good starting point if anyone else wants to pick up the development effort where you've left off. For the sake of posterity (and anyone else that might pick up where you left off):
Yes... but you don't need two flags to differentiate MacCatalyst from macOS. If you know the ABI is |
Can't we just do this with macOS regularly, and since the last time I checked, without an XCFramework, dynamic libraries built for macOS could be linked against Mac Catalyst, just link it? Or is there some API that CPython or any of those projects uses that are available in Mac Catalyst but not macOS? |
@johnzhou721 I haven't done a deep dive to confirm if there's any APIs in use that would be a problem - so it may well be possible to use macOS libraries without special handling. However, an explicitly Catalyst-linked build will always be safer as you know that they're safe from an API usage perspective. |
About half a year ago I tried to do this, there was an issues with a header file, but after I tweak it a bit it was okay. Also upstream report: |
In addition should I try to revive this? |
I'll gladly merge a PR adding macCatalyst support if someone revives it. It might even make sense to roll this into the #50, given the changes will be almost exact parallels of each other. |
Hi! I just opened beeware/Python-Apple-support#269 which can't even initialize the interpreter at the moment. Must have tons of bugs. If you have time, would you please take a look? Thank you! Let me fix stuff in #50 now |
This is to add Mac Catalyst support to the cpython-apple-source-deps.
Added a new target MacCatalyst.
PR Checklist: