Skip to content
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

CMakeLists: Support building for iOS #12672

Merged
merged 1 commit into from Feb 2, 2024
Merged

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Jan 28, 2024

This PR implements the necessary changes to instruct CMake to build for iOS. This mainly boils down to:

The rationale for targeting 2.4 is that, even if we don't build 2.4 for iOS, we avoid painful merge conflicts in the future by already having the relevant conditionals in place.

CMakeLists.txt Outdated
Comment on lines 163 to 187
if(VCPKG_TARGET_TRIPLET MATCHES "^[a-zA-Z0-9]+-osx")
message(STATUS "Targeting macOS (${VCPKG_TARGET_TRIPLET})")
set(CMAKE_SYSTEM_NAME Darwin CACHE STRING "Target macOS")
if(VCPKG_TARGET_TRIPLET MATCHES "^arm64-")
# Minimum macOS version for arm64 Support
set(CMAKE_OSX_DEPLOYMENT_TARGET 11.0 CACHE STRING "Minimum macOS version the build will be able to run on")
set(CMAKE_OSX_ARCHITECTURES arm64 CACHE STRING "The target achritecture")
set(CMAKE_OSX_ARCHITECTURES arm64 CACHE STRING "The target architecture")
set(CMAKE_SYSTEM_PROCESSOR arm64 CACHE STRING "The target system processor")
set(CMAKE_SYSTEM_NAME Darwin CACHE STRING "Setting this enables CMAKE_CROSSCOMPILE")
else()
# Minimum macOS version supported by Qt 5.12
set(CMAKE_OSX_DEPLOYMENT_TARGET 10.12 CACHE STRING "Minimum macOS version the build will be able to run on")
if(NOT VCPKG_TARGET_TRIPLET)
set(VCPKG_TARGET_TRIPLET "x64-osx-min1012")
if(QT6)
# Minimum macOS version supported by Qt 6
set(CMAKE_OSX_DEPLOYMENT_TARGET 10.15 CACHE STRING "Minimum macOS version the build will be able to run on")
else()
# Minimum macOS version supported by Qt 5.12
set(CMAKE_OSX_DEPLOYMENT_TARGET 10.12 CACHE STRING "Minimum macOS version the build will be able to run on")
# Needed for deployment target < 10.14
add_compile_options(-fno-aligned-allocation)
endif()
# Needed for deployment target < 10.14
add_compile_options(-fno-aligned-allocation)
endif()
elseif(VCPKG_TARGET_TRIPLET MATCHES "^[a-zA-Z0-9]+-ios")
message(STATUS "Targeting iOS (${VCPKG_TARGET_TRIPLET})")
set(CMAKE_SYSTEM_NAME iOS CACHE STRING "Target iOS")
set(CMAKE_OSX_DEPLOYMENT_TARGET 14.0 CACHE STRING "Minimum iOS version to target")
else()
message(WARNING "Targeting an Apple platform, but VCPKG_TARGET_TRIPLET is not set. This is not a supported scenario!")
endif()
endif()
Copy link
Member Author

Choose a reason for hiding this comment

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

Implementation note: This logic slightly refactors the macOS part too and is based on the current main. I believe the clear separation between macOS (arm64/x64) and iOS is worth it.

On the next 2.4 -> main merge this will produce a conflict, in which case this entire incoming hunk should be picked, as it already includes the main's changes (mostly just making the arm64 regex more general).

@fwcd fwcd mentioned this pull request Jan 28, 2024
48 tasks
@JoergAtGithub
Copy link
Member

Is 2.4 really the correct target for this PR? It should be for bugfixes only.

@fwcd
Copy link
Member Author

fwcd commented Jan 28, 2024

Since the majority of this logic doesn't apply to the desktop 2.4 builds anyway, I think not having to deal with merge conflicts would be worth it here (even minor things such as adding a source file could get annoying when 2.5 wraps everything into a NOT IOS conditional).

But I'd be open to target 2.5/main too if making changes to the build logic right before a stable release is too risky. In that case I would retarget #12676 too (since the PRs essentially depend on each other, but are split for easier reviewability).

@daschuer
Copy link
Member

This change looks to risky for me at least in the current phase of 2.4.0 release candidate.
I am in doubt that it is reasonable to introduce an iOS version based on Qt5 unless we have specific reasons for it.
How about moving this to 2.5 and Qt6?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Let's fix our plan before merge.

@fwcd
Copy link
Member Author

fwcd commented Jan 29, 2024

Seems reasonable, I'll rebase this one and #12676.

Could you take a look at merging 2.4 to main? That would let me rebase my iOS branch to see what's still missing

@daschuer
Copy link
Member

I can do it tonight, it is also welcome to do it yourself. Don't know if there are difficult conflicts.
I use SmartGit for a nice 3 way diff.

@fwcd
Copy link
Member Author

fwcd commented Jan 30, 2024

Marking as a draft for now, once #12685 is merged, I'll retarget this to main and push my local conflict resolutions (if I do it now, the GitHub labeler will run wild and the PR will blow up with commits again)

@fwcd fwcd changed the base branch from 2.4 to main February 1, 2024 16:11
@fwcd fwcd marked this pull request as ready for review February 1, 2024 16:11
endif()
elseif(VCPKG_TARGET_TRIPLET MATCHES "^[a-zA-Z0-9]+-ios")
Copy link
Member

Choose a reason for hiding this comment

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

We have now a redundant way to select IOS, can we ditch the external IOS flag and handle everything automatically using the VCPKG_TARGET_TRIPLET?

Copy link
Member Author

Choose a reason for hiding this comment

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

The IOS flag is set by CMake, so it's pretty much what we do: https://cmake.org/cmake/help/latest/variable/IOS.html#ios

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I was not aware.

Copy link
Member

@daschuer daschuer 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 at least a check of IOS is missing when dealing with the triplets.

@daschuer daschuer merged commit 33e95e0 into mixxxdj:main Feb 2, 2024
13 checks passed
@fwcd fwcd deleted the cmake-ios-support branch February 2, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants