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

Test with android NDK r27 #40339

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Test with android NDK r27 #40339

wants to merge 4 commits into from

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Aug 7, 2024

New LTS Version

Problems:

@Neumann-A
Copy link
Contributor

breaking compatibility with CMake projects which don't ask for CMake 3.5.

Why? Just set the required policy in the toolchain?

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 7, 2024

Why?

Sounds aggressive. Ask Google.

Just set the required policy in the toolchain?

My plan is to do just that, in scripts/toolchains/android.cmake.

@bwrsandman
Copy link
Contributor

CMake projects which don't ask for CMake 3.5.

Looked into the versions. It looks like 3.3 (not 3.5) is the minimum necessary

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 7, 2024

https://github.com/android/ndk/wiki/Changelog-r27:

Android V will allow OEMs to ship arm64-v8a devices with 16KiB page sizes. Devices that use this configuration will not be able to run existing apps that use native code. To be compatible with these devices, applications will need to rebuild all their native code to be 16KiB aligned, and rewrite any code which assumes a specific page size.

Also https://android.googlesource.com/platform/ndk/+/master/docs/BuildSystemMaintainers.md#Page-sizes

Q: Shall 16KiB be the default for NDK r27 arm64-android?
Q: Shall there be triplet variable?

OTOH it isn't more urgent than raising the API level.

@Neumann-A
Copy link
Contributor

Why?

Sounds aggressive. Ask Google.

Reads like: Words, has no sound.... ¯\(ツ)/¯ Don't mind me asking stupid questions.
But you already expressed my opinion upstream before I even had one :) .

Q: Shall there be a special arm64-v8a triplet?

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 7, 2024

Initial success:

Success Fail Cascade
arm-neon-android 1120 252 230
arm64-android 1139 255 (due to IN_LIST: 251) 257
x64-android 1135 263 278

@MonicaLiu0311 MonicaLiu0311 added the requires:testing Needs tests added before merging label Aug 8, 2024
@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 8, 2024

I have difficulties to get CMP0057 from the toolchain scope (android.cmake, called via CMakeDetermineSystem.cmake) to the failing scope (flags.cmake, called via CMakeSystem.cmake). Even when I put it directly into vcpkg.cmake.

Testing the proposed fix to the NDK instead.

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 8, 2024

This works:

diff --git a/scripts/toolchains/android.cmake b/scripts/toolchains/android.cmake
index d675d06a73..4564f29e7e 100644
--- a/scripts/toolchains/android.cmake
+++ b/scripts/toolchains/android.cmake
@@ -31,6 +31,11 @@ endif()
 
 include("${ANDROID_NDK_HOME}/build/cmake/android.toolchain.cmake")
 
+if(ANDROID_NDK_MAJOR STREQUAL "27")
+    # https://github.com/android/ndk/issues/2032#issuecomment-2274923977
+    string(APPEND CMAKE_SYSTEM_CUSTOM_CODE "cmake_policy(SET CMP0057 NEW)\n")
+endif()
+
 if(NOT _VCPKG_ANDROID_TOOLCHAIN)
     set(_VCPKG_ANDROID_TOOLCHAIN 1)
 

But it might be a somewhat tight binding to both CMake and NDK script code.

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 8, 2024

Success with patch to NDK's flags.cmake:

Success Fail Cascade
arm-neon-android n/a* 6 20
arm64-android n/a* 8 (due to IN_LIST: 0 28
x64-android n/a* 9 77

*) successfuild builds from the first run were cached and not tested again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires:testing Needs tests added before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants