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

Pass ANDROID_ABI & ANDROID_NDK to cmake #4538

Conversation

CAMOBAP
Copy link
Contributor

@CAMOBAP CAMOBAP commented Feb 17, 2019

closes #4537

Changelog: Feature: Pass ANDROID_ABI & ANDROID_NDK to CMake
Changelog: Feature: Created tool to covert arch to abi in Andrid: tools.to_android_abi()
Docs: conan-io/docs#1102

Checklist

  • Refer to the issue that supports this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@CLAassistant
Copy link

CLAassistant commented Feb 17, 2019

CLA assistant check
All committers have signed the CLA.

@CAMOBAP CAMOBAP force-pushed the feature/pass_android_ndk_and_android_abi_to_cmake branch 3 times, most recently from 506bd87 to b4c12dc Compare February 17, 2019 21:36
@CAMOBAP CAMOBAP force-pushed the feature/pass_android_ndk_and_android_abi_to_cmake branch 2 times, most recently from dc1f80d to 5e58029 Compare February 21, 2019 18:45
@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Mar 6, 2019

PR for docs is ready

@SSE4 @theodelrieu this is my first PR, maybe you can suggest what can I do to speedup it?

Copy link
Member

@danimtb danimtb left a comment

Choose a reason for hiding this comment

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

Looks reasonable

@SSE4
Copy link
Contributor

SSE4 commented Mar 6, 2019

as identified here: conan-io/conan-docker-tools#107
conan has to set the following environment variables, so toolchain from Android NDK works:

  • ANDROID_STL
  • ANDROID_NDK
  • ANDROID_ABI
  • ANDROID_PLATFORM
  • ANDROID_TOOLCHAIN
  • ANDROID_NATIVE_API_LEVEL

for instance:

    ANDROID_NDK=/android-ndk-r19b
    ANDROID_STL=c++_shared \
    ANDROID_ABI=x86_64 \
    ANDROID_PLATFORM=android-21 \
    ANDROID_TOOLCHAIN=clang \
    ANDROID_NATIVE_API_LEVEL=21 \

see also:

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Mar 6, 2019

@SSE4 agree with you,

Currently only ANDROID_ABI and ANDROID_NDK done by this PR,

I will update it to cover ANDROID_STL, ANDROID_PLATFORM, ANDROID_TOOLCHAIN and ANDROID_NATIVE_API_LEVEL too

@CAMOBAP CAMOBAP force-pushed the feature/pass_android_ndk_and_android_abi_to_cmake branch 2 times, most recently from 379eddf to 32f6cd6 Compare March 7, 2019 20:08
@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Mar 7, 2019

For ANDROID_STL according to https://developer.android.com/ndk/guides/cpp-support.html acceptable values are:

  • c++_shared, c++_static
  • none, expect that compiler.libcxx not set at all
  • system, correspond to compiler.libcxx = libstdc++
  • gnustl_shared, gnustl_static (deprecated from NDK r17 and removed in NDK r18)
  • stlport_shared, gnustl_static (deprecated from NDK r17 and removed in NDK r18)

According to https://android.googlesource.com/platform/ndk.git/+/refs/tags/ndk-r19b/build/cmake/android.toolchain.cmake (latest available NDK r19 at this moment)

I have 3 question at this moment:

  • Should we 'conanize' compiler.libcxx value or it's ok to use library names from android NDK? If ok should we add it to settings.yml by default?
  • Should we care about another 'input' definitions from android.toolchain.cmake ?
  • Looks like ANDROID_PLATFORM and ANDROID_NATIVE_API_LEVEL doing the same, maybe make sense to skip ANDROID_NATIVE_API_LEVEL, I have checked couple of android.toolchain.cmake from different NDK releases, it's ok to do this

@SSE4
Copy link
Contributor

SSE4 commented Mar 8, 2019

  1. I would just leave c++_shared and c++_static, as all other variants are deprecated or even removed in NDK (GNU STL aka libstdc++, STL Port).

ANDROID_NATIVE_API_LEVEL - recipe from @theodelrieu used to set this one. but if it's no longer required, I suppose we may skip this one.
are there other input definitions from android.toolchain.cmake that are missing? I believe they might be important in some cases for certain specific libraries, so probably it might be good to set them as well for the sanity and safety.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Mar 8, 2019

@SSE4 thanks I will update PR according your suggestions

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Mar 9, 2019

@SSE4 I just checked rest variables, in fact they control CXX & LD flags, so no reason to pass them

One more question: should I use c++_shared and c++_static or libc++_shared and libc++_static for compiler.libcxx ?

@SSE4
Copy link
Contributor

SSE4 commented Mar 10, 2019

@CAMOBAP795 I personally think it would be easier to just have same values as android NDK has (c++_shared & c++_static) - these way we can just use str(self.settings.compiler.libcxx) and avoid additional mapping functions in recipes or conan codebase.

@CAMOBAP CAMOBAP force-pushed the feature/pass_android_ndk_and_android_abi_to_cmake branch from 32f6cd6 to 70c43f2 Compare March 10, 2019 10:23
@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Mar 10, 2019

@SSE4 done, code ready for review

@CAMOBAP CAMOBAP force-pushed the feature/pass_android_ndk_and_android_abi_to_cmake branch from 70c43f2 to 79c6ee0 Compare March 12, 2019 09:54
@theodelrieu
Copy link
Contributor

Looks good to me. About the other CMake variables present in the NDK toolchain file, I do use ANDROID_ARM_MODE and ANDROID_ARM_NEON as well, but there is likely more to investigate:

# Export configurable variables for the try_compile() command.
set(CMAKE_TRY_COMPILE_PLATFORM_VARIABLES
  ANDROID_TOOLCHAIN
  ANDROID_ABI
  ANDROID_PLATFORM
  ANDROID_STL
  ANDROID_PIE
  ANDROID_CPP_FEATURES
  ANDROID_ALLOW_UNDEFINED_SYMBOLS
  ANDROID_ARM_MODE
  ANDROID_ARM_NEON
  ANDROID_DISABLE_NO_EXECUTE
  ANDROID_DISABLE_RELRO
  ANDROID_DISABLE_FORMAT_STRING_CHECKS
  ANDROID_CCACHE)

@danimtb danimtb self-assigned this May 3, 2019
@lasote
Copy link
Contributor

lasote commented May 21, 2019

@SSE4 and why all these new vars doesn't follow the naming of the other Conan-Cmake related stuff? https://docs.conan.io/en/latest/reference/env_vars.html#cmake-related-variables Why are there different? Maybe they shouldn't

@lasote lasote added this to the 1.16 milestone May 21, 2019
@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented May 21, 2019

@lasote because these variables expected to be consumed by android cmake toolchain.

@lasote Are you propose to follow CONAN_${VAR} approach to pass android related cmake variables?

@lasote
Copy link
Contributor

lasote commented May 21, 2019

If these variables are not "standard" env vars but "standard" cmake variables, and Conan will use them to know when to inject definitions in the CMake() build helper, then yes, they should follow the CONAN_CMAKE_ prefix as others.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented May 21, 2019

If these variables are not "standard" env vars but "standard" cmake variables

They are not "standard" env vars (AFAIK) and they are not "standard" cmake variables, as I said before they are toolchain specific https://android.googlesource.com/platform/ndk/+/master/build/cmake/android.toolchain.cmake

So as far as I understood they should not follow the CONAN_CMAKE_ prefix, moreover because most of them can be taken from Conan's profile confing

@lasote make sense?

@lasote
Copy link
Contributor

lasote commented May 21, 2019

If they are clearly declared in the NDK cmake toolchain they are "standard" enough.

So as far as I understood they should not follow the CONAN_CMAKE_ prefix, moreover because most of them can be taken from Conan's profile confing

There is no reason you cannot declare CONAN_CMAKE_whatever in a profile. :P
So yes, they have to be prefixed to follow the previous cmake env vars managed by conan.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented May 21, 2019

There is no reason you cannot declare CONAN_CMAKE_whatever in a profile. :P

Currently, my profile looks like:

standalone_toolchain=/opt/android-ndk-toolchains/arm-27
ndk_home=/opt/android-ndk
api_level=27

[settings]
os=Android
os.api_level=$api_level
arch=armv7hf
compiler=clang
compiler.version=7.0
compiler.libcxx=libstdc++

[env]
ANDROID_NDK_HOME=$ndk_home
CONAN_CMAKE_FIND_ROOT_PATH=$standalone_toolchain/sysroot
PATH=[$standalone_toolchain/bin]
CHOST=$target_host
...

@lasote according to your suggestion I should duplicate information which already provided with:

  • os.api_level
  • arch
  • compiler
  • compiler.version
  • compiler.libcxx

By providing extra CONAN_CMAKE_ , right?

@lasote
Copy link
Contributor

lasote commented May 21, 2019

No, if it works for you only specifying ANDROID_NDK_HOME as env var, I'm asking you to change it to CONAN_CMAKE_ANDROID_NDK_HOME.

@lasote
Copy link
Contributor

lasote commented May 21, 2019

I'm talking about the vars that you are reading from the env, of course.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented May 21, 2019

@lasote, oh, I got your point, totally make sense, I will update PR

@lasote
Copy link
Contributor

lasote commented May 28, 2019

@CAMOBAP795 could you update the branch and introduce the mentioned changes? we will try to open the release branch ASAP. Otherwise I will do it and I will preserve your contribution, of course.

@CAMOBAP CAMOBAP force-pushed the feature/pass_android_ndk_and_android_abi_to_cmake branch from 79c6ee0 to 8ff9a37 Compare May 28, 2019 16:10
@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented May 28, 2019

@lasote done

@lasote
Copy link
Contributor

lasote commented May 30, 2019

Thanks! I'm merging this, we will introduce some small tuning described here: #5258 But your contribution will be kept, of course. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass ANDROID_ABI & ANDROID_NDK variables to cmake
6 participants