Skip to content

Fix android ndk clang #21222

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.

Sign up for GitHub

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

Closed
holmesconan wants to merge 13 commits into from
Closed

Fix android ndk clang #21222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5e2a675
fix: vcpkg_configure_make for NDK r23 clang cross-compiling
holmesconan Sep 29, 2021
077ad28
remove merge marks
holmesconan Oct 19, 2021
6367b06
fix: Android NDK r23+
holmesconan Oct 25, 2021
f2eb05f
merged to latest master branch
holmesconan Nov 3, 2021
02e7ba7
Merge branch 'master' into fix-android-ndk-clang
holmesconan Nov 6, 2021
bd5de96
fix: 1. -> ; 2. -avoid-version patch ignored
holmesconan Nov 6, 2021
eec092c
fix for review
holmesconan Nov 6, 2021
0920613
Merge branch 'fix-android-ndk-clang' of github.com:holmescn/vcpkg int…
holmesconan Nov 6, 2021
854ca58
fix variable renamed inside z_vcpkg_determine_autotools_host_cpu
holmesconan Nov 6, 2021
1ba1b4f
fix: 1. guard arg_BUILD_TRIPLET; 2. typo of VCPKG_DETECTED_CMAKE_HOST…
holmesconan Nov 6, 2021
00ad9dc
fix: try to fix z_vcpkg_determine_autotools_host_cpu for x86_64
holmesconan Nov 9, 2021
1b1fbdc
Merge branch 'fix-android-ndk-clang' of github.com:holmescn/vcpkg int…
holmesconan Nov 9, 2021
632c5a1
fix: remove CMAKE_SYSTEM_VERSION in get_cmake_vars
holmesconan Nov 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion scripts/cmake/vcpkg_configure_make.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ macro(z_vcpkg_determine_autotools_host_cpu out_var)
endif()
if(host_arch MATCHES "(amd|AMD)64")
set(${out_var} x86_64)
elseif(host_arch MATCHES "(x|X)86_64")
set(${out_var} x86_64)
elseif(host_arch MATCHES "(x|X)86")
set(${out_var} i686)
elseif(host_arch MATCHES "^(ARM|arm)64$")
Expand Down Expand Up @@ -316,7 +318,7 @@ function(vcpkg_configure_make)
# Only for ports using autotools so we can assume that they follow the common conventions for build/target/host
if(CMAKE_HOST_WIN32)
set(arg_BUILD_TRIPLET "--build=${BUILD_ARCH}-pc-mingw32") # This is required since we are running in a msys
# shell which will be otherwise identified as ${BUILD_ARCH}-pc-msys
# shell which will be otherwise identified as ${BUILD_ARCH}-pc-msys
endif()
if(NOT TARGET_ARCH MATCHES "${BUILD_ARCH}" OR NOT CMAKE_HOST_WIN32) # we don't need to specify the additional flags if we build nativly, this does not hold when we are not on windows
string(APPEND arg_BUILD_TRIPLET " --host=${TARGET_ARCH}-pc-mingw32") # (Host activates crosscompilation; The name given here is just the prefix of the host tools for the target)
Expand Down Expand Up @@ -465,6 +467,55 @@ function(vcpkg_configure_make)
endif()
endif()

# Android - cross-compiling support
if(VCPKG_TARGET_IS_ANDROID)
if (VCPKG_DETECTED_CMAKE_CXX_COMPILER_ID MATCHES "^Clang$")
string(REPLACE "-static-libstdc++" "-lc++_static" VCPKG_DETECTED_CMAKE_CXX_STANDARD_LIBRARIES ${VCPKG_DETECTED_CMAKE_CXX_STANDARD_LIBRARIES})
Copy link
Contributor

@dg0yt dg0yt Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this should be wrong in the detected libraries. IIRC, the NDK's toolchain has variables to configure the C++ runtime. Can't we hook into chainloading and configuring this toolchain, and then leave the detected values untouched?

Copy link
Author

@holmesconan holmesconan Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot answer this. I have tried to hook the <VCPKG_ROOT>/scripts/toolchains/android.cmake for adding some util variables, but it seems that vcpkg_configure_make.cmake do not reference that file. So I have to specify the ANDROID_API_LEVEL again. So I do not think it is easy to hook NDK's toolchain file to obtain the detected. Or if I could fully understand how to do it.

Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This export should be done in the CMakeLists.txt of vcpkg_get_cmake_vars so that meson and other buildsystems are also informed about the required flags.


if (DEFINED ENV{ANDROID_API_LEVEL})
set(ANDROID_API_LEVEL $ENV{ANDROID_API_LEVEL} CACHE STRING "")
else()
set(ANDROID_API_LEVEL ${VCPKG_DETECTED_CMAKE_SYSTEM_VERSION} CACHE STRING "")
endif()
Copy link
Contributor

@dg0yt dg0yt Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if respecting ENV{ANDROID_API_LEVEL} at this point may break binary caching.

Copy link
Author

@holmesconan holmesconan Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes, it would be. Since different API level may have different ABI, especially for the libc++. If one library is compiled with ANDROID_API_LEVEL equals 21, but others with 23, for example. They could not be linked into the final .so file.

Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why we should consider ENV{ANDROID_API_LEVEL} here?


if (arg_DETERMINE_BUILD_TRIPLET OR NOT arg_BUILD_TRIPLET)
if(VCPKG_HOST_IS_WINDOWS)
z_vcpkg_determine_autotools_host_cpu(BUILD_ARCH) # machine you are building on => --build=
z_vcpkg_determine_host_mingw(BUILD_MINGW)
set(arg_BUILD_TRIPLET "--build=${BUILD_ARCH}-pc-${BUILD_MINGW}") # This is required since we are running in a msys
# shell which will be otherwise identified as ${BUILD_ARCH}-pc-msys
elseif(VCPKG_HOST_IS_OSX)
z_vcpkg_determine_autotools_host_arch_mac(BUILD_ARCH) # machine you are building on => --build=
set(arg_BUILD_TRIPLET "--build=${BUILD_ARCH}-apple-darwin${VCPKG_DETECTED_CMAKE_HOST_SYSTEM_VERSION}")
elseif(VCPKG_HOST_IS_LINUX)
z_vcpkg_determine_autotools_host_cpu(BUILD_ARCH) # machine you are building on => --build=
set(arg_BUILD_TRIPLET "--build=${BUILD_ARCH}-pc-linux-gnu")
endif()

z_vcpkg_determine_autotools_target_cpu(TARGET_ARCH)
if (TARGET_ARCH MATCHES "armv7-a")
string(APPEND arg_BUILD_TRIPLET " --host=arm-linux-androideabi")
else()
string(APPEND arg_BUILD_TRIPLET " --host=${TARGET_ARCH}-linux-android")
endif()
endif()
debug_message("Using make triplet: ${arg_BUILD_TRIPLET}")

if (TARGET_ARCH MATCHES "armv7-a")
string(APPEND configure_env " CC=armv7a-linux-androideabi${ANDROID_API_LEVEL}-clang")
string(APPEND configure_env " CXX=armv7a-linux-androideabi${ANDROID_API_LEVEL}-clang++")
else()
string(APPEND configure_env " CC=${TARGET_ARCH}-linux-android${ANDROID_API_LEVEL}-clang")
string(APPEND configure_env " CXX=${TARGET_ARCH}-linux-android${ANDROID_API_LEVEL}-clang++")
endif()

string(APPEND configure_env " AR=llvm-ar")
string(APPEND configure_env " RANLIB=llvm-ranlib")
string(APPEND configure_env " READELF=llvm-readelf")
string(APPEND configure_env " STRIP=llvm-strip")
Comment on lines +504 to +515
Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply no. Setup the toolchain correctly..... CC and CXX and the rest of the programs should be correctly setup by cmake.

Copy link
Author

@holmesconan holmesconan Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the ENV{ANDROID_API_LEVEL} since I don't want to add another argument for vcpkg_configure_make. It needs more consideration about the general purpose. It seems that an argument is reasonable? One need to customize the ANDROID API LEVEL instead of the detected version.

Copy link
Author

@holmesconan holmesconan Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the CC and CXX environment variables are the core patch of this PR. Without that, the autotools configure script could not find the compiler properly.

Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the CC and CXX environment variables are the core patch of this PR

but not in this way because it is too specific. If necessary there needs to be a simple branch which setups all progs correctly to the VCPKG_DETECTED_ variables (like in the windows branch)

Copy link
Author

@holmesconan holmesconan Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toolchain did not correctly setup by cmake since it was out of control. When reading config-x86-android-dbg-out.txt in buildtree//, I found that most cross-compile toolchains are grabbed from the system, not in the NDK. It won't work. So I need to manually specify the toolchain through configure_env.

Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that is not a problem for vcpkg_configure_make to solve.

endif()
endif()

# Cleanup previous build dirs
file(REMOVE_RECURSE "${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel"
"${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-dbg"
Expand Down
3 changes: 2 additions & 1 deletion scripts/get_cmake_vars/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ list(APPEND VCPKG_DEFAULT_VARS_TO_CHECK CMAKE_CROSSCOMPILING
CMAKE_SYSTEM_NAME
CMAKE_HOST_SYSTEM_NAME
CMAKE_SYSTEM_PROCESSOR
CMAKE_HOST_SYSTEM_PROCESSOR)
CMAKE_HOST_SYSTEM_PROCESSOR
CMAKE_HOST_SYSTEM_VERSION)
Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CMAKE_HOST_SYSTEM_VERSION)

don't see why we should care about CMAKE_HOST_SYSTEM_VERSION. vcpkg should only focus on building for the target.

Copy link
Author

@holmesconan holmesconan Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cross-compile python3, one needs to specify the --build option and on osx, it will be x86_64-apple-darwin20.6.0, where 20.6.0 is the host system version in cmake. In other words, when cross-compile some libraries on osx, one may need that variable. Yes I will shrink it to CMAKE_HOST_SYSTEM_VERSION only.

Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cross-compile python3

that seems oddly specific to python3. The typically configure scripts don't care about anything after *-darwin*

if(CMAKE_SYSTEM_NAME MATCHES "Darwin")
list(APPEND VCPKG_DEFAULT_VARS_TO_CHECK CMAKE_OSX_DEPLOYMENT_TARGET
CMAKE_OSX_SYSROOT)
Expand Down
9 changes: 8 additions & 1 deletion scripts/toolchains/android.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
set(ANDROID_CPP_FEATURES "rtti exceptions" CACHE STRING "")
set(CMAKE_SYSTEM_NAME Android CACHE STRING "")
set(ANDROID_TOOLCHAIN clang CACHE STRING "")
set(ANDROID_NATIVE_API_LEVEL ${CMAKE_SYSTEM_VERSION} CACHE STRING "")
set(CMAKE_ANDROID_NDK_TOOLCHAIN_VERSION clang CACHE STRING "")

if (DEFINED ENV{ANDROID_API_LEVEL})
set(ANDROID_NATIVE_API_LEVEL $ENV{ANDROID_API_LEVEL} CACHE STRING "")
set(ANDROID_PLATFORM $ENV{ANDROID_API_LEVEL} CACHE STRING "")
else()
set(ANDROID_NATIVE_API_LEVEL ${CMAKE_SYSTEM_VERSION} CACHE STRING "")
set(ANDROID_PLATFORM ${CMAKE_SYSTEM_VERSION} CACHE STRING "")
endif()

Comment on lines +6 to +13
Copy link
Contributor

@Neumann-A Neumann-A Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without env passthrough in the triplet ENV{ANDROID_API_LEVEL} will be always empty on windows. So a custom triplet will be necessary anyway and if that is already necessary having a custom specialized toolchain is also not too far away. (I really hate this all purpose android toolchain which includes all possible arches .... )

Copy link
Author

@holmesconan holmesconan Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is why I add the ENV here. The first time I use it, the previous version only set the ANDROID_NATIVE_API_LEVEL with an empty value. I don't know how to fix this.

if (VCPKG_TARGET_TRIPLET MATCHES "^arm64-android")
set(ANDROID_ABI arm64-v8a CACHE STRING "")
elseif(VCPKG_TARGET_TRIPLET MATCHES "^armv6-android")
Expand Down