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

Do not prepend SDK path to CMAKE_FIND_ROOT_PATH #102

Closed
wants to merge 4 commits into from

Conversation

mologie
Copy link

@mologie mologie commented Mar 7, 2021

Prepending to CMAKE_FIND_ROOT_PATH is harmful [1], because if a library exists in both the SDK and in the user's path (via e.g. the Conan Package Manager), the SDK's library is used. Linking against the wrong library (e.g. libprotobuf on iOS) may crash the app or get it rejected from the store.

Compared to the NDK issue I references we can just drop modification of CMAKE_FIND_ROOT_PATH completely because CMake searches in the custom CMAKE_OSX_SYSROOT anyway.

This pull request implements the suggested change and fixes find_host_package to properly restore the user's find root settings.

[1] android/ndk#912


Here is a test case to confirm that libraries available in the iOS SDK are still found after this patch:

CMakeLists.txt:

cmake_minimum_required(VERSION 3.0.0 FATAL_ERROR)
project(test C)
find_package(ZLIB)
$ cmake -S . -B build -D CMAKE_TOOLCHAIN_FILE=~/devel/ios-cmake/ios.toolchain.cmake -D PLATFORM=OS64 
-- [DEFAULTS] Using the default min-version since DEPLOYMENT_TARGET not provided!
-- [DEFAULTS] Enabling bitcode support by default. ENABLE_BITCODE not provided!
-- [DEFAULTS] Enabling ARC support by default. ENABLE_ARC not provided!
-- [DEFAULTS] Hiding symbols visibility by default. ENABLE_VISIBILITY not provided!
-- [DEFAULTS] Using NON-strict compiler checks by default. ENABLE_STRICT_TRY_COMPILE not provided!
-- Configuring iphoneos build for platform: OS64, architecture(s): arm64
-- Using SDK: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS14.4.sdk
-- Using C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
-- Using CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
-- Using libtool: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool
-- Using install name tool: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/install_name_tool
-- Autoconf target triple: aarch64-apple-ios
-- Using minimum deployment version: 11.0 (SDK version: 14.4)
-- Merging integrated CMake 3.14+ iOS,tvOS,watchOS,macOS toolchain(s) with this toolchain!
-- CMake version: 3.19.6
-- Using a data_ptr size of: 8
-- Bitcode: Enabled
-- ARC: Enabled
-- Hiding symbols: Enabled
-- The C compiler identification is AppleClang 12.0.0.12000032
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Found ZLIB: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS14.4.sdk/usr/lib/libz.tbd (found version "1.2.11") 
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/ios-cmake-find-bug/build

Removing changing IOS, seems to be pretty irrelevant to CMake anyway.
It would previously force IOS to FALSE to to a line swap.
This interferes with Conan (zlib and protobuf of the iOS SDK are
picked up instead of versions that are part of the Conan project).
CMake searches the sysroot anyway, and setting CMAKE_FIND_ROOT_PATH
to CMAKE_PREFIX_PATH might be unintuitive for the user.
@leetal
Copy link
Owner

leetal commented Mar 8, 2021

This change seems to have broken the tests somehow? Perhaps a better idea would be to append to any existing CMAKE_FIND_ROOT_PATH (as the Android toolchain does)?

@mologie
Copy link
Author

mologie commented Mar 8, 2021

I don't think prepending is a solution, it will still break package managers / Conan that way. This seems to be an issue with how the toolchain behaves when targetting Xcode -- I will look into it! Please just keep this PR open meanwhile.

@leetal
Copy link
Owner

leetal commented Mar 11, 2021

I have made some changes to this now on master. I cannot verify if it works or not since i personally don't use Conan, but i think that if you specify CMAKE_FIND_ROOT_PATH to the Conan directories the changes will allow the find_* calls to actually lookup the Conan versions instead of the toolchain versions.

@leetal leetal closed this Dec 12, 2021
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.

2 participants