Add iOS, tvOS, and watchOS support #528
Conversation
There was a problem hiding this comment.
This is looking good from my side, thanks very much for contributing it.
Assigning to @jcar87, that has more expertise than me in CMake and OSX
Just checking in, curious if this might get a look soon? |
|
Yes, sorry for the delay, we will manage this shortly. |
conan_provider.cmake
Outdated
| @@ -19,18 +17,53 @@ function(detect_os OS OS_API_LEVEL OS_VERSION) | |||
| message(STATUS "CMake-Conan: android_platform=${ANDROID_PLATFORM}") | |||
| set(${OS_API_LEVEL} ${_OS_API_LEVEL} PARENT_SCOPE) | |||
| endif() | |||
| if(${CMAKE_SYSTEM_NAME} STREQUAL "Darwin" OR | |||
There was a problem hiding this comment.
I believe this could be simplified a little with
if(CMAKE_SYSTEM_NAME MATCHES "Darwin|iOS|tvOS|watchOS")
also perhaps for style reasons is valid to use check against the string-value of the variable, if(${CMAKE_SYSTEM_NAME}) but CMake's if will also accept a "variable name" instead: https://cmake.org/cmake/help/latest/command/if.html#comparisons
note that if this conditional is only to account for the os.sdk setting, would it make sense to leave out Darwin out? as that will match macOS, which doesn't have that setting
There was a problem hiding this comment.
Darwin is in there to ensure that OS_VERSION is set to CMAKE_OSX_DEPLOYMENT_TARGET, if it exists, which is relevant across all Apple platforms.
| if(CMAKE_SYSTEM_PROCESSOR STREQUAL aarch64) | ||
| # CMAKE_OSX_ARCHITECTURES can contain multiple architectures, but Conan only supports one. | ||
| # Therefore this code only finds one. If the recipes support mulitple architectures, the | ||
| # build will work. Otherwise, there will be a linker error for the missing architecture(s). |
There was a problem hiding this comment.
based on this comment and the logic, I'm not sure this is worth leaving here. If CMAKE_OSX_ARCHITECTURES is defined, then we cannot map to a single architecture. If I'm reading the conditional properly, with the proposed changes in this PR the conan architecture would be whichever matches CMAKE_SYSTEM_PROCESSOR or CMAKE_OSX_ARCHITECTURES when going in order (arm64, armv7-a, armv7, armv7s, x86_64) - not sure this makes for a predictable experience especially considering that unless the caller has made provisions to ensure that the binaries are fat binaries, there will be linker errors.
I would suggest falling back to issuing a warning when CMAKE_OSX_ARCHITECTURES is defined, and stick to following what CMAKE_SYSTEM_PROCESSOR says. In the future we can evaluate how to best handle this case but I have concerns this is fragile at the moment.
There was a problem hiding this comment.
Unfortunately, CMAKE_SYSTEM_PROCESSOR is blank for iOS/tvOS/watchOS builds and set to the build machine architecture for macOS builds. I don't see a way around using CMAKE_OSX_ARCHITECTURES for this.
I can add a warning if CMAKE_OSX_ARCHITECTURES contains more than one architecture, though.
There was a problem hiding this comment.
Thanks @ssrobins!! Where would CMAKE_OSX_ARCHITECTURES be initialized? Does CMake give it default values if it is blank?
As for CMAKE_SYSTEM_PROCESSOR, this is typically set in the same toolchain file that also sets CMAKE_SYSTEM_NAME to iOS - in fact when cross-building this is one of the recommended variables to set in a toolchain, so I'm wondering what the most conventional way is to configure an iOS/tvOS/watchOS build with CMake? I think when Xcode is the generator possibly Cmake can configure the build without an architecture as one can be selected at build time instead with Xcode?
There was a problem hiding this comment.
CMAKE_OSX_ARCHITECTURES would be set by the user, as shown in the documentation, which I would consider the conventional way to build iOS/tvOS/watchOS with CMake since it was added in 3.14. Before then, we had to rely on community-supported toolchains, which did set CMAKE_SYSTEM_PROCESSOR.
CMAKE_OSX_ARCHITECTURES is blank by default, but the generated build defaults to the build machine's architecture so in that case CMAKE_SYSTEM_PROCESSOR is helpful.
There was a problem hiding this comment.
Added a warning if CMAKE_OSX_ARCHITECTURES contains more than one architecture.
conan_provider.cmake
Outdated
| ${CMAKE_SYSTEM_NAME} STREQUAL "tvOS" OR | ||
| ${CMAKE_SYSTEM_NAME} STREQUAL "watchOS") | ||
| if(DEFINED CMAKE_OSX_SYSROOT) | ||
| # CMAKE_OSX_SYSROOT contains the full path to the SDK for MakeFile/Ninja |
There was a problem hiding this comment.
if I'm getting this right, perhaps the logic can be simplified? Just a minor comment
if(NOT IS_DIRECTORY "${CMAKE_OSX_SYSROOT}
# this should cover the case for the Xcode generator,
# assuming that the values here have a 1:1 with the `os.sdk` setting in Conan
# we may want to restrict this to iOS/watchOS/tvOS,
set(_OS_SDK ${CMAKE_OSX_SYSROOT})
else()
# see comment below
endif()
for the simulator ones, all of them have the Simulator.platform substring in the path, perhaps we can make it easier that way - although from what I can se, we need to map iOS -> iphoneos, tvOS -> appletvos, so maybe not straightforward
There was a problem hiding this comment.
I'll work on it a bit tomorrow, see if I can get something simpler than what I currently have.
There was a problem hiding this comment.
Your suggestion simplified the logic a bit, let me know what you think.
| if(CMAKE_SYSTEM_PROCESSOR STREQUAL aarch64) | ||
| # CMAKE_OSX_ARCHITECTURES can contain multiple architectures, but Conan only supports one. | ||
| # Therefore this code only finds one. If the recipes support mulitple architectures, the | ||
| # build will work. Otherwise, there will be a linker error for the missing architecture(s). |
There was a problem hiding this comment.
Added a warning if CMAKE_OSX_ARCHITECTURES contains more than one architecture.
ssrobins
force-pushed
the
ios-support
branch
2 times, most recently
from
af1f217
to
bfeb950
Compare
conan-io#528 introduced new behaviour to support iOS, tvOS and watchOS. This also added a reliance on CMAKE_OSX_ARCHITECTURES, which may be unset in some configurations when building for MacOS. In this case, we should still fallback to CMAKE_SYSTEM_PROCESSOR and assume we're trying to build for the current target only.
* Add a fallback in case CMAKE_OSX_ARCHITECTURES isn't set #528 introduced new behaviour to support iOS, tvOS and watchOS. This also added a reliance on CMAKE_OSX_ARCHITECTURES, which may be unset in some configurations when building for MacOS. In this case, we should still fallback to CMAKE_SYSTEM_PROCESSOR and assume we're trying to build for the current target only. * Add test around cpu architecture detection on macos --------- Co-authored-by: Luis Caro Campos <[email protected]>
Adding support for iOS, tvOS, and watchOS by following this doc:
https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-ios-tvos-or-watchos
Test coverage is imperfect because there are lots of permutations with the three devices, several architectures, and multiple generators and I didn't want the test to take too long. I was also unable to provide x86 test coverage because the Xcode version in the select GitHub Actions image is too new, but I did test it on an older Xcode locally and it does work.