Add Mac Catalyst support#256
Add Mac Catalyst support#256asavva-2016 wants to merge 11 commits intobeeware:mainfrom asavva-2016:add-maccatalyst-support-314
Conversation
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
As with the PR on cpython-apple-source-deps - nice work!
Many of the comments I made on that PR apply here as well - especially with regard to the handling of the MacCatalyst target. I suspect that with that high level structural change, a lot of the rest of this patch will become a lot simpler as well.
The other big are of feedback is the patches Python itself. Those are best handled as a PR against https://github.com/freakboy3742/cpython/tree/3.14-patched, so that they can be kept up to date over time.
| PYTHON_MICRO_VERSION=$(shell echo $(PYTHON_VERSION) | grep -Eo "\d+\.\d+\.\d+") | ||
| PYTHON_PKG_MICRO_VERSION=$(shell echo $(PYTHON_PKG_VERSION) | grep -Eo "\d+\.\d+\.\d+") | ||
| PYTHON_VER=$(basename $(PYTHON_VERSION)) | ||
| PYTHON_PATCHED_VERSION=$(PYTHON_VER)-patched |
There was a problem hiding this comment.
Why make this change? The variable is used exactly once?
| TARGETS-iOS=iphonesimulator.x86_64 iphonesimulator.arm64 iphoneos.arm64 | ||
| VERSION_MIN-iOS=13.0 | ||
| TARGETS-iOS=iphonesimulator.x86_64 iphonesimulator.arm64 iphoneos.arm64 maccatalyst.x86_64 maccatalyst.arm64 | ||
| VERSION_MIN-iOS=14.2 |
There was a problem hiding this comment.
As with the cpython-apple-source-deps repo, this should likely be configured as a separate TARGETS-MacCatalyst et al.
There was a problem hiding this comment.
Added the MacCatalyst target as suggested
| $$(CFLAGS-$(os)) | ||
| LDFLAGS-$(target)=\ | ||
| -isysroot $$(SDK_ROOT-$(target)) \ | ||
| $$(CFLAGS-$(os)) |
There was a problem hiding this comment.
Why are these options needed? The CPython makefile should be setting all these correctly
There was a problem hiding this comment.
I think it's required when building C++ targets in mobile-forge (e.g. numpy). Without this I wasn't able to get that to compile. However, let me double check on this and confirm once I've addressed the other comments.
There was a problem hiding this comment.
If it's C++ targets that are the problem, that seems more likely due to either (a) the clang++ shims not existing, or (b) the clang++ shims not being on the path when mobile-forge is running.
There was a problem hiding this comment.
Yes you were correct, I've resolved this issue now with the correct configuration of the shims
| x86_64-apple-ios*-simulator) AR=x86_64-apple-ios-simulator-ar ;; | ||
| + | ||
| + aarch64-apple-tvos*-simulator) AR=arm64-apple-tvos-simulator-ar ;; | ||
| + aarch64-apple-tvos*-macabi) AR=arm64-apple-tvos-macabi-ar ;; |
There was a problem hiding this comment.
... tvos*-macabi? Does that even exist? Same for watchos*-macabi below
| if test -z "$AR"; then | ||
| case "$host" in | ||
| aarch64-apple-ios*-simulator) AR=arm64-apple-ios-simulator-ar ;; | ||
| + aarch64-apple-ios*-macabi) AR=arm64-apple-ios-macabi-ar ;; |
There was a problem hiding this comment.
Watch for tab vs spaces indentation in the patch.
There was a problem hiding this comment.
These changes shouldn't be needed; they're likely an artefact of how you generated the patch.
There was a problem hiding this comment.
Yes I'll update the patch as you detailed above
There was a problem hiding this comment.
Added a PR for the cpython patch, let me know if there is anything that needs changing in the new MacCatalyst testbed, etc
|
Acknowledging that I've seen these updates; however, I can't really review them until the CPython and dependencies PRs are ready, so I'll hold off until then for a deep review. |
2 participants
Add Mac Catalyst support
PR Checklist: