Fix android ndk clang by holmesconan · Pull Request #21222 · microsoft/vcpkg

holmesconan · 2021-11-06T01:57:26Z

This is a fix of Android triplets that using clang as the default compiler.

What does your PR fix?

Fix vcpkg_configure_make based port cannot find gcc (aka clang) compiler during config stage
Fix ANDROID_NATIVE_API_LEVEL only detected from CMAKE_SYSTEM_VERSION, add ENV{ANDROID_API_LEVEL} for customizing
Which triplets are supported/not supported? Have you updated the CI baseline?

android, No
Does your PR follow the maintainer guide?

Yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

No

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

ghost · 2021-11-06T01:57:38Z

All CLA requirements met.

holmesconan · 2021-11-06T02:12:58Z

The only notification is that, please add Android NDK toolchain executable path:

export PATH=$ANDROID_NDK_HOME/toolchains/llvm/prebuilt/<host-tag>/bin:$PATH

host-tag is linux-x86_64 on linux and 'darwin-x86_64' on osx. Windows is not tested yet.

dg0yt

Thanks for the PR. This is just a quick review in Github.

dg0yt · 2021-11-06T07:07:44Z

scripts/cmake/vcpkg_configure_make.cmake

+    # 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})


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?

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.

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.

dg0yt · 2021-11-06T07:10:14Z

scripts/cmake/vcpkg_configure_make.cmake

+                set(ANDROID_API_LEVEL $ENV{ANDROID_API_LEVEL} CACHE STRING "")
+            else()
+                set(ANDROID_API_LEVEL ${VCPKG_DETECTED_CMAKE_SYSTEM_VERSION} CACHE STRING "")
+            endif()


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

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.

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

dg0yt · 2021-11-06T07:11:48Z

scripts/cmake/vcpkg_configure_make.cmake

    set(base_cmd)
    if(CMAKE_HOST_WIN32)
-        set(base_cmd ${bash_executable} --noprofile --norc --debug)
+        set(base_cmd ${BASH} --noprofile --norc --debug)


There is no BASH here. I think this was renamed to bash_executable as part of the script audit, parallel to your work.

Yes, I checked the upstream version, I will fix it. It was caused by merging without check. shame.

This caused the CI to fail. fixed.

dg0yt · 2021-11-06T07:12:32Z

scripts/cmake/vcpkg_configure_make.cmake

        # -avoid-version is handled specially by libtool link mode, this flag is not forwarded to linker,
        # and libtool tries to avoid versioning for shared libraries and no symbolic links are created.
-        if(VCPKG_TARGET_IS_ANDROID)
+        if(VCPKG_TARGET_IS_ANDROID AND VCPKG_DETECTED_CMAKE_C_COMPILER_ID MATCHES "^GNU$")


Isn't it needed, or does it break, with clang?

PS: STREQUAL "GNU" would suffice.

actually, it just generated a warning for argument unused during compilation.

This is removed. God bless no weird problems happen.

1. ${BASH} -> ${bash_executable} 2. -avoid-version patch ignored

…o fix-android-ndk-clang

dg0yt

We should not unconditionally override (set) arg_BUILD_TRIPLET for Android. It is a valid argument to be supplied by the calling portfile, and it may be overridden by the triplet file. Even if not used in vcpg at the moment, this is public interface possibly used in user's ports or triplets.
The pattern for windows and macos is:

if(VCPKG_TARGET_IS_ANDROID)
    if(arg_DETERMINE_BUILD_TRIPLET OR NOT arg_BUILD_TRIPLET)
        ...
        set(arg_BUILD_TRIPLET ...

dg0yt · 2021-11-06T09:00:26Z

scripts/cmake/vcpkg_configure_make.cmake

+                                                                                           # 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_CMAKE_HOST_SYSTEM_VERSION}")


I guess the last variable is named VCPKG_DETECTED_CMAKE_HOST_SYSTEM_VERSION.

…_SYSTEM_VERSION

JackBoosY · 2021-11-08T06:23:42Z

Failed to build python3:x64-osx:

configure: WARNING: unrecognized options: --disable-silent-rules, --enable-static
Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
./../src/v3.10.0-dcf0f0f52f.clean/configure: line 10530: PKG_PROG_PKG_CONFIG: command not found
./../src/v3.10.0-dcf0f0f52f.clean/configure: line 10673: /usr/local/bin/pkg-config --static: No such file or directory
configure: WARNING: unrecognized options: --disable-silent-rules, --enable-static

dg0yt · 2021-11-08T06:56:47Z

Failed to build python3:x64-osx

I wonder how this PR can have a side effect on /usr/local/bin/pkg-config --static being considered a single value ("No such file or directory") instead of a list for python3:x64-osx.

dg0yt · 2021-11-08T07:00:56Z

Failed to build python3:x64-osx

I wonder how this PR can have a side effect on /usr/local/bin/pkg-config --static being considered a single value ("No such file or directory") instead of a list for python3:x64-osx.

Well, there is one changed line which is unrelated to android, but very much related to x64-osx:

if(host_arch MATCHES "(amd|AMD|x86_|X86_)64")

This change is reasonable, but maybe it needs investigation in a separate PR. By changing host detection, the cross-build property will change to, and this can uncover bugs in other ports.

…o fix-android-ndk-clang

holmesconan · 2021-11-09T01:53:22Z

made a little changes for the z_vcpkg_determine_autotools_host_cpu. if it doesn't resolve the problem, I cannot fix it now. My Macbook is dead.....

holmesconan · 2021-11-10T01:19:23Z

More tests on Android libraries are needed. Anyone who have problems are welcome.

JackBoosY · 2021-11-10T02:43:29Z

Please ping me if this PR is ready for review.

Also, cc @Neumann-A for review the configure_make part.

holmesconan · 2021-11-10T05:53:14Z

@JackBoosY Yes, it is ready for review. @Neumann-A

Neumann-A · 2021-11-11T09:41:27Z

scripts/cmake/vcpkg_configure_make.cmake

+                set(ANDROID_API_LEVEL $ENV{ANDROID_API_LEVEL} CACHE STRING "")
+            else()
+                set(ANDROID_API_LEVEL ${VCPKG_DETECTED_CMAKE_SYSTEM_VERSION} CACHE STRING "")
+            endif()


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

Neumann-A · 2021-11-11T09:42:00Z

scripts/cmake/vcpkg_configure_make.cmake

+            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")


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

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.

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.

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)

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.

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

Neumann-A · 2021-11-11T09:44:01Z

scripts/get_cmake_vars/CMakeLists.txt

-                                        CMAKE_HOST_SYSTEM_PROCESSOR)
+                                        CMAKE_HOST_SYSTEM_PROCESSOR
+                                        CMAKE_SYSTEM_VERSION
+                                        CMAKE_HOST_SYSTEM_VERSION)


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.

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.

When cross-compile python3

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

Neumann-A · 2021-11-11T09:47:45Z

scripts/toolchains/android.cmake

+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()
+


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 .... )

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.

Neumann-A · 2021-11-11T11:44:27Z

Again I feel like the default android toolchain vcpkg provides should be removed and replaced with specialized toolchains for each architecture and case. There seems to be a bigger misunderstanding of vcpkg is supposed to work and what work should be done by the configure scripts.

dg0yt · 2021-11-12T06:25:47Z

@holmescn You just closed this PR while I was writing a comment. I actively invited this PR, and I hoped we could help to form an acceptable result, no matter how hard it is to get Android right in vcpkg. I don't see another effort to move things for Android in vcpkg.

holmesconan · 2021-11-12T06:40:46Z

@dg0yt Thank you for your invitation. Since @Neumann-A made a difficult goal that needs to solve the problem generically. I don't think I have the time and skill to accomplish that target. This is a temporary solution that has many problems unknown. I hope any successor could find more elegant ways to touch the goal, maybe inspired by this effort or not. I would like to give any help if I could.

holmesconan · 2021-11-12T06:57:41Z

As a generic solution, we need to consider from scratch that how vcpkg passthrough each detected variable down to the build system and why they are not catched by the Android NDK toolchain. It needs more knowledge about how vcpkg works.

holmesconan added 5 commits October 18, 2021 22:31

fix: vcpkg_configure_make for NDK r23 clang cross-compiling

5e2a675

remove merge marks

077ad28

fix: Android NDK r23+

6367b06

merged to latest master branch

f2eb05f

Merge branch 'master' into fix-android-ndk-clang

02e7ba7

holmesconan mentioned this pull request Nov 6, 2021

[android] Android NDK r23 and clang support #20844

Closed

dg0yt reviewed Nov 6, 2021

View reviewed changes

holmesconan added 4 commits November 6, 2021 07:56

fix: 1. -> ; 2. -avoid-version patch ignored

bd5de96

fix for review

eec092c

1. ${BASH} -> ${bash_executable} 2. -avoid-version patch ignored

Merge branch 'fix-android-ndk-clang' of github.com:holmescn/vcpkg int…

0920613

…o fix-android-ndk-clang

fix variable renamed inside z_vcpkg_determine_autotools_host_cpu

854ca58

dg0yt reviewed Nov 6, 2021

View reviewed changes

fix: 1. guard arg_BUILD_TRIPLET; 2. typo of VCPKG_DETECTED_CMAKE_HOST…

1ba1b4f

…_SYSTEM_VERSION

JonLiu1993 assigned PhoebeHui Nov 8, 2021

PhoebeHui assigned JackBoosY and unassigned PhoebeHui Nov 8, 2021

github-actions bot approved these changes Nov 8, 2021

View reviewed changes

JackBoosY added category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly labels Nov 8, 2021

holmesconan added 2 commits November 9, 2021 01:51

fix: try to fix z_vcpkg_determine_autotools_host_cpu for x86_64

00ad9dc

Merge branch 'fix-android-ndk-clang' of github.com:holmescn/vcpkg int…

1b1fbdc

…o fix-android-ndk-clang

github-actions bot approved these changes Nov 9, 2021

View reviewed changes

JackBoosY marked this pull request as ready for review November 10, 2021 07:24

JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Nov 11, 2021

Neumann-A reviewed Nov 11, 2021

View reviewed changes

fix: remove CMAKE_SYSTEM_VERSION in get_cmake_vars

632c5a1

github-actions bot approved these changes Nov 12, 2021

View reviewed changes

holmesconan closed this Nov 12, 2021

holmesconan mentioned this pull request Nov 12, 2021

New Android NDK support request #21345

Closed

m-kuhn mentioned this pull request Dec 3, 2021

[android] support "make" based ports #21831

Closed

Conversation

holmesconan commented Nov 6, 2021

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Uh oh!

ghost commented Nov 6, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holmesconan commented Nov 6, 2021

Uh oh!

dg0yt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg0yt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackBoosY commented Nov 8, 2021

Uh oh!

dg0yt commented Nov 8, 2021

Uh oh!

dg0yt commented Nov 8, 2021

Uh oh!

holmesconan commented Nov 9, 2021

Uh oh!

holmesconan commented Nov 10, 2021

Uh oh!

JackBoosY commented Nov 10, 2021

Uh oh!

holmesconan commented Nov 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

ghost commented Nov 6, 2021 •

edited by ghost

Loading

holmesconan commented Nov 12, 2021 •

edited

Loading