Fixed android toolchain with newest NDK by christophe-calmejane · Pull Request #21465 · microsoft/vcpkg

christophe-calmejane · 2021-11-16T15:46:32Z

Describe the pull request
ANDROID_NATIVE_API_LEVEL has been removed from NDK cmake toolchain so we now have to use ANDROID_PLATFORM

What does your PR fix?

Using latest android NDK

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

Only affects android triplets

Does your PR follow the maintainer guide?

Yes

ANDROID_NATIVE_API_LEVEL has been removed from NDK cmake toolchain so we now have to use ANDROID_PLATFORM

cenit · 2021-11-16T20:12:26Z

why not setting both, to preserve compatibility with older ndk?

christophe-calmejane · 2021-11-16T21:04:35Z

I noticed some weird stuff when setting both. Some versions of the NDK do not execute the same code when both are set (checks are made in the toolchain if both variables are set or not set).
ANDROID_PLATFORM is actually here since a long time but it was not clear which variable was "the new one" when it was first added to this script.

JackBoosY · 2021-11-17T02:42:58Z

cc @talregev @tronical @jherico @huangqinjin @jamiebk for review this PR.

talregev · 2021-11-17T04:05:56Z

@AenBleidd

dg0yt · 2021-11-17T04:43:40Z

CC @holmescn @m-kuhn

huangqinjin · 2021-11-17T10:10:13Z

If I recall correctly, since NDK 19, ANDROID_NATIVE_API_LEVEL is same as ANDROID_PLATFORM, as stated in https://developer.android.com/ndk/guides/cmake#android_platform. But before that, ANDROID_PLATFORM only supports android-NN format.

Seems the latest NDK dropped ANDROID_NATIVE_API_LEVEL for new toolchain, but still support for legacy toolchain. Sorry to bother @DanAlbert, is that intentional?

DanAlbert · 2021-11-17T19:05:53Z

If I recall correctly, since NDK 19, ANDROID_NATIVE_API_LEVEL is same as ANDROID_PLATFORM, as stated in https://developer.android.com/ndk/guides/cmake#android_platform. But before that, ANDROID_PLATFORM only supports android-NN format.

Seems the latest NDK dropped ANDROID_NATIVE_API_LEVEL for new toolchain, but still support for legacy toolchain. Sorry to bother @DanAlbert, is that intentional?

File an NDK bug please.

AenBleidd · 2021-11-17T20:27:39Z

Building with this change failed on compiler-detect at least on armv6:

CMake Error at /home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r15c/build/cmake/android.toolchain.cmake:408 (message):
  Invalid Android platform: 16.
Call Stack (most recent call first):
  /home/runner/work/boinc/boinc/3rdParty/android/vcpkg/scripts/toolchains/android.cmake:46 (include)
  /home/runner/work/boinc/boinc/3rdParty/android/vcpkg/scripts/buildsystems/vcpkg.cmake:215 (include)
  /usr/local/share/cmake-3.21/Modules/CMakeDetermineSystem.cmake:124 (include)
  CMakeLists.txt:2 (project)

So in order to fix the usage of the newest NDK, a little bit more work is required here.

huangqinjin · 2021-11-18T03:05:16Z

Filed bug android/ndk#1610.

talregev · 2021-11-20T16:30:10Z

I think CMAKE_SYSTEM_VERSION should change both:
ANDROID_NATIVE_API_LEVEL and ANDROID_PLATFORM
Then it will support old and new android sdk.
Can you try?

christophe-calmejane · 2021-11-22T07:48:23Z

I think CMAKE_SYSTEM_VERSION should change both: ANDROID_NATIVE_API_LEVEL and ANDROID_PLATFORM Then it will support old and new android sdk. Can you try?

The issue with setting both is that the 2 variables are not fully compatible (in syntax), depending on the NDK version used by the caller.
I noticed some weird issues/errors when trying to set both at first.

Not sure what is the best solution is, just that for people updating to the latest NDK, vcpkg is not working anymore.

talregev · 2021-11-22T11:09:06Z

If both not working, then we need to separate the update according to NDK version.
old version (let say 21 and less) will update the old var: ANDROID_NATIVE_API_LEVEL
The new version will update the new var: ANDROID_PLATFORM
You can take the version of the ndk from the cmake (or the path) that user will providing you.
Can you try?

christophe-calmejane · 2021-11-22T14:34:59Z

You can take the version of the ndk from the cmake (or the path) that user will providing you.
Can you try?

I unfortunately won't have the time to test this at the moment.
I'm not sure it will be that easy as you have to define the variables before including the NDK cmake toolchain, so it might be difficult to get a value from it before including it, to define said variable based on what has been retrieved.

Maybe someone has a better idea, but currently vcpkg is broken if you get latest NDK. Setting both variables will break vcpkg for people with an older NDK depending on what is set in their CMAKE_SYSTEM_VERSION

talregev · 2021-11-22T20:07:24Z

You don't have to include the cmake to find version number. maybe simple parse from one of the txt/cmake file for ndk version.
You can parse source.properties
Here how it look like for ndk 15:

Pkg.Desc = Android NDK
Pkg.Revision = 15.2.4203891

talregev · 2021-11-22T20:21:32Z

I look on the android.toolchain.cmake inside the ndk 15. we actually can use ANDROID_PLATFORM
we need to set it to android-16 and not just 16.
@AenBleidd Can you try and let us know?

AenBleidd · 2021-11-22T20:29:16Z

@talregev, not sure I got your idea.

talregev · 2021-11-22T20:30:31Z

@AenBleidd I am writing you in discord.

talregev · 2021-11-22T22:13:14Z

ok. I understand the confusion.
the ANDROID_PLATFORM also work in old ndks but in needed to add the prefix android- before the platform number that it will work.
For backward compatibility and not confuse others, I am suggesting another change:
to check if CMAKE_SYSTEM_VERSION it just a number, and if yes, add it android- prefix to the number.

if(CMAKE_SYSTEM_VERSION MATCHES "[0-9]+$")
    set(ANDROID_PLATFORM android-${CMAKE_SYSTEM_VERSION} CACHE STRING "")
else()
    set(ANDROID_PLATFORM ${CMAKE_SYSTEM_VERSION} CACHE STRING "")
endif()

talregev · 2021-11-22T22:16:46Z

We change our triplets to
set(VCPKG_CMAKE_SYSTEM_VERSION android-x)
and it working. It also working before and after your change.

JackBoosY · 2021-11-23T02:16:57Z

So everyone approved this changes?

holmesconan · 2021-11-23T02:19:07Z

I think before we can refactor the entire build process, any patches would temporarily work. Although Android NDK is not evolved so fast, version fragmentation is serious. Issue #21345 tried to discuss and resolve this problem from scratch. But any effort is welcome.

AenBleidd · 2021-11-23T05:55:25Z

@JackBoosY, I'm ok with these changes.

talregev · 2021-11-23T06:00:06Z

@JackBoosY Please consider my suggestion before merge.

talregev · 2021-11-23T18:43:18Z

ok. I understand the confusion. the ANDROID_PLATFORM also work in old ndks but in needed to add the prefix android- before the platform number that it will work. For backward compatibility and not confuse others, I am suggesting another change: to check if CMAKE_SYSTEM_VERSION it just a number, and if yes, add it android- prefix to the number.
if(CMAKE_SYSTEM_VERSION MATCHES "[0-9]+$")
    set(ANDROID_PLATFORM android-${CMAKE_SYSTEM_VERSION} CACHE STRING "")
else()
    set(ANDROID_PLATFORM ${CMAKE_SYSTEM_VERSION} CACHE STRING "")
endif()

@christophe-calmejane Can you try my suggestion?
#21465 (comment)

christophe-calmejane · 2021-11-23T18:46:16Z

I quickly checked and it fails if you set CMAKE_SYSTEM_VERSION to android-15 for exemple

talregev · 2021-11-23T18:47:10Z

There is no android-15. the number is no ndk. we using android-16. for armv6 in ndk 15.
We already check that android-16 is working with your change.
The number is represent android api level. android 16 it mean android 4.1.
And in the latest ndk, google drop the support for android-15.
Can you make my change and check if it work on your use case?

talregev · 2021-11-23T19:04:10Z

https://developer.android.com/ndk/downloads/revision_history

from ndk 18 google drop the support for android 15

Android NDK r18b
Support for ICS (android-14 and android-15) has been removed. Apps using executables no longer need to provide both a PIE and non-PIE executable.

christophe-calmejane · 2021-11-23T19:06:27Z

Sorry I just used any random number when replying, use any number you want, it fails with your change if you set android-NN

talregev · 2021-11-23T19:07:40Z

use any number you want, it fails with your change if you set android

Can you tell me the ndk version are you using, and past here the error?

JackBoosY · 2021-11-24T09:24:25Z

Since I have less knowledge with Android, I will follow the opinions of most people.

christophe-calmejane · 2021-11-24T10:11:24Z

use any number you want, it fails with your change if you set android

Can you tell me the ndk version are you using, and past here the error?

Your code causes android-19 to be transformed to android-android-19 which is bad.
It might just be as simple as to add strict matching "^[0-9]+$" but I don't have time this week to test more

talregev · 2021-11-24T22:59:35Z

Thank you that you find error in my regular expression. I meant if the var contain only digits. can you fix it?

talregev · 2021-11-24T23:30:20Z

use any number you want, it fails with your change if you set android

Can you tell me the ndk version are you using, and past here the error?

Your code causes android-19 to be transformed to android-android-19 which is bad. It might just be as simple as to add strict matching "^[0-9]+$" but I don't have time this week to test more

@christophe-calmejane

I understand the my error
Also read here:
https://cmake.org/cmake/help/latest/command/string.html#regex-specification

Can you test it with the fix?

if(CMAKE_SYSTEM_VERSION MATCHES "^[0-9]+$")
    set(ANDROID_PLATFORM android-${CMAKE_SYSTEM_VERSION} CACHE STRING "")
else()
    set(ANDROID_PLATFORM ${CMAKE_SYSTEM_VERSION} CACHE STRING "")
endif()

christophe-calmejane · 2021-11-30T17:09:34Z

Sorry for the long delay, I'm gonna test some changes as soon as I find a bit of time, hopefully before the end of the week.

christophe-calmejane · 2021-12-01T14:51:08Z

I pushed a new commit, including your proposal. Seems fine on my side.

JackBoosY · 2021-12-03T10:03:40Z

@talregev What's your opinion now?

talregev · 2021-12-03T10:05:39Z

I am happy with the changes. It also support back compatibility.
@christophe-calmejane Thank you for your changes and fix.
@JackBoosY Can you merge it please?

JackBoosY · 2021-12-06T02:25:57Z

Maybe we should also update our docs.

BillyONeal · 2021-12-07T04:15:17Z

Maybe we should also update our docs.

I don't see anywhere any of these variables are mentioned in our docs for Android.

BillyONeal · 2021-12-07T04:15:28Z

Thanks for the bugfix!

Fixed android toolchain with newest NDK

ecc8127

ANDROID_NATIVE_API_LEVEL has been removed from NDK cmake toolchain so we now have to use ANDROID_PLATFORM

github-actions bot approved these changes Nov 16, 2021

View reviewed changes

JonLiu1993 assigned JackBoosY Nov 17, 2021

JackBoosY added the category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. label Nov 17, 2021

talregev mentioned this pull request Nov 22, 2021

Change triplets file for new change of vcpkg. BOINC/boinc#4585

Merged

JackBoosY approved these changes Nov 23, 2021

View reviewed changes

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

holmesconan mentioned this pull request Dec 1, 2021

New Android NDK support request #21345

Closed

Fix android toolchain: Maintain backward compatibility for older NDK

fa90074

github-actions bot approved these changes Dec 1, 2021

View reviewed changes

JackBoosY approved these changes Dec 6, 2021

View reviewed changes

JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Dec 6, 2021

m-kuhn mentioned this pull request Dec 6, 2021

[vcpkg] Enhancement android triplet #21847

Closed

BillyONeal merged commit be65fbb into microsoft:master Dec 7, 2021

Conversation

christophe-calmejane commented Nov 16, 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?

Uh oh!

cenit commented Nov 16, 2021

Uh oh!

christophe-calmejane commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackBoosY commented Nov 17, 2021

Uh oh!

talregev commented Nov 17, 2021

Uh oh!

dg0yt commented Nov 17, 2021

Uh oh!

huangqinjin commented Nov 17, 2021

Uh oh!

DanAlbert commented Nov 17, 2021

Uh oh!

AenBleidd commented Nov 17, 2021

Uh oh!

huangqinjin commented Nov 18, 2021

Uh oh!

talregev commented Nov 20, 2021

Uh oh!

christophe-calmejane commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talregev commented Nov 22, 2021

Uh oh!

christophe-calmejane commented Nov 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AenBleidd commented Nov 22, 2021

Uh oh!

talregev commented Nov 22, 2021

Uh oh!

talregev commented Nov 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

JackBoosY commented Nov 23, 2021

Uh oh!

holmesconan commented Nov 23, 2021

Uh oh!

AenBleidd commented Nov 23, 2021

Uh oh!

talregev commented Nov 23, 2021

Uh oh!

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

Uh oh!

Uh oh!

christophe-calmejane commented Nov 23, 2021

Uh oh!

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

Uh oh!

Uh oh!

talregev commented Nov 23, 2021

Uh oh!

christophe-calmejane commented Nov 23, 2021

Uh oh!

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

Uh oh!

Uh oh!

JackBoosY commented Nov 24, 2021

Uh oh!

christophe-calmejane commented Nov 24, 2021

Uh oh!

talregev commented Nov 24, 2021

Uh oh!

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

Uh oh!

christophe-calmejane commented Nov 16, 2021 •

edited

Loading

christophe-calmejane commented Nov 22, 2021 •

edited

Loading

talregev commented Nov 22, 2021 •

edited

Loading

talregev commented Nov 22, 2021 •

edited

Loading

talregev commented Nov 22, 2021 •

edited

Loading

talregev commented Nov 23, 2021 •

edited

Loading

talregev commented Nov 23, 2021 •

edited

Loading

talregev commented Nov 23, 2021 •

edited

Loading

talregev commented Nov 24, 2021 •

edited

Loading