[BUG] CMake 3.21 + NDK r23 sets MINGW=1 in CMake

[jheydebrand]

Description

Observed behaviour:
When using CMake 3.21 with NDK r23 the compiler detection performed by CMake identifies NDK's Clang toolchain as MINGW.
This causes failures during the subsequent build because MINGW=1 implies that the Windows platform is targeted which means
Windows specific files and libraries might be included into the build.

$ cat CMakeLists.txt
cmake_minimum_required(VERSION 3.21.0)

project(NewAndroidCMakeRepro LANGUAGES C CXX)

if(DEFINED MINGW)
    message(STATUS "MINGW defined")
endif()
message(STATUS "MINGW=${MINGW}")
message(STATUS "CMAKE_SYSTEM_NAME=${CMAKE_SYSTEM_NAME}")

# find_program(git git REQUIRED)


$ cat tc.txt
set(ANDROID_CPP_FEATURES rtti exceptions)
set(ANDROID_ABI arm64-v8a)
set(ANDROID_STL c++_shared)
set(ANDROID_NATIVE_API_LEVEL 27)

# Needed for find_program(git) because Android is the only CMake platform which explicitly sets this to OFF
# Only required when ANDROID_USE_LEGACY_TOOLCHAIN_FILE=OFF
set(CMAKE_FIND_USE_SYSTEM_ENVIRONMENT_PATH 1)

# Force compiler ID detection to build for the target instead of the host and thus avoid MINGW=1 on Windows host
# Mismatching these flags with ANDROID_NATIVE_API_LEVEL and ANDROID_ABI might be really bad...
#set(CMAKE_CXX_FLAGS "--target=armv7-none-linux-androideabi16")
#set(CMAKE_C_FLAGS "--target=armv7-none-linux-androideabi16")

# Legacy toolchain does not cause MINGW=1 even in the absence of the CMAKE_<LANG>_FLAGS from above
#set(ANDROID_USE_LEGACY_TOOLCHAIN_FILE 1)

include("/path/to/ndk/23.0.7599858/build/cmake/android.toolchain.cmake")


$ cmake.exe  -GNinja -DCMAKE_TOOLCHAIN_FILE=tc.txt -S . -B .
-- Android: Targeting API '27' with architecture 'arm64', ABI 'arm64-v8a', and processor 'aarch64'
-- Android: Selected unified Clang toolchain
-- The C compiler identification is Clang 12.0.5
-- The CXX compiler identification is Clang 12.0.5
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /path/to/ndk/23.0.7599858/toolchains/llvm/prebuilt/windows-x86_64/bin/clang.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /path/to/ndk/23.0.7599858/toolchains/llvm/prebuilt/windows-x86_64/bin/clang++.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- MINGW defined
-- MINGW=1
-- CMAKE_SYSTEM_NAME=Android
-- Configuring done
-- Generating done
-- Build files have been written to: ./build

Expected behaviour:
The MINGW variable is not defined in CMake.

The expected behaviour can be restored either by setting ANDROID_USE_LEGACY_TOOLCHAIN_FILE=1 or by manipulating CMAKE_<LANG>_FLAGS in tc.txt.

Environment Details

• cmake version 3.21.1; CMake suite maintained and supported by Kitware (kitware.com/cmake).
• Windows 10 Host system (Build 19042.631)
• NDK Version 23.0.7599858
• ABI (arm64-v8a) and API level (27) do not seem to matter

[DanAlbert]

@bradking hoping you can offer some advice here. It looks like the bug here is on the CMake side. If I've understood correctly, https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/CMakeDetermineCXXCompiler.cmake#L120 can only ever identify the default target of the compiler since the -target flag won't be in cxxflags until after the compiler has been proven to be clang. All this code is unfamiliar to me though, so there's a pretty good chance I'm wrong :) lmk if you agree and I'll file a bug over on the cmake side.

[bradking]

@DanAlbert you're correct. The logic here predates Clang or any notion of a compiler that supports more than one target. Therefore for Clang, it determines the default target. It can probably be updated to be aware of CMAKE_<LANG>_COMPILER_TARGET.

[DanAlbert]

Thanks for confirming. Filed https://gitlab.kitware.com/cmake/cmake/-/issues/22647

We might be able to forcibly add the target argument to the compiler name so that CMake can't avoid it in compiler ID, but idk if that'll cause other problems.

If that doesn't work we probably need to bump the CMake version that's required for using the non-legacy toolchain since this does render this path quite unusable on windows (and probably has subtle bugs elsewhere). Unfortunately that would mean effectively unshipping this feature for now since there obviously is no CMake version with the fix yet.

[DanAlbert]

Okay, looks like I can add this to the (new) toolchain file as a workaround:

if(CMAKE_ANDROID_ARCH_ABI STREQUAL armeabi-v7a)
  set(ANDROID_LLVM_TRIPLE armv7-none-linux-androideabi)
elseif(CMAKE_ANDROID_ARCH_ABI STREQUAL arm64-v8a)
  set(ANDROID_LLVM_TRIPLE aarch64-none-linux-android)
elseif(CMAKE_ANDROID_ARCH_ABI STREQUAL x86)
  set(ANDROID_LLVM_TRIPLE i686-none-linux-android)
elseif(CMAKE_ANDROID_ARCH_ABI STREQUAL x86_64)
  set(ANDROID_LLVM_TRIPLE x86_64-none-linux-android)
else()
  message(FATAL_ERROR "Invalid Android ABI: ${ANDROID_ABI}.")
endif()
set(CMAKE_C_COMPILER_TARGET "${ANDROID_LLVM_TRIPLE}${CMAKE_SYSTEM_VERSION}")
set(CMAKE_CXX_COMPILER_TARGET "${ANDROID_LLVM_TRIPLE}${CMAKE_SYSTEM_VERSION}")

(thanks Brad for the hint on the other bug)

It won't help anyone not using our toolchain file, but I wasn't having any luck finding another way to do it, and our toolchain file is the workflow with the regression, so I think that's probably fine?

[DanAlbert]

https://android-review.googlesource.com/c/platform/ndk/+/1827522 adds a workaround.

[hhb]

CMAKE_CXX_COMPILER_TARGET is used at CMakeDetermineCXXCompiler.cmake.
It then includes Android-Determine-CXX.cmake.
And then includes Android/Determine-Compiler.cmake.
We have a hook at the beginning of Android/Determine-Compiler.cmake.

So we can set CMAKE_CXX_COMPILER_TARGET at ${CMAKE_ANDROID_NDK}/build/cmake/hooks/pre/Determine-Compiler.cmake. It will be included by Android/Determine-Compiler.cmake. This will work for both cmake users and toolchain file users.

[DanAlbert]

Aha, thanks! I'll try that change tomorrow. I'd poked around in a few of the hooks but whatever I'd tried didn't work. Haven't really gotten familiar with the new system yet.

[hhb]

Aha, thanks! I'll try that change tomorrow. I'd poked around in a few of the hooks but whatever I'd tried didn't work. Haven't really gotten familiar with the new system yet.

Yeah.. sorry for all the mess I left here...😭

[DanAlbert]

Not so much a mess as just I haven't taken the time to learn it yet :)

[DanAlbert]

Uploaded https://android-review.googlesource.com/c/platform/ndk/+/1829052 to apply the workaround better and https://android-review.googlesource.com/c/platform/ndk/+/1829092 to make it non-permanent, since Brad already has a fix up targeted at 3.22. We shouldn't submit the latter until the CMake fix actually lands so not on autosubmit.

[DanAlbert]

Hmm, although that breaks the legacy toolchain because apparently the hooks are loaded even in that case (where CMAKE_SYSTEM_VERSION is always 1 because that's how we tell CMake to disable all its own behavior). I thought the hooks were only supposed to be used for the non-legacy toolchain? Am I remembering wrong, or is that a bug? If the hooks need to support both workflows that's going to get messy fast.

[DanAlbert]

For now I've just opted that hook out. We may want to copy/paste that into all the hooks.

[bradking]

The hooks are there so that the NDK can add logic before/after CMake's logic. The hooks themselves can start with some kind of if(legacy) => return condition if they don't want to work for the legacy toolchain.

[DanAlbert]

Gotcha. That's what I've done, but wasn't sure if that was the intent. Thanks for confirming.