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.

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

add conan.tools.android.android_abi() #12873

Merged
merged 5 commits into from
Feb 14, 2023
Merged

add conan.tools.android.android_abi() #12873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
624ef22
add conan.tools.android.android_abi
SpaceIm Jan 9, 2023
a4d59e6
improve unit tests
SpaceIm Jan 9, 2023
7b8c368
improt unit test again
SpaceIm Jan 9, 2023
d9a0003
Dont fall back to settings if context settings are missing
AbrilRBS Feb 14, 2023
57a86f5
Fix tests
AbrilRBS Feb 14, 2023
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
1 change: 1 addition & 0 deletions conan/tools/android/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from conan.tools.android.utils import android_abi
31 changes: 31 additions & 0 deletions conan/tools/android/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from conan.errors import ConanException


def android_abi(conanfile, context="host"):
"""
Returns Android-NDK ABI
:param conanfile: ConanFile instance
:param context: either "host", "build" or "target"
:return: Android-NDK ABI
"""
if context not in ("host", "build", "target"):
raise ConanException(f"context argument must be either 'host', 'build' or 'target', was '{context}'")

try:
settings = getattr(conanfile, f"settings_{context}")
except AttributeError:
if context == "host":
settings = conanfile.settings
else:
raise ConanException(f"settings_{context} not declared in recipe")
arch = settings.get_safe("arch")
# https://cmake.org/cmake/help/latest/variable/CMAKE_ANDROID_ARCH_ABI.html
return {
"armv5el": "armeabi",
"armv5hf": "armeabi",
"armv5": "armeabi",
"armv6": "armeabi-v6",
"armv7": "armeabi-v7a",
"armv7hf": "armeabi-v7a",
"armv8": "arm64-v8a",
SpaceIm marked this conversation as resolved.
Show resolved Hide resolved
}.get(arch, arch)
Copy link
Member

Choose a reason for hiding this comment

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

In the old code, android_abi would have been None for missing archs. The second parameter here changes that bahaviour, which might be more correct (No idea), but we should make sure it does not have any unintended side-effects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well there are 2 old codes, inconsistent:

  • legacy toos.to_android_abi() => it fallbacks to str(conanfile.settings.arch)
  • CMakeToolchain => fallbacks to None

So a decision has to be made for this new one, as you wish, I've not a strong opinion on this.

Copy link
Member

Choose a reason for hiding this comment

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

I think falling back to the arch input instead of None might be better, for cases where the users extend settings.yml with their own architectures, if they map directly to android ones, then this will work. So I am fine with this as-is.

8 changes: 2 additions & 6 deletions conan/tools/cmake/toolchain/blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from jinja2 import Template

from conan.tools._compilers import architecture_flag, libcxx_flags
from conan.tools.android.utils import android_abi
from conan.tools.apple.apple import is_apple_os, to_apple_arch
from conan.tools.build import build_jobs
from conan.tools.build.cross_building import cross_building
Expand Down Expand Up @@ -286,11 +287,6 @@ def context(self):
if os_ != "Android":
return

android_abi = {"x86": "x86",
"x86_64": "x86_64",
"armv7": "armeabi-v7a",
"armv8": "arm64-v8a"}.get(str(self._conanfile.settings.arch))

# TODO: only 'c++_shared' y 'c++_static' supported?
# https://developer.android.com/ndk/guides/cpp-support
libcxx_str = self._conanfile.settings.get_safe("compiler.libcxx")
Expand All @@ -302,7 +298,7 @@ def context(self):

ctxt_toolchain = {
'android_platform': 'android-' + str(self._conanfile.settings.os.api_level),
'android_abi': android_abi,
'android_abi': android_abi(self._conanfile),
'android_stl': libcxx_str,
'android_ndk_path': android_ndk_path,
}
Expand Down
Empty file added conans/test/unittests/tools/android/__init__.py
View file
Open in desktop
Empty file.
79 changes: 79 additions & 0 deletions conans/test/unittests/tools/android/test_android_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
from conans.test.utils.mocks import ConanFileMock, MockSettings
from conans.errors import ConanException
from conan.tools.android import android_abi

from pytest import raises

def test_tools_android_abi():
settings_linux = MockSettings({"os": "Linux", "arch": "foo"})

for (arch, expected) in [
Copy link
Member

Choose a reason for hiding this comment

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

This is usually better to be done with a @parametrized decorator in the function itself, so failures are easier to debug, but I'm ok with this either way :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember to have written tests with @pytest.mark.parametrize, then move to this implementation but I don't remember why. I can change back to parametrize.

Copy link
Member

Choose a reason for hiding this comment

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

No big deal, if it works with parametrize easily, then do it, but don't waste time if you hit some issue.

("armv5el", "armeabi"),
("armv5hf", "armeabi"),
("armv5", "armeabi"),
("armv6", "armeabi-v6"),
("armv7", "armeabi-v7a"),
("armv7hf", "armeabi-v7a"),
("armv8", "arm64-v8a"),
("x86", "x86"),
("x86_64", "x86_64"),
("mips", "mips"),
("mips_64", "mips_64"),
]:
conanfile = ConanFileMock()
settings_android = MockSettings({"os": "Android", "arch": arch})

# 1 profile (legacy native build)
conanfile.settings = settings_android
assert android_abi(conanfile) == expected
assert android_abi(conanfile, context="host") == expected

with raises(ConanException):
assert android_abi(conanfile, context="build") == expected

with raises(ConanException):
assert android_abi(conanfile, context="target") == expected

# 2 profiles
## native build
conanfile.settings = settings_android
conanfile.settings_host = settings_android
conanfile.settings_build = settings_android
assert android_abi(conanfile) == expected
assert android_abi(conanfile, context="host") == expected
assert android_abi(conanfile, context="build") == expected

with raises(ConanException):
assert android_abi(conanfile, context="target") == expected

## cross-build from Android to Linux (quite hypothetical)
conanfile.settings = settings_linux
conanfile.settings_host = settings_linux
conanfile.settings_build = settings_android
assert android_abi(conanfile) != expected
assert android_abi(conanfile, context="host") != expected
assert android_abi(conanfile, context="build") == expected

with raises(ConanException):
assert android_abi(conanfile, context="target")

## cross-build a recipe from Linux to Android:
### test android_abi in recipe itself
conanfile.settings = settings_android
conanfile.settings_host = settings_android
conanfile.settings_build = settings_linux
assert android_abi(conanfile) == expected
assert android_abi(conanfile, context="host") == expected
assert android_abi(conanfile, context="build") != expected
with raises(ConanException):
android_abi(conanfile, context="target")

### test android_abi in "compiler recipe" (ie a android-ndk recipe in tool_requires of recipe being cross-build)
conanfile.settings = settings_linux
conanfile.settings_host = settings_linux
conanfile.settings_build = settings_linux
conanfile.settings_target = settings_android
assert android_abi(conanfile) != expected
assert android_abi(conanfile, context="host") != expected
assert android_abi(conanfile, context="build") != expected
assert android_abi(conanfile, context="target") == expected