Skip to content

Commit

Permalink
Bump CMake to 3.22.1 to properly honor CMAKE_BUILD_TYPE (#35857)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
cortinico authored and kelset committed Jan 19, 2023
1 parent 1d64766 commit 36e9d87
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 12 deletions.
2 changes: 1 addition & 1 deletion .circleci/Dockerfiles/Dockerfile.android
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# and build a Android application that can be used to run the
# tests specified in the scripts/ directory.
#
FROM reactnativecommunity/react-native-android:6.1
FROM reactnativecommunity/react-native-android:6.2

LABEL Description="React Native Android Test Image"
LABEL maintainer="Héctor Ramos <hector@fb.com>"
Expand Down
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ executors:
reactnativeandroid:
<<: *defaults
docker:
- image: reactnativecommunity/react-native-android:6.1
- image: reactnativecommunity/react-native-android:6.2
resource_class: "xlarge"
environment:
- TERM: "dumb"
Expand Down
15 changes: 11 additions & 4 deletions ReactAndroid/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ def downloadsDir = customDownloadsDir ? new File(customDownloadsDir) : new File(
def thirdPartyNdkDir = new File("$buildDir/third-party-ndk")
def reactNativeRootDir = projectDir.parent

// We put the publishing version from gradle.properties inside ext. so other
// subprojects can access it as well.
ext.publishing_version = VERSION_NAME

// This is the version of CMake we're requesting to the Android SDK to use.
// If missing it will be downloaded automatically. Only CMake versions shipped with the
// Android SDK are supported (you can find them listed in the SDK Manager of Android Studio).
def cmakeVersion = System.getenv("CMAKE_VERSION") ?: "3.22.1"
ext.cmake_version = cmakeVersion

// You need to have following folders in this directory:
// - boost_1_76_0
// - double-conversion-1.1.6
Expand Down Expand Up @@ -226,10 +236,6 @@ final def preparePrefab = tasks.register("preparePrefab", PreparePrefabHeadersTa
it.outputDir.set(prefabHeadersDir)
}

// We put the publishing version from gradle.properties inside ext. so other
// subprojects can access it as well.
ext.publishing_version = VERSION_NAME

task createNativeDepsDirectories {
downloadsDir.mkdirs()
thirdPartyNdkDir.mkdirs()
Expand Down Expand Up @@ -489,6 +495,7 @@ android {

externalNativeBuild {
cmake {
version cmakeVersion
path "src/main/jni/CMakeLists.txt"
}
}
Expand Down
11 changes: 6 additions & 5 deletions ReactAndroid/hermes-engine/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,19 @@ plugins {

group = "com.facebook.react"
version = parent.publishing_version
def cmakeVersion = parent.cmake_version

def cmakeVersion = "3.18.1"
/**
* We use the bundled version of CMake in the Android SDK if available, to don't force Android
* users to install CMake externally.
*/
def findCmakePath(cmakeVersion) {
if (System.getenv("ANDROID_SDK_ROOT")) {
return "${System.getenv("ANDROID_SDK_ROOT")}/cmake/${cmakeVersion}/bin/cmake"
def cmakeRelativePath = "/cmake/${cmakeVersion}/bin/cmake"
if (System.getenv("ANDROID_SDK_ROOT") && new File("${System.getenv("ANDROID_SDK_ROOT")}/${cmakeRelativePath}").exists()) {
return "${System.getenv("ANDROID_SDK_ROOT")}/${cmakeRelativePath}"
}
if (System.getenv("ANDROID_HOME")) {
return "${System.getenv("ANDROID_HOME")}/cmake/${cmakeVersion}/bin/cmake"
if (System.getenv("ANDROID_HOME") && new File("${System.getenv("ANDROID_HOME")}/${cmakeRelativePath}").exists()) {
return "${System.getenv("ANDROID_HOME")}/${cmakeRelativePath}"
}
return "cmake"
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
"prettier": "prettier --write \"./**/*.{js,md,yml,ts,tsx}\"",
"format-check": "prettier --list-different \"./**/*.{js,md,yml,ts,tsx}\"",
"update-lock": "npx yarn-deduplicate",
"docker-setup-android": "docker pull reactnativecommunity/react-native-android:6.1",
"docker-setup-android": "docker pull reactnativecommunity/react-native-android:6.2",
"docker-build-android": "docker build -t reactnativeci/android -f .circleci/Dockerfiles/Dockerfile.android .",
"test-android-run-instrumentation": "docker run --cap-add=SYS_ADMIN -it reactnativeci/android bash .circleci/Dockerfiles/scripts/run-android-docker-instrumentation-tests.sh",
"test-android-run-unit": "docker run --cap-add=SYS_ADMIN -it reactnativeci/android bash .circleci/Dockerfiles/scripts/run-android-docker-unit-tests.sh",
Expand Down

0 comments on commit 36e9d87

Please sign in to comment.