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

[poc] CMake + Android #7843

Merged
merged 36 commits into from Oct 29, 2020
Merged

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Oct 7, 2020

Changelog: Feature: Add POC on a toolchain for Android (using CMake provided modules).
Docs: conan-io/docs#1902

Close #7809

There are two main different approaches to compile to Android using CMake, both of them should be equivalent:

Not sure which one is better, here it is implemented the first one. Do we want to let the user choose?


  • There are many TODOs to work on yet

@jgsogo jgsogo added this to the 1.31 milestone Oct 7, 2020
@jgsogo jgsogo self-assigned this Oct 7, 2020
return CMakeNativeToolchain(conanfile=conanfile, **kwargs)
else:
# Exceptions to cross-building scenarios
if conanfile.settings.os == 'Windows' and conanfile.settings_build.os == 'Windows':
Copy link
Contributor

Choose a reason for hiding this comment

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

why exceptions for Windows? anyway, this sounds like out of scope for Android, don't understand why it should be changed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More about it here (#7841), this PR is built on top of that one. But, specifically, comments related to the Windows exception here (reviews and comments: #7827), current Conan behavior doesn't consider a cross-building scenario when going from x64 to x86 in Windows or Linux.

from .base import CMakeToolchainBase


class CMakeAndroidToolchain(CMakeToolchainBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

so, do we need Toolchain class for every OS? this doesn't scale, and won't work with custom settings. is it our general strategy going forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it can scale much better than adding if/else conditions inside the methods: we can compose templates and classes, and override methods. Otherwise, we probably end up with the template full of {% if something %}{{ something }}{% endif %} (like now) and methods full of if/else (like they are now in the build-helepers): harder to maintain, harder to reason about and really hard to modify them in parallel (many of us are working on toolchains right now).

At least, I think it would be convenient while we work in parallel on different toolchains, once we now the features around the toolchains we can think if this is the best implementation or not and refactor it.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like I don't understand the model fully. I was thinking like we are going to use CMakeToolchain, possibly specifying some additional things like base toolchain and additional definitions, if needed. for instance, for Android, it may look like:

tc = CMakeToolchain(self)
tc.base = os.path.join(android_ndk, "build", "cmake", "android.toolchain.cmake")
android_variables = {"ANDROID_PLATFORM": "android-21", "ANDROID_ABI": "arm64-v8a", "ANDROID_STL=c++_static"}
tc.variables += android_variables

similarly, we may use the same approach for other platforms. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We really don't know, that's the main purpose of implementing different toolchains, to realize what they need and try to define an interface that satisfies all the different needs. But first, we need those POCs to see different scenarios.

IMHO, any recipe that builds with CMake should be exactly the same, nothing more than this (and everything could be simplified to a declaration):

image

and then we need to provide a way to build this package using other platforms and/or toolchains without the need to modify the recipe itself. Some recipes might require some tweaks, but those should be exceptions.

Having this in mind there are two approaches needed:

  • a way to inject a custom toolchain from outside (defined in a profile or coming from a build-requires): there is no POC for this, but IMO it should be built-in, it shouldn't require to modify the recipe at all.
  • some well-known scenarios can be managed by Conan (I would say that those that are listed in the default settings.yml). We can implement everything needed for Android behind the scenes as this PR does.

From this PR I learn two things:

  • the class-based + inheritance approach is cleaner, I don't need to modify the base template at all, no if/else, no increase in complexity, implementation is straightforward (right now it is intentionally duplicating code).
  • to work with Android we need the ANDROID_NDK path, we need to figure out how to pass that information to the toolchain implementation (right now it will be via environment variable).

_template_project_include = '' # TODO: This file is not useful to Android, there is no MSVC runtime MD/MT

# TODO: Factorize with the native one
_template_toolchain = textwrap.dedent("""
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 one of my concerns, that this diverge too early, and it is difficult to understand common parts and specialization parts, and fixes in other platforms fail to apply to others.

Yes, please factorize this so we can better see isolated the differences of the Android toolchain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's another PR to develop and yes, it is totally necessary (WIP). From this PR we should take some learnings about the toolchain feature, the same for the rest of PRs related to CMake toolchain. In this one I'm learning.

You will love how it looks like after the refactor 😄

@memsharded memsharded added this to To do in Toolchain Oct 20, 2020
@memsharded memsharded moved this from To do to In Progress in Toolchain Oct 20, 2020
conans/client/toolchain/cmake/android.py Outdated Show resolved Hide resolved
libcxx_str = str(self._conanfile.settings.compiler.libcxx)
return libcxx_str # TODO: only 'c++_shared' y 'c++_static' supported?

def _guess_android_ndk(self):
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong, not returning anything?

@jgsogo jgsogo marked this pull request as ready for review October 29, 2020 13:32
@memsharded memsharded merged commit 8bbd75c into conan-io:develop Oct 29, 2020
Toolchain automation moved this from In Progress to Done Oct 29, 2020
@jgsogo jgsogo deleted the poc/toolchain-android branch October 29, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Toolchain
  
Done
Development

Successfully merging this pull request may close these issues.

[feature] CMakeToolchain + Android POC
3 participants