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

provide NDK integration for CMake as part of the NDK package #463

Closed
DanAlbert opened this issue Jul 20, 2017 · 77 comments
Closed

provide NDK integration for CMake as part of the NDK package #463

DanAlbert opened this issue Jul 20, 2017 · 77 comments
Assignees
Labels

Comments

@DanAlbert
Copy link
Member

DanAlbert commented Jul 20, 2017

@bradking @rcdailey @kravindran

Tracking bug for getting the NDK's CMake support integrated with the stuff built in to CMake.

Based on a chat we had with @bradking, the right approach here is to teach CMake to load most of the knowledge that is currently in our cmake toolchain from the CMake side of things. If CMake is building for the NDK and it finds those files in the NDK (it won't for NDK releases prior to this being fixed), it will import those NDK modules to allow us to override CMake's rules (so we can add features that don't require a new CMake release).

We'll still need a CMake toolchain file in the NDK that uses these plugins for the foreseeable future since we still need to support versions of CMake older than whatever version this new implementation makes it into.

https://android-review.googlesource.com/c/395513/ was the first stab at implementing this. It's definitely bitrotted by now, and it still needs the CMake side of the work to be done (not sure if @kravindran had some WIP commits for this, but I didn't find them).

EDIT 22 Jan 2019

The most up to date plan for this is to ship the Platform/*.cmake files that CMake uses to support Android as part of the NDK itself. The toolchain file will continue to exist as a way to interface to this new implementation. When using an older version of CMake, the toolchain file will need to disable policy CMP0017 (hopefully that's the only workaround that we'll need).

This gives the NDK the control it needs to work around toolchain bugs and update default behaviors while also reducing the friction caused by the toolchain file that does much more than toolchain files were ever intended to do.

@Zingam
Copy link

Zingam commented Oct 7, 2017

Now that AS 3 supports external CMake and hopefully will work in Beta 8 will you prioritize completing the CMake integration?

@DanAlbert
Copy link
Member Author

@kravindran, any update on this?

@alexcohn
Copy link

alexcohn commented Oct 8, 2017

What about 'teaching' cmake to use Android.mk? I mean, let it invoke ndk-build for sub-modules.

@DanAlbert DanAlbert modified the milestones: unplanned, r20 Jul 30, 2018
@DanAlbert
Copy link
Member Author

Unfortunately the Studio engineer that was working on this left the team quite some time ago (hence the lack of updates). I'll most likely be picking this up in r20.

This is something that should get a lot simpler with r19 since most of the knowledge that CMake currently needs is just going to be set as toolchain defaults. I'm guessing I won't have time to fix both the NDK and CMake in r19 though, which is why this is being slated for r20 instead. If I'm wrong and have time in r19, I'll pull this work in to that release.

@DanAlbert DanAlbert self-assigned this Jul 30, 2018
@Zingam
Copy link

Zingam commented Jul 31, 2018

This issue should be a priority really. I hope that it won't get postponed too much again for some reason.

@DanAlbert
Copy link
Member Author

While I don't disagree, I do wonder: why do folks think this is a priority? It's a priority for me because it causes a ton of confusion, especially for beginners (I 100% agree that it's absurd that this is a situation that even needs to be explained). Is that your reason? I would imagine that it doesn't matter much after the initial confusion is cleared up since I don't suspect people care how it's implemented, just that they have something that works, which is the case today.

Regardless, yes, this is approaching the near term road map. The r19 toolchain updates (i.e. not needed to pass a hundred flags to make clang work) are a part of this, it's just not the complete project.

@alexcohn
Copy link

alexcohn commented Jul 31, 2018

To begin with, there should be easily discoverable instructions on how to build something for Android with CMake from command line (i.e. not through Android Studio).

@rcdailey
Copy link

rcdailey commented Jul 31, 2018

I worked a lot with Dan and the CMake guys on this a few months ago, which is why this issue exists. I struggled getting CMake's built-in Android support to work with the NDK because CMake missed a lot of edge-case type stuff that the toolchain was doing. So there were a lot of cases where builds would fail and I just couldn't get it working.

From the developer team perspective, when the NDK changes, CMake also has to change (if not using the toolchain). Right now the NDK developers update just 1 project, but now they somehow need to update an unrelated one any time they make a change to the NDK. I'm also not sure what the backward compatibility implications are (looks like it can be handled in Modules/Platform/Android-Determine.cmake. I don't have much insight here but it seems like more management overhead for the NDK project, and there wasn't an immediate need for it. I'm not sure how things will work going forward with that additional dependency on CMake.

From my perspective, the toolchain isn't much harder to use than the built-in version. The parameters are mostly the same. Furthermore, you likely have build scripts to help generate projects using CMake for the NDK platform, which can simplify/hide the details of how you invoke CMake to build for those platforms. There is no sense of urgency from my perspective. I'm able to use the newest version of CMake today with the NDK's provided toolchain just fine.

In terms of "easily discoverable instructions", it already is. You only have two places to look:

So: Documentation is fine, there's no urgency. To be honest, the only reason I think this issue even exists is at the request of the CMake developers. They've asserted (and I agree) that toolchain files are not supported to (and were not designed to) do a lot of system introspection. They're meant to describe some requirements for the platform but that's it. It's supposed to be a simple 20 line file (tops). But the NDK toolchain obviously violates those philosophies, so the developers there are agreeing to follow the CMake principles and treat their NDK as a platform in CMake, the same way it treats linux, windows, etc.

Maybe I'm missing some important aspects of this, but as far as my involvement goes and as a user of both CMake and Android NDK, that's my understanding.

@alexcohn
Copy link

alexcohn commented Jul 31, 2018

Note that https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-android-with-the-ndk is quite outdated (fits NDK r14 or earlier). Even if same instructions work for latest NDK, they refer to many obsolete settings and features.

@rcdailey
Copy link

rcdailey commented Jul 31, 2018

How do you know it's the documentation being out of date as opposed to just lack of support for newer NDKs? Again this goes back to the fact that CMake has to be changed each time the NDK changes. If no one is doing that actively (which it doesn't seem they are) then this is expected. That's not an issue of documentation, it's an issue of continuity of support.

@Zingam
Copy link

Zingam commented Aug 18, 2018

@DanAlbert Our project supports 1, 2, 3, 4, 5... etc. current and even last generation platforms and we are less developers than people who comment here. So every little hiccup with the tools piles up and becomes a burden.
There is this platform vendor S, whose tools are implemented in such way that all you need to do is: 1. Install the SDK, 2. Provision the DevKit, 3. Start a new project - and everything just works. Which is an example that things could be done properly. Then there are other vendors like M, which everytime whenever they release a new SDK everything falls apart.

I really would prefer to work on developing an end-user product than being a build systems and tools virtuoso.

@enh
Copy link
Contributor

enh commented Aug 20, 2018

I really would prefer to work on developing an end-user product than being a build systems and tools virtuoso.

indeed. that's probably true for everyone :-)

we are trying to get to the point where we're closer to "company S" than "company M" on the spectrum, but unfortunately we're having to dig our way out of a lot of historical problems. part of the big idea behind NDK r19 is that at that point all (or at least most) of the configuration disappears into the toolchain.

with r18 we're pretty close: there's one compiler, one libc++, one set of headers... r19 moves a lot of the build system configuration into the toolchain (which, sadly, will cause disruption for anyone with a custom build system, but should mean that that's the last time we disrupt them in that way). more critically for this specific bug, r19 is when we plan to work with upstream cmake so that they "just work" with r19 and future NDKs.

longer-term there might be some more disruption (we currently have different linkers for different architectures, and we'd like to get everyone on to lld), but again, that's disruption in the name of long-term calm.

sorry that you have to live through the turbulent times, but hopefully it's a consolation that the very purpose of the disruption is to move to the "company S" kind of world :-)

@Zingam
Copy link

Zingam commented Sep 19, 2018

In that context if external CMake is used in Android Studio why is it necessary to set the version of CMake:

            ...
            version "3.7.1"
        }

If CMake can do that already: cmake_minimum_required(VERSION 3.6.0).

@rcdailey
Copy link

rcdailey commented Sep 19, 2018 via email

@Zingam
Copy link

Zingam commented Sep 19, 2018

@rcdailey I know that to use an external CMake I need to specify "version" but I don't see why instead of using a Boolean value it is necessary to type in a specific unportable number. Is the build system somehow version dependent?

@rcdailey
Copy link

rcdailey commented Sep 19, 2018 via email

@enh
Copy link
Contributor

enh commented Sep 19, 2018

Ah I see what you mean. Yes, I had the same issue, because the "version" attribute does not follow the same semantics as cmake_minimum_required() (the gradle attribute enforces an exact match, while CMake only requires a minimum, so it's less restrictive). Gradle's functionality should not interfere or supersede any functionality in CMake from my standpoint. I filed a bug about this issue on the Google bug tracker here: https://issuetracker.google.com/issues/110693527 I encourage you to contribute a comment to that issue. I had also been emailing Jomo at Google directly to discuss this, since he's responsible for the CMake integration functionality of the gradle plugin. You may consider opening another issue for better visibility since he's since closed the one I linked above.

looking at that bug, it looks like there's new syntax so you can say "3.7+" to mean >= 3.7... isn't that sufficient?

@rcdailey
Copy link

looking at that bug, it looks like there's new syntax so you can say "3.7+" to mean >= 3.7... isn't that sufficient?

I thought so too, until Jomo replied after that with:

When we add a more recent version to the SDK then that version will be used (if installed)

So basically if the internal CMake within the SDK is upgraded, "3.7+" won't mean "use external CMake" anymore. And this would be a functional issue, if say, in the build.gradle it was "3.7+" (to force usage of external CMake) and in CMakeLists.txt it specified cmake_minimum_required(VERSION 3.12) and they upgraded the bundled CMake to something like version 3.9. CMake would all of a sudden fail because the minimum required CMake version is no longer met, even though a viable candidate version is on PATH in the host system.

@DanAlbert
Copy link
Member Author

I've started looking in to this. I have a toy implementation that works, but it raised a handful of questions, and I could use some help clarifying what people are hoping to get out of this.

I think the two problems we're trying to solve are:

  1. User confusion over the fact that there are two of these and which one should be used.
  2. Version slip between CMake and the NDK can raise incompatibility issues.

I'm operating under the following assumptions:

  1. We need to continue to support the NDK toolchain file interface. This is already widely adopted and removing it would be very disruptive.
  2. We need to continue to support the CMake built-in interface. The whole point of using CMake with the NDK is to reduce the Android-specific nature of the NDK, so we should be adopting a format closer to what CMake uses for other platforms.
  3. The NDK needs to continue to support CMake versions back to 3.6. It's still the default in the SDK iirc.
  4. CMake needs to continue to support old NDK versions. CMake still supports pre-r11 NDKs aiui, so I don't see support for pre-r20 NDKs going away anytime soon.

The problem that I've run in to is that I don't see a way to maintain those 4 assumptions without making problem 1 worse. If we want to support new and old versions of CMake, as well as toolchain and non-toolchain interfaces to each, then the problem goes from toolchain vs not toolchain vs not X cmake new enough vs not.

If we're willing to give up the 3rd assumption then we can get back to toolchain vs not toolchain, and at this point the toolchain would just be a front end to the not toolchain version to keep the interface stable rather than an entirely separate implementation. If we go down this route, short term this problem gets worse but long term it gets a little bit better.

Is requiring CMake 3.13+ to work with the NDK reasonable (assuming that we'd have a transition period of 6-12 months where we kept working with older versions)? How often are people in a state where they can't update CMake? I know various Linux distros will not always have a modern version available in their repositories (this would be an annoyance for me if I were in that situation), but a newer version can always be installed from their website.

So... is this worth doing? I'm trying to get a handle on whether this is something that helps you folks or if it's just a technical debt cleanup that only helps me and Brad.

@Zingam: You've been the most outspoken on wanting this fixed (hopefully I didn't misinterpret @alexcohn's and @rcdailey's posts, but it sounds like they both think better docs would solve the problem), but at the same time your post explaining why you want this fixed seems counterintuitive to me. Your complaint (100% valid, in case it sounds like I think it isn't) is that you want to avoid adapting to constant tooling changes that don't offer anything. So the part I don't get is: what do you get out of this? The only thing I see this changing for you is that it gives you the option of adapting to a new tooling change. From my understanding, the best thing for you would be for me to close this bug and not change the CMake tooling at all. I've clearly misunderstood something, but I'm not sure what.

@alexcohn
Copy link

alexcohn commented Sep 19, 2018

If you ask me, I don't see why explicit choice of external CMake may be important. It's understandable that somebody may choose the newest CMake that is not (yet) bundled with Android SDK. Or decide to save disk space and choose not to download CMake with AS SDK Manager. But if you specify "3.7+", why should it matter which copy of compliant CMake is picked up?

@rcdailey
Copy link

rcdailey commented Sep 20, 2018

But if you specify "3.7+", why should it matter which copy of compliant CMake is picked up?

@alexcohn As I explained in my previous reply, the version it picks up might meet the minimum version requirements specified by build.gradle, but not the minimum required specified by the CMakeLists.txt. Please review the detailed example I gave earlier, I feel it explains the issue pretty well.

On to @DanAlbert's response: I apologize as technically mentioning the Gradle issues here is off topic. I got off on a tangent starting with @Zingam's question related to Gradle. I think it's worth discussing, but the NDK developers probably won't be able to help with that as it's an issue specific to Android Studio & Gradle, as far as I see it.

Is requiring CMake 3.13+ to work with the NDK reasonable (assuming that we'd have a transition period of 6-12 months where we kept working with older versions)?

I like your idea of defining a cut-off point. We already know that even though NDK support exists in CMake, it wasn't really ready. You and I have discussed a couple of issues in the past where the toolchain just operates superiorly to CMake. There are actual issues that appear when using CMake's Android support that go away when switching to the toolchain file. I mean, given this, we don't want to encourage people to use it, especially since we can't go back in time and make it work better. So:

  1. A specific version of CMake (let's say 3.13, for the sake of an example) is "blessed" by the NDK team to represent the first release of CMake with acceptable/stable NDK support
  2. Versions of CMake 3.13 and greater are expected to support versions of the NDK back to a NDK-team-defined minimum NDK version (you used r11 as an example I think).
  3. Using the toolchain file with CMake 3.13 and greater should issue an error, and a diagnostic that points the user to the Android Platform page of the CMake docs to assist in the transition (you could make this a warning if you prefer to deprecate it for a time).
  4. Versions of CMake prior to 3.13 should use the toolchain file, not the built in support. You unfortunately won't be able to enforce this, the user will just have to notice something isn't working right.

Also worth noting that I think "updating cmake" might not be as invasive as it sounds (in the case where you're worried people can't update it). From what I saw in the CMake code, all the Android platform stuff is stored in several CMake scripts provided with the installation. So if it's easier for them to update that in isolation, or perhaps provide their own (which CMake also defines a structure for) then that may make it a non-issue.

This presents an interesting challenge to the CMake developers, where it might make more sense to allow distribution of cross compilation platform support scripts independently of CMake itself. In other words, allowing groups of CMake scripts under Modules/Platform to be bundled and distributed outside of CMake itself. This might give you the best of both worlds:

  1. CMake still controls the platform toolchain behavior
  2. The owners/maintainers of the platform toolchain logic can have more ownership & control over those scripts, and distribute them to older versions of CMake (assuming the toolchain scripts themselves are backward compatible)

hopefully I didn't misinterpret @alexcohn's and @rcdailey's posts, but it sounds like they both think better docs would solve the problem

Actually I have an interest in not using the toolchain either, because of a few reasons:

  1. I don't have to worry about the path to the NDK in my build scripts (right now I need it to point CMake to the toolchain file).
  2. I don't have to make copies of the toolchain script and use it locally (instead of the NDK provided one) in cases where bugs come up
  3. Overall I just find CMake's built in support a little easier to understand and use, although most options aren't that much different (ABI, Min API, things like that).

I definitely am on board with 1 system to rule them all. We have 2 right now which is confusing and unnecessary, I also think leaving it the way it is hurts users due to the confusion of choice and conflicting documentation between NDK project and CMake project.

@DanAlbert
Copy link
Member Author

Using the toolchain file with CMake 3.13 and greater should issue an error, and a diagnostic that points the user to the Android Platform page of the CMake docs to assist in the transition (you could make this a warning if you prefer to deprecate it for a time).

This is something I'm fairly certain we can't do. You don't even have to look outside this thread to find people who think the best thing we could do to the NDK is not change the interface :)

This presents an interesting challenge to the CMake developers, where it might make more sense to allow distribution of cross compilation platform support scripts independently of CMake itself. In other words, allowing groups of CMake scripts under Modules/Platform to be bundled and distributed outside of CMake itself.

The implementation I have actually does more or less this. The files in Modules/Platform just delegate to the same files that live in the NDK. It seems to work pretty well.

Actually I have an interest in not using the toolchain either, because of a few reasons:

  1. I don't have to worry about the path to the NDK in my build scripts (right now I need it to point CMake to the toolchain file).
  2. I don't have to make copies of the toolchain script and use it locally (instead of the NDK provided one) in cases where bugs come up

Isn't that still the case using the built-in stuff? CMake still needs to know where the NDK is, and you'll still need to locally modify something if you want to get early bug fixes.

  1. Overall I just find CMake's built in support a little easier to understand and use, although most options aren't that much different (ABI, Min API, things like that).

Anything we can improve in the toolchain file in the mean time?

@DanAlbert
Copy link
Member Author

@bradking Thanks for the update and sorry for not having had the time to resolve this yet.

The proposed change is what I had in mind, and I'd played a bit with something similar back when I was first looking at this with good results.

Agreed that simpler is better. We'll have a look at exactly what's left for the r19+ toolchains. We might be able to do something simpler there, but won't know until we look. Not sure exactly when that will be. Might be something @hhb can look at soon.

@enh-google
Copy link
Collaborator

...and in the meantime, if you have specific examples of things we could do that would help make even more of "CMake's builtin knowledge of Android flags and such ... no longer needed", let us know.

@bradking
Copy link

if you have specific examples of things we could do that would help make even more of "CMake's builtin knowledge of Android flags and such ... no longer needed",

@enh-google off the top of my head the main remaining problem is all the abi-specific flags like -march=armv7-a and -mstackrealign. The Android-specific toolchains should be able to know all the Android ABI conventions based only on the target triple. Build systems should not have to all memorize that information. If the toolchain were to apply such settings automatically then they would be maintained inside the NDK regardless of what build system is used.

@DanAlbert
Copy link
Member Author

-mstackrealign already is managed by clang, I thought. Maybe I'm misremembering.

We do need to find some time to finish pushing things into the driver though.

@DanAlbert DanAlbert modified the milestones: r23, unplanned Apr 13, 2020
@DanAlbert
Copy link
Member Author

DanAlbert commented Apr 13, 2020

(I'm hoping someone can start working on this for r23, but keeping in unplanned until we actually have someone free for it)

@ericoporto
Copy link

Any chance this is picked up?

@DanAlbert
Copy link
Member Author

Not yet. I think it's what @hhb is looking at after LLDB.

@DanAlbert DanAlbert assigned hhb and unassigned DanAlbert Aug 14, 2020
@hhb
Copy link
Collaborator

hhb commented Oct 27, 2020

To make sure everyone is on the same page: currently there are two ways to build for Android using cmake.

  1. Using toolchain file provided by NDK: -DCMAKE_TOOLCHAIN=/path/to/android.toolchain.cmake
  2. Using cmake upstream support: -DCMAKE_SYSTEM_NAME=Android

They have totally different implementation and depend on each other's internal. The goal of this work is to merge the implementation:

  1. android.toolchain.cmake will become a layer on top of cmake upstream support. It will provide the ANDROID_* interface.
  2. some mechanism is needed for NDK to hook into cmake, to provide additional information.

Since android.toolchain.cmake will be built on top of cmake Android support. As long as cmake interface is stable (likely), thing should work. On the other hand, if a NDK bug requires a change on cmake, people will be forced to upgrade their cmake. That's what we should try to avoid.

I plan to add hooks similar to cmake1112. For each related cmake module, we add both pre and post hook like:

if(CMAKE_ANDROID_NDK)
  include(${CMAKE_ANDROID_NDK}/build/cmake/hooks/{pre,post}/Android-Determine.cmake OPTIONAL)
endif()

(See also cmake4372, cmake1092, ndk395513)

In most cases, the hooks will only add cxx / linker flags. Because flags are more frequently changed on NDK side. It is easier to manage them in NDK. But we can also use the hooks to change cmake behaviors. Some examples:

e043c26 Switch to llvm-strip, advocate its use.
48b8e52 Switch to llvm-ar
cmake sets _CMAKE_TOOLCHAIN_PREFIX and prefers ${triple}-ar. With hooks, NDK can set CMAKE_AR / CMAKE_STRIP, until triple prefixed binutils are removed, or some fix is applied upstream in cmake.

f563d92 Fix find_library search order.
This will have to be fixed in cmake. But we may hack CMAKE_SYSTEM_*_PATH in a hook for older cmake.

@DanAlbert
Copy link
Member Author

And a further note that wherever possible we'll fix those extra flags directly in Clang rather than needing to patch them into CMake like this. Ideally those files will be a no-op when using an up to date CMake. Toolchain updates take a long time though so that's not always practical.

@DanAlbert
Copy link
Member Author

The NDK side of this merged (for r23) yesterday. It requires CMake 3.20.

@rprichard
Copy link
Collaborator

The NDK side of this merged (for r23) yesterday. It requires CMake 3.20.

I think we bumped the minimum up to 3.21 to fix #1478.

cortinico added a commit to cortinico/react-native that referenced this issue Jan 17, 2023
Summary:
It seems like there is an incompatibility between NDK 23 (shipped in 0.71)
and the usage of custom `CMAKE_BUILD_TYPE` we do for Hermes.

Specifically the `-DCMAKE_BUILD_TYPE=Release` we specify for the debug
variant of Hermes is partially ignored by the new Android native build toolchain.
See android/ndk#463 for mentions on how the
toolchains requires CMake 3.20+

As AGP 7.3 defaults to use CMake 3.18 unless specified, and NDK 23 unless specified.
AGP 7.4 defaults to use CMake 3.22 unless specified, and NDK 23 unless specified.
See: https://developer.android.com/studio/releases/gradle-plugin#7-4-0

Here I'm:
1. Bumping the docker image to an image that contains the CMake 3.22
2. Updating the logic for building `react-native` & `hermes-engine` to use 3.22
3. Provide fallbacks if the user specified `CMAKE_VERSION`

Template tests will run on AGP 7.3 and will still use CMake 3.18, but I forecast
no problem there as the user is not supposed to specify custom `CMAKE_BUILD_TYPE`.
This is only a problem as we build `hermes-engine` with custom build types.

Changelog:
[Android] [Fixed] - Bump CMake to 3.22.1 to properly honor CMAKE_BUILD_TYPE

Differential Revision: D42544864

fbshipit-source-id: 9dca41944a506f89af8b52b614c29ed03ecb9202
cortinico added a commit to cortinico/react-native that referenced this issue Jan 18, 2023
Summary:
Pull Request resolved: facebook#35857

It seems like there is an incompatibility between NDK 23 (shipped in 0.71)
and the usage of custom `CMAKE_BUILD_TYPE` we do for Hermes.

Specifically the `-DCMAKE_BUILD_TYPE=Release` we specify for the debug
variant of Hermes is partially ignored by the new Android native build toolchain.
See android/ndk#463 for mentions on how the
toolchains requires CMake 3.20+

As AGP 7.3 defaults to use CMake 3.18 unless specified, and NDK 23 unless specified.
AGP 7.4 defaults to use CMake 3.22 unless specified, and NDK 23 unless specified.
See: https://developer.android.com/studio/releases/gradle-plugin#7-4-0

Here I'm:
1. Bumping the docker image to an image that contains the CMake 3.22
2. Updating the logic for building `react-native` & `hermes-engine` to use 3.22
3. Provide fallbacks if the user specified `CMAKE_VERSION`

Template tests will run on AGP 7.3 and will still use CMake 3.18, but I forecast
no problem there as the user is not supposed to specify custom `CMAKE_BUILD_TYPE`.
This is only a problem as we build `hermes-engine` with custom build types.

Changelog:
[Android] [Fixed] - Bump CMake to 3.22.1 to properly honor CMAKE_BUILD_TYPE

Reviewed By: cipolleschi

Differential Revision: D42544864

fbshipit-source-id: d9840b4a14baabf5d20d21d71cc86e2e73a8d0ea
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jan 18, 2023
Summary:
Pull Request resolved: #35857

It seems like there is an incompatibility between NDK 23 (shipped in 0.71)
and the usage of custom `CMAKE_BUILD_TYPE` we do for Hermes.

Specifically the `-DCMAKE_BUILD_TYPE=Release` we specify for the debug
variant of Hermes is partially ignored by the new Android native build toolchain.
See android/ndk#463 for mentions on how the
toolchains requires CMake 3.20+

As AGP 7.3 defaults to use CMake 3.18 unless specified, and NDK 23 unless specified.
AGP 7.4 defaults to use CMake 3.22 unless specified, and NDK 23 unless specified.
See: https://developer.android.com/studio/releases/gradle-plugin#7-4-0

Here I'm:
1. Bumping the docker image to an image that contains the CMake 3.22
2. Updating the logic for building `react-native` & `hermes-engine` to use 3.22
3. Provide fallbacks if the user specified `CMAKE_VERSION`

Template tests will run on AGP 7.3 and will still use CMake 3.18, but I forecast
no problem there as the user is not supposed to specify custom `CMAKE_BUILD_TYPE`.
This is only a problem as we build `hermes-engine` with custom build types.

Changelog:
[Android] [Fixed] - Bump CMake to 3.22.1 to properly honor CMAKE_BUILD_TYPE

Reviewed By: cipolleschi

Differential Revision: D42544864

fbshipit-source-id: efd0f51120370fb808337c201df31d71f4ddfdbc
kelset pushed a commit to facebook/react-native that referenced this issue Jan 19, 2023
Summary:
Pull Request resolved: #35857

It seems like there is an incompatibility between NDK 23 (shipped in 0.71)
and the usage of custom `CMAKE_BUILD_TYPE` we do for Hermes.

Specifically the `-DCMAKE_BUILD_TYPE=Release` we specify for the debug
variant of Hermes is partially ignored by the new Android native build toolchain.
See android/ndk#463 for mentions on how the
toolchains requires CMake 3.20+

As AGP 7.3 defaults to use CMake 3.18 unless specified, and NDK 23 unless specified.
AGP 7.4 defaults to use CMake 3.22 unless specified, and NDK 23 unless specified.
See: https://developer.android.com/studio/releases/gradle-plugin#7-4-0

Here I'm:
1. Bumping the docker image to an image that contains the CMake 3.22
2. Updating the logic for building `react-native` & `hermes-engine` to use 3.22
3. Provide fallbacks if the user specified `CMAKE_VERSION`

Template tests will run on AGP 7.3 and will still use CMake 3.18, but I forecast
no problem there as the user is not supposed to specify custom `CMAKE_BUILD_TYPE`.
This is only a problem as we build `hermes-engine` with custom build types.

Changelog:
[Android] [Fixed] - Bump CMake to 3.22.1 to properly honor CMAKE_BUILD_TYPE

Reviewed By: cipolleschi

Differential Revision: D42544864

fbshipit-source-id: efd0f51120370fb808337c201df31d71f4ddfdbc
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this issue May 22, 2023
Summary:
Pull Request resolved: facebook#35857

It seems like there is an incompatibility between NDK 23 (shipped in 0.71)
and the usage of custom `CMAKE_BUILD_TYPE` we do for Hermes.

Specifically the `-DCMAKE_BUILD_TYPE=Release` we specify for the debug
variant of Hermes is partially ignored by the new Android native build toolchain.
See android/ndk#463 for mentions on how the
toolchains requires CMake 3.20+

As AGP 7.3 defaults to use CMake 3.18 unless specified, and NDK 23 unless specified.
AGP 7.4 defaults to use CMake 3.22 unless specified, and NDK 23 unless specified.
See: https://developer.android.com/studio/releases/gradle-plugin#7-4-0

Here I'm:
1. Bumping the docker image to an image that contains the CMake 3.22
2. Updating the logic for building `react-native` & `hermes-engine` to use 3.22
3. Provide fallbacks if the user specified `CMAKE_VERSION`

Template tests will run on AGP 7.3 and will still use CMake 3.18, but I forecast
no problem there as the user is not supposed to specify custom `CMAKE_BUILD_TYPE`.
This is only a problem as we build `hermes-engine` with custom build types.

Changelog:
[Android] [Fixed] - Bump CMake to 3.22.1 to properly honor CMAKE_BUILD_TYPE

Reviewed By: cipolleschi

Differential Revision: D42544864

fbshipit-source-id: efd0f51120370fb808337c201df31d71f4ddfdbc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

14 participants