[FR] push/pop required policies in cmake toolchain file

[andreya108]

Description

Trying to build project with new NDK we got the following for each component:

-- The C compiler identification is Clang 18.0.1
-- The CXX compiler identification is Clang 18.0.1
CMake Error at /home/andrey/android-sdk-linux/ndk/27.0.11902837/build/cmake/flags.cmake:50 (if):
  if given arguments:

    "hwaddress" "IN_LIST" "ANDROID_SANITIZE"

  Unknown arguments specified
Call Stack (most recent call first):
  /snap/cmake/1399/share/cmake-3.29/Modules/Platform/Android-Clang.cmake:23 (include)
  /snap/cmake/1399/share/cmake-3.29/Modules/Platform/Android-Clang-C.cmake:1 (include)
  /snap/cmake/1399/share/cmake-3.29/Modules/CMakeCInformation.cmake:48 (include)
  CMakeLists.txt:3 (project)


-- Configuring incomplete, errors occurred!

Adding

if(POLICY CMP0057)                                                                                                                                                          
  cmake_policy(SET CMP0057 NEW)
endif()

to the beginning of file ndk/27.0.11902837/build/cmake/flags.cmake solves the problem.

Why it is not set there (https://android.googlesource.com/platform/ndk/+/refs/heads/main/build/cmake/flags.cmake) by default?

P.S.: using $ANDROID_SDK/cmake/3.22.1 gives the same result

Affected versions

r27

Canary version

No response

Host OS

Linux

Host OS version

Ubuntu 20.04

Affected ABIs

arm64-v8a

Build system

CMake

Other build system

No response

minSdkVersion

26

Device API level

No response

[DanAlbert]

Why it is not set there (https://android.googlesource.com/platform/ndk/+/refs/heads/main/build/cmake/flags.cmake) by default?

Because we should not be making policy decisions on your behalf. Any policies we set would change the behavior of your CMakeLists.txt.

Presumably your project either doesn't have a cmake_minimum_required, or has a very old one that's not actually correct? Anything 3.3 or newer would enable that policy by default. That's not something toolchain files are supposed to do (that's my understanding from reading the docs and having spoken with the CMake owner a few times), so we don't set it in the toolchain file, but if you're building android you minimum cannot be lower than 3.6. The minimum may even be higher than that, but I know for sure that you must use at least 3.6.

https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html says that we could probably add something like:

if (CMAKE_MINIMUM_REQUIRED LESS 3.6)
  message(FATAL_ERROR "Android requires cmake_minimum_required(3.6) or greater")
endif()

That's probably the best we can do. Probably a worthwhile improvement, but doesn't actually fix any bugs (and might break some builds if I'm wrong and there's actually a safe minimum between 3.3 and 3.6), so I'm not going to do that so late in the cycle in r27. We can do that for r28 though.

[DanAlbert]

(if any of that analysis is wrong, that's the best I can do without a repro case, and that's why we always ask for one, so upload one if you think I'm wrong)

[FeodorFitsner]

I had the same error while building libjpeg. Indeed, changing minimum CMake version in CMakeLists.txt worked (it's 2.8.12 in the source archive):

cmake_minimum_required(VERSION 3.3)

[DanAlbert]

Okay, so it's a project that wasn't written for android, and for non-android targets it doesn't require anything newer than 2.8.12, but CMake (for good reasons) doesn't allow per-target minimum versions. That makes sense. I don't think we can do any better than a warning for this case, but definitely the warning would be useful.

[dg0yt]

if (CMAKE_MINIMUM_REQUIRED LESS 3.6)
  message(FATAL_ERROR "Android requires cmake_minimum_required(3.6) or greater")
endif()

That's probably the best we can do. Probably a worthwhile improvement, but doesn't actually fix any bugs (and might break some builds if I'm wrong and there's actually a safe minimum between 3.3 and 3.6), so I'm not going to do that so late in the cycle in r27. We can do that for r28 though.

This topic comes up in vcpkg now.
The current state is: An error occurs in the toolchain cmake code, without good diagnostics for the uninitiated, for some ports.
What could have been done for better UX is: Check if cmake policy CMP0057 is set to NEW, and suggest setting CMAKE_POLICY_DEFAULT_CMP0057 to NEW as a config option, in an early FATAL_ERROR.
(Admittedly, not yet tested with CMAKE_POLICY_DEFAULT_CMP0057.)

[DanAlbert]

It's already on the todo list for r28, fwiw. I don't need additional convincing :)

What could have been done for better UX is: Check if cmake policy CMP0057 is set to NEW, and suggest setting CMAKE_POLICY_DEFAULT_CMP0057 to NEW as a config option, in an early FATAL_ERROR.
(Admittedly, not yet tested with CMAKE_POLICY_DEFAULT_CMP0057.)

But we know we need 3.6, and requiring 3.6 already covers that case. To do it with only policy requirements, we'd just have to list all the policies we require between the first version of CMake and 3.6, which is just a really verbose way of requiring 3.6 (for us and for users).

[dg0yt]

But we know we need 3.6

I can understand that the toolchain needs at least CMake 3.6, and I wouldn't mind if it even needs a higher version. But I can't believe that it needs (almost) all of CMP001 to CMP0065. CMake 3.5 and 3.6 even didn't add new policies.

[dg0yt]

Some of CMake's modules (e.g. CMakeFindBinUtils.cmake) simply use

cmake_policy(PUSH)
cmake_policy(SET CMP0057 NEW)
...
cmake_policy(POP)

[dg0yt]

Some of CMake's modules (e.g. CMakeFindBinUtils.cmake) simply use

cmake_policy(PUSH)
cmake_policy(SET CMP0057 NEW)
...
cmake_policy(POP)

Applying these three lines to NDK's flags.cmake brings the number of failing ports in vcpkg down from app. 250 to less than 10.

(The remaining failures include one clang++ crash, for port gdal.)

NDK 27 seems to be reaching vcpkg users in GH actions now.

[DanAlbert]

NDK 27 seems to be reaching vcpkg users in GH actions now.

Then roll back to r26d until we can fix it. It's going to be a while still until r27b comes out.

[DanAlbert]

cmake_policy(PUSH)
cmake_policy(SET CMP0057 NEW)
...
cmake_policy(POP)

Ah, I didn't realize we could push/pop those. We can do that instead. Thanks! The downside is that we don't have any idea what the list of policies we need is, but we can start with 57 and continue updating the list as people uncover issues.

[dg0yt]

Only 0057 IN_LIST is needed at the moment - that's the result of the vcpkg CI run.

Then roll back to r26d until we can fix it. It's going to be a while still until r27b comes out.

Well, "me" can't change Github.

[DanAlbert]

Neither can I. Use a docker image so github can't change your NDK out from underneath you?

[dg0yt]

Dear GHA users who strand here: The other NDK is still in the images, e.g. https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2204-Readme.md#android. You only need to switch some environment variables.

[DanAlbert]

Huh, we actually do attempt to deal with this. There's a cmake_minimum_required(VERSION 3.6.0) right at the top of the toolchain file. I guess that doesn't do anything?

My first attempt at push/pop didn't work because I didn't do it in flags.cmake (because there are other places and I didn't want to miss them), because apparently this is automatic: https://cmake.org/cmake/help/latest/command/cmake_policy.html#cmake-policy-stack:

CMake keeps policy settings on a stack, so changes made by the cmake_policy command affect only the top of the stack. A new entry on the policy stack is managed automatically for each subdirectory to protect its parents and siblings. CMake also manages a new entry for scripts loaded by include() and find_package() commands except when invoked with the NO_POLICY_SCOPE option

and

Calls to the cmake_minimum_required(VERSION)... commands influence only the current top of the policy stack.

Which probably explains why the cmake_minimum_required in the toolchain file does nothing.

But also all this means that my concerns above aren't actually a problem in the first place so we don't even need the push/pop dance. We can just set this in the files that use IN_LIST and it won't infect callers.

[DanAlbert]

In fact, we don't even need to mess with policies. We just needed another cmake_minimum_required(VERSION 3.6.0) in flags.cmake.

https://android-review.googlesource.com/c/platform/ndk/+/3211647 should fix this. The test I added now passes, anyway.