[BUG] ANDROID_USE_LEGACY_TOOLCHAIN_FILE not preserved correctly

[smeenai]

Description

android.toolchain.cmake adds ANDROID_USE_LEGACY_TOOLCHAIN_FILE to CMAKE_TRY_COMPILE_PLATFORM_VARIABLES, so that its value is preserved in try_compile checks. However, that preservation is done after the _USE_LEGACY_TOOLCHAIN_FILE branch, which just does a return() in case you're using the legacy toolchain file, so we never actually preserve ANDROID_USE_LEGACY_TOOLCHAIN_FILE correctly. android-legacy.toolchain.cmake has its own set of CMAKE_TRY_COMPILE_PLATFORM_VARIABLES, but it doesn't include ANDROID_USE_LEGACY_TOOLCHAIN_FILE, so that doesn't work either. This is a moot point right now because we default to using the legacy toolchain file anyway, but it may become relevant in the future.

Affected versions

r25, r26

Canary version

No response

Host OS

Linux, Mac, Windows

Host OS version

Any

Affected ABIs

armeabi-v7a, arm64-v8a, x86, x86_64

Build system

CMake

Other build system

No response

minSdkVersion

Any

Device API level

No response

[DanAlbert]

I don't actually understand any of this. Can you upload something that shows the problem?

[smeenai]

Sorry, I'd written this up in a hurry.

For NDK versions where the legacy CMake toolchain isn't the default but you configure it to be used (via ANDROID_USE_LEGACY_TOOLCHAIN_FILE), the legacy toolchain will be used for the build, but try_compile checks will still use the non-legacy toolchain. I can't actually think of an issue this will cause in practice (I thought the behavior difference between the toolchains around overriding CMAKE_C_FLAGS might cause issues, but CMake handles that in try_compile checks in a way that doesn't), but it's easy enough to fix by adding ANDROID_USE_LEGACY_TOOLCHAIN_FILE to CMAKE_TRY_COMPILE_PLATFORM_VARIABLES in android-legacy.toolchain.cmake.

You can verify that by making a dummy change to error out if using the non-legacy toolchain:

--- a/android.toolchain.cmake.orig
+++ b/android.toolchain.cmake
@@ -53,10 +53,12 @@ endif()
 if(_USE_LEGACY_TOOLCHAIN_FILE)
   include("${CMAKE_CURRENT_LIST_DIR}/android-legacy.toolchain.cmake")
   return()
 endif()

+message(FATAL_ERROR "Using non-legacy toolchain file")

Pair that with a dummy CMakeLists.txt, e.g.

cmake_minimum_required(VERSION 3.6.0)
project(cmaketest)

Configuring with -DANDROID_USE_LEGACY_TOOLCHAIN_FILE=ON (and CMake >= 3.21 and an NDK which doesn't force the legacy toolchain, e.g. r24) will fail:

$ cmake -DCMAKE_TOOLCHAIN_FILE=$HOME/path/to/r24/build/cmake/android.toolchain.cmake \
  -DANDROID_USE_LEGACY_TOOLCHAIN_FILE=ON ..
-- ANDROID_PLATFORM not set. Defaulting to minimum supported version
19.
-- The C compiler identification is Clang 14.0.1
-- The CXX compiler identification is Clang 14.0.1
-- Detecting C compiler ABI info
CMake Error at /home/smeenai/local/ndk/linux/r24/build/cmake/android.toolchain.cmake:58 (message):
  Using non-legacy toolchain file
Call Stack (most recent call first):
  /tmp/cmtest/build/CMakeFiles/3.27.4/CMakeSystem.cmake:6 (include)
  /tmp/cmtest/build/CMakeFiles/CMakeScratch/TryCompile-zqA89s/CMakeLists.txt:4 (project)


CMake Error: CMAKE_C_COMPILER not set, after EnableLanguage
CMake Error at /data/users/smeenai/cmake-3.27.4-linux-x86_64/share/cmake-3.27/Modules/CMakeDetermineCompilerABI.cmake:57 (try_compile):
  Failed to configure test project build system.
Call Stack (most recent call first):
  /data/users/smeenai/cmake-3.27.4-linux-x86_64/share/cmake-3.27/Modules/CMakeTestCCompiler.cmake:26 (CMAKE_DETERMINE_COMPILER_ABI)
  CMakeLists.txt:2 (project)


-- Configuring incomplete, errors occurred!

It'll work as expected if android-legacy.toolchain.cmake preserves the variable, i.e.

--- a/android-legacy.toolchain.cmake.orig
+++ b/android-legacy.toolchain.cmake
@@ -254,24 +254,25 @@ endif()
 set(CMAKE_TRY_COMPILE_PLATFORM_VARIABLES
   ANDROID_ABI
   ANDROID_ALLOW_UNDEFINED_SYMBOLS
   ANDROID_ARM_MODE
   ANDROID_ARM_NEON
   ANDROID_CCACHE
   ANDROID_CPP_FEATURES
   ANDROID_DISABLE_FORMAT_STRING_CHECKS
   ANDROID_PIE
   ANDROID_PLATFORM
   ANDROID_STL
   ANDROID_TOOLCHAIN
+  ANDROID_USE_LEGACY_TOOLCHAIN_FILE
 )

So primarily a theoretical concern, but it seemed like a potential oversight that was easy enough to fix.

[DanAlbert]

Ah, I see. Thanks for the detailed explanation 👍 This part of CMake is a mystery to me.

[DanAlbert]

Triaging for r27 since we're pretty far along in the r26 cycle, this isn't a regression, and (if I've understood you correctly) is asymptomatic. If someone discovers a symptom it's an easy cherry-pick into a minor release.

[DanAlbert]

Interestingly r23 tried to do this, but missed this one corner case: https://r.android.com/1810691. https://android-review.googlesource.com/c/platform/ndk/+/2924815 fixes this (though as we suspected above, the problem was asymptomatic, so it shouldn't be of any interest until we switch back to the new toolchain file by default, and we've got no plans to do so).