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

SpaceIm · 2023-01-09T13:36:32Z

Changelog: Feature: Add conan.tools.android.android_abi() function to return the Android standard ABI name based on Conan.
Docs: conan-io/docs#2975

closes #12814

Refer to the issue that supports this Pull Request.
If the issue has missing info, explain the purpose/use case/pain/need that covers 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.

^{Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.}

conan/tools/android/utils.py

AbrilRBS · 2023-01-10T14:30:17Z

I think this would be cool to have added in the docs :)

SpaceIm · 2023-01-16T12:12:16Z

Sure, but when I know it has a chance to be merged. I don't want to work on documentation if this feature is rejected.

memsharded

I think this is looking good and could be merged, but I'd prefer to avoid a silent fallback to another context, better raise if profile build or target not correctly defined.

memsharded · 2023-01-26T08:44:41Z

conan/tools/android/utils.py

+    if context not in ["host", "build", "target"]:
+        raise ConanException("context argument must be either 'host', 'build' or 'target'")
+
+    settings = getattr(conanfile, f"settings_{context}", conanfile.settings)


If we are providing the context, then falling back to the host settings doesn't look great, it should be explicit.
I ask you for the "build" context, you return the build context or fail. If this is being used in a cross-build scenario with 1 profile only, then it is allowed (should) fail.

I agree with @memsharded, this is something that should not fall back if missing. What usecase did you have in mind for allowing the fallback?

Yes I had 1 profile in mind, to not raise even if it makes little sense to call this function with 1 profile. Also I didn't want to be too strict with settings_target since it depends on context in which a recipe is called, I'm not sure of the consequences in recipes.

So what would you want? Raise ConanException if settings_{context} is not an attribute of conanfile? Even for settings_host?

settings_host obviously doesn't exist, so it would be necessary to do the right thing, and do an if context == "host" or a dict mapping or something like that. But yes, it is better to raise if not defined, and this function should only be used with 2 profiles. It is typically better to be initially restrictive and raise, then letting some behavior slip, and then users depending on this behavior (for 1 profile) will complain later if this breaks for some reason that we need to force exclusively for 2 profiles (already happened a few times)

AbrilRBS

Thanks for taking the time to implement this. I only have a few minor comments before we can merge :)

AbrilRBS · 2023-01-27T09:14:39Z

conans/test/unittests/tools/android/test_android_tools.py

+def test_tools_android_abi():
+    settings_linux = MockSettings({"os": "Linux", "arch": "foo"})
+
+    for (arch, expected) in [


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

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.

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

AbrilRBS · 2023-01-27T09:17:25Z

conan/tools/android/utils.py

+    if context not in ["host", "build", "target"]:
+        raise ConanException("context argument must be either 'host', 'build' or 'target'")
+
+    settings = getattr(conanfile, f"settings_{context}", conanfile.settings)


I agree with @memsharded, this is something that should not fall back if missing. What usecase did you have in mind for allowing the fallback?

AbrilRBS · 2023-01-27T09:19:37Z

conan/tools/android/utils.py

+        "armv7": "armeabi-v7a",
+        "armv7hf": "armeabi-v7a",
+        "armv8": "arm64-v8a",
+    }.get(arch, arch)


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

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.

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.

AbrilRBS

Thanks! (Sorry about the delay!)
Please don't forget about adding docs for this, will merge once linked so it's easier to write the changelog!

memsharded · 2023-01-30T13:13:10Z

We are releasing 1.58 asap, this would be missing settings raising when not defined, moving to 1.59

memsharded

Apparently this is not that needed in ConanCenter, but I am fine with moving this forward for 1.59, if the settings fallback to host is removed.

SpaceIm · 2023-02-14T22:59:51Z

Thanks for the final polishing. It will be useful in android-ndk recipe (but not anymore in opencv recipe, I have a PR removing its usage).

SpaceIm force-pushed the feat/android_abi branch from 49555d6 to 7571d30 Compare January 9, 2023 13:42

SSE4 reviewed Jan 9, 2023

View reviewed changes

conan/tools/android/utils.py Show resolved Hide resolved

SpaceIm force-pushed the feat/android_abi branch from 7571d30 to 3bee5f5 Compare January 9, 2023 13:44

add conan.tools.android.android_abi

624ef22

SpaceIm force-pushed the feat/android_abi branch from 3bee5f5 to 624ef22 Compare January 9, 2023 13:50

SpaceIm added 2 commits January 9, 2023 15:18

improve unit tests

a4d59e6

improt unit test again

7b8c368

SSE4 approved these changes Jan 9, 2023

View reviewed changes

memsharded reviewed Jan 26, 2023

View reviewed changes

memsharded added this to the 1.58 milestone Jan 26, 2023

memsharded requested a review from AbrilRBS January 26, 2023 08:46

AbrilRBS reviewed Jan 27, 2023

View reviewed changes

AbrilRBS approved these changes Jan 29, 2023

View reviewed changes

memsharded modified the milestones: 1.58, 1.59 Jan 30, 2023

memsharded reviewed Feb 8, 2023

View reviewed changes

AbrilRBS added 2 commits February 14, 2023 17:59

Dont fall back to settings if context settings are missing

d9a0003

Fix tests

57a86f5

memsharded approved these changes Feb 14, 2023

View reviewed changes

memsharded merged commit ec2aa42 into conan-io:develop Feb 14, 2023

SpaceIm deleted the feat/android_abi branch February 14, 2023 22:58

memsharded mentioned this pull request Feb 16, 2023

android_abi() conan-io/docs#2975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SpaceIm commented Jan 9, 2023 •

edited by czoido

Loading

AbrilRBS commented Jan 10, 2023

SpaceIm commented Jan 16, 2023 •

edited

Loading

memsharded left a comment

memsharded Jan 26, 2023

AbrilRBS Jan 27, 2023

SpaceIm Jan 29, 2023

memsharded Jan 29, 2023

AbrilRBS left a comment

AbrilRBS Jan 27, 2023

SpaceIm Jan 29, 2023

memsharded Jan 29, 2023

AbrilRBS Jan 27, 2023

AbrilRBS Jan 27, 2023

SpaceIm Jan 29, 2023

memsharded Jan 29, 2023

AbrilRBS left a comment

memsharded commented Jan 30, 2023

memsharded left a comment

SpaceIm commented Feb 14, 2023

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

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

Conversation

SpaceIm commented Jan 9, 2023 • edited by czoido Loading

AbrilRBS commented Jan 10, 2023

SpaceIm commented Jan 16, 2023 • edited Loading

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbrilRBS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbrilRBS left a comment

Choose a reason for hiding this comment

memsharded commented Jan 30, 2023

memsharded left a comment

Choose a reason for hiding this comment

SpaceIm commented Feb 14, 2023

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

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

SpaceIm commented Jan 9, 2023 •

edited by czoido

Loading

SpaceIm commented Jan 16, 2023 •

edited

Loading