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

[question] How to guarantee that a specific Android NDK is used for resolving packages #16453

Closed
1 task done
elvisdukaj opened this issue Jun 10, 2024 · 15 comments · Fixed by #16494
Closed
1 task done
Assignees
Milestone

Comments

@elvisdukaj
Copy link
Contributor

elvisdukaj commented Jun 10, 2024

What is your question?

In the settings of a package it's possible to set up a lot of parameters, like the os, compiler.version and others. When building using the Android NDK unfortunately the compiler.version is not enough to guarantee an ABI compatibility between packages. There are cases where also a change of the minor version of the ndk is causing abi issues.

The problem is that both ndks have the same compiler version (clang 9.0.8) and there is no way to distinguish between them in the current state.

Probably in case of an Androiod build, a settings.os.ndk.version settings could be added to compute the package_id. The android ndk version could be deduced from the ndk itself by reading the <ndk_path>/toolchains/llvm/prebuilt/windows-x86_64/sysroot/usr/include/android/ndk-version.h header file or by the name of packages itself (21a -> 21.0, 21b -> 21.1, 25c -> 25.2).

Is there any proposal to handle this case? Are there similar scenario with other toolchains?

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded memsharded self-assigned this Jun 10, 2024
@memsharded
Copy link
Member

Hi @elvisdukaj

The default settings already include:

Android:
        api_level: [ANY]

From our understanding the api_level should be good to represent the binary compatibility, not the actual NDK version (kind of the same as VS with different toolsets, the one affecting the binary is the toolset version, not the IDE version). Have you tried it already?

@elvisdukaj
Copy link
Contributor Author

elvisdukaj commented Jun 10, 2024

Hi @memsharded, thank you for the quick feedback!

I am aware of the os.api_level that can affect the ABI, I will open another issue about the os.api_level as is not working as expected though.

Android API level and Android NDK version unfortunately are not stricly correlated. As an example building a library using android-ndk/r21a and android-ndk/r21b using the same os.api_level (we used 19) leads to a broken build when these settings are mixed.

Even worst is that it's impossible to distinguish between these 2 ndk, they have same compiler version for instance :\

@elvisdukaj
Copy link
Contributor Author

About the os api_level is meant to be forward compatible, that means that if I built with api_level=17 this can be used with any api_level >= 17, but the ABI is not guarantee for lower version.

With the actual implementation instead I need to rebuild all the packages even when moving for instance from API Level 30 to 31 even if there is no need.

I will work on a fix that is setting the api level to the minimum required instead to avoid dupplications.

@memsharded
Copy link
Member

In any case, if you want to add an ndk version to the default settings, that would be trivial, wouldn't it?

Something like:

Android:
        api_level: [ANY]
        ndk_version: [ANY]

And from there you can add os.ndk_version=XXX to your Android profiles and every version will create a distinct binary. Am I missing something here?

@elvisdukaj
Copy link
Contributor Author

Hi @elvisdukaj

The default settings already include:

Android:
        api_level: [ANY]

From our understanding the api_level should be good to represent the binary compatibility, not the actual NDK version (kind of the same as VS with different toolsets, the one affecting the binary is the toolset version, not the IDE version). Have you tried it already?

Hi @elvisdukaj

The default settings already include:

Android:
        api_level: [ANY]

From our understanding the api_level should be good to represent the binary compatibility, not the actual NDK version (kind of the same as VS with different toolsets, the one affecting the binary is the toolset version, not the IDE version). Have you tried it already?

I was thinking to add something similar as the sdk_version in MacOS.

Android:
    api_level: [ANY]
    ndk_version: [ANY] 

Maybe I will propose an MR if that works internally. For making this even nicer I need to change the recipes in of android-ndk so that this value is automatically set when the android-ndk tool is required for building.

@elvisdukaj
Copy link
Contributor Author

In any case, if you want to add an ndk version to the default settings, that would be trivial, wouldn't it?

Something like:

Android:
        api_level: [ANY]
        ndk_version: [ANY]

And from there you can add os.ndk_version=XXX to your Android profiles and every version will create a distinct binary. Am I missing something here?

In any case, if you want to add an ndk version to the default settings, that would be trivial, wouldn't it?

Something like:

Android:
        api_level: [ANY]
        ndk_version: [ANY]

And from there you can add os.ndk_version=XXX to your Android profiles and every version will create a distinct binary. Am I missing something here?

No you didn't miss anything, this is exactly how I would like to proceed. I just want to point out that this is not just my special case, but I think it's a missing parameter in the actual settings.

@memsharded
Copy link
Member

The problem is that this forces Android users to define a ndk_version, which would be breaking. At the very least it should be [null, "ANY"]. But still, if there are no other direct implications in the codebase, probably the recommendation would be to keep it in your own settings_user.yml, no need to contribute it to the main Conan settings if not necessary mainstream.

I am not so sure that the binary compatibility is so bad in the general case for different NDK versions. Is this something that happens in your case all the time for all the packages using different NDK versions? Or it is more like it fails in some special, particular cases?

@elvisdukaj
Copy link
Contributor Author

Android unfortunately is not consistent in keeping ABI compatibility between different ndk, in our internal expirience the ABI compatibility between NDKs was missing, despite the toolchain major and minor build was the same.

I am fine to keep this as internal setting for now, the only downside I see is that I need to fork the android-ndk recipes so that the correct ndk_version value is set. This way it will be harder to merge the upstream in future if things change or to contribute to the project for improvement.

@elvisdukaj
Copy link
Contributor Author

I wanted to point out that msvc has the toolset while iOS, watchOS, tvOS, visionOS and Macos have the mandatory sdk_version that is simlar to the ndk_version and I found this a miss for Android!

@memsharded
Copy link
Member

I am fine to keep this as internal setting for now, the only downside I see is that I need to fork the android-ndk recipes so that the correct ndk_version value is set.

I am not sure what does it mean. The ndk version will be a property of the tool_requires, do you mean when using a tool_requires? But if you define it as tool_requires, the version will be already define, it is not that you can define it from the input setting.

@elvisdukaj
Copy link
Contributor Author

Essentially in the android-ndk recipes I need to have a table matching to get the right ndk_version value based on the version of the recipe.

Something like:

class AndroidNDKConan(ConanFile):
    # Even better if this is defined in the YAML file instead of here?
    @property
    def _ndk_version(self):
        version_map = {
            "r19c": "19.2",
            "r20b": "20.1",
            "r21e": "21.2",
            "r21d": "21.3",
            "r22": "22",
            "r22b": "22.1",
            #  [...]
        }
        return version_map[self.version]
 
    # [...]
    def package_info(self):
        # [...]
        ??? Force self.settings.os.ndk_version = self._ndk_version
        # [...]

I don't know really if this is possible at all though 🤔

@memsharded
Copy link
Member

Force self.settings.os.ndk_version = self._ndk_version

No, not possible, see: https://docs.conan.io/2/knowledge/guidelines.html#forbidden-practices

Settings and configuration (conf) are read-only in recipes: The settings and configuration cannot be defined or assigned values in recipes.

@elvisdukaj
Copy link
Contributor Author

I see, I guess the best option I have then is to put it inside the profile for now. Thank you for the help!

@memsharded memsharded added this to the 2.5.0 milestone Jun 11, 2024
@memsharded
Copy link
Member

I got feedback from other user about the binary incompatibility from different NDK versions, so:

  • I am proposing to the team to add a default ndk_version to Android subsetting
  • It might accept a null value, to not break existing users
  • The incompatibility seems to come from major versions r25, r26, but not from minors a, b. We might want to be able to model both scenarios

@elvisdukaj
Copy link
Contributor Author

Thank you for the feedback! That will be awesome already!

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

Successfully merging a pull request may close this issue.

2 participants