Skip to content

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

Closed

Conversation

asavva-2016
Copy link

@asavva-2016 asavva-2016 commented Mar 8, 2025

This is to add Mac Catalyst support to the cpython-apple-source-deps.

Added a new target MacCatalyst.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a 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:

  1. 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.
  2. mpdecimal and xz are a little harder, as the actual patch is to config.sub, which is part of autoconf, and the autoconf 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.
  3. 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.

Copy link
Member

@freakboy3742 freakboy3742 left a 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.

+ ;;
*-eabi*- | *-gnueabi*-)
+ ;;
+ ios*-macabi- )
Copy link
Member

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relocate to L16

Copy link
Member

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?

Copy link
Author

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
Copy link
Member

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"?

Copy link
Author

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

Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again - same weird addition...

Copy link
Author

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.

Copy link
Author

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

Copy link
Member

@freakboy3742 freakboy3742 left a 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
Copy link
Member

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" ]
Copy link
Member

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 asavva-2016 closed this by deleting the head repository Mar 15, 2025
@freakboy3742
Copy link
Member

@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.

@asavva-2016
Copy link
Author

@freakboy3742 I was doing this in my spare time and it was just consuming too much of my time.

@asavva-2016
Copy link
Author

asavva-2016 commented Mar 16, 2025

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)?

Re: this @freakboy3742. It is required for Mac Catalyst builds because the SDK is macosx and in this case we need to pass through the --build flag here:

-                [] if platform.sdk == "macosx" else [f"--build={os.uname().machine}-apple-darwin"]
+                [] if platform.sdk == "macosx" and platform.abi != "macabi" else [f"--build={os.uname().machine}-apple-darwin"]

Without the abi flag the --build flag isn't passed through and it doesn't compile for the correct target.

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 macosx SDK is not sufficient anymore.

@freakboy3742
Copy link
Member

@freakboy3742 I was doing this in my spare time and it was just consuming too much of my time.

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):

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?

Re: this @freakboy3742. It is required for Mac Catalyst builds because the SDK is macosx and in this case we need to pass through the --build flag here:
...
In case I wasn't clear elsewhere the Mac Catalyst build uses the macosx SDK with a different ABI. Therefore this check for the macosx SDK is not sufficient anymore.

Yes... but you don't need two flags to differentiate MacCatalyst from macOS. If you know the ABI is macabi, you also know the SDK is macosx. You can unambiguously determine one from the other. Yes, this means changes in logic in various places, but all the information you need is available.

@johnzhou721
Copy link
Contributor

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?

@freakboy3742
Copy link
Member

@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.

@johnzhou721
Copy link
Contributor

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:
python/cpython#98297

@johnzhou721
Copy link
Contributor

In addition should I try to revive this?

@freakboy3742
Copy link
Member

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.

@johnzhou721
Copy link
Contributor

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

@johnzhou721 johnzhou721 mentioned this pull request Apr 23, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants