Skip to content

Conversation

@gedeagas
Copy link
Contributor

Summary

When I try to run RNTester with Gradle the RNTester Required me to use NDK 20.0.5594570. I can't seem to find an explicit NDK version anywhere in ReactAndroid and RNTester. This PR Aims to add an explicit NDK version to RNTester and ReactAndroid.

Screenshot from 2020-09-19 21-13-17

Changelog

[Android] [Added] - Add an explicit NDK version to RNTester and ReactAndroid.

Test Plan

Build manually from RNTester

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 19, 2020
@react-native-bot react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Sep 19, 2020
@gedeagas
Copy link
Contributor Author

cc @dulmandakh @safaiyeh

@fkgozali what is the NDK version does FB use internally on the RNTester buckconfig? I used the same version specified on the Gradle template file PR that @safaiyeh worked on.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,226,809 0
android hermes armeabi-v7a 6,875,593 0
android hermes x86 7,663,581 0
android hermes x86_64 7,551,365 0
android jsc arm64-v8a 9,371,974 0
android jsc armeabi-v7a 9,012,906 0
android jsc x86 9,234,330 0
android jsc x86_64 9,811,474 0

Base commit: 7b82df2

@analysis-bot
Copy link

analysis-bot commented Sep 19, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: abff021

@fkgozali
Copy link
Contributor

Actually I have a question about this explicit version specification: #29403 (comment)

what is the NDK version does FB use internally on the RNTester buckconfig?

FB uses a different internal version of the NDK, so it will conflict with the version you specify here, hence my question linked above

@fkgozali
Copy link
Contributor

Based on #29403 (comment), I wonder if we can make https://github.com/facebook/react-native/blob/master/ReactAndroid/build.gradle#L249-L271 a little smarter by detecting the version you specify here. If that's possible, that would reduce confusion a lot.

Side note: is there a way to put all these "blessed versions" in one place so that we minimize repetition? E.g. perhaps we could put it in the root project config?

@gedeagas
Copy link
Contributor Author

gedeagas commented Sep 22, 2020

@fkgozali Kindly check I just updated the findNdkBuildFullPath() to only use the explicitly specified NDK. But this means that we will be dropping support for users that install the Android NDK outside of the SDK Manager.

For me, I think this is the best approach, considering that support for ANDROID_NDK_HOME, ndk.dir is deprecated and will be removed in the future.

@friederbluemle
Copy link
Contributor

friederbluemle commented Sep 24, 2020

Thanks for the PR @gedeagas! I think in general we should keep this aligned with whatever the currently used version the Android Gradle plugin defaults to, unless there is a specific reason to use a different version.

For AGP versions 3.5 and later, this page states:

(Recommended) Use the ndkVersion property to set the NDK version.

Since React Native is currently using 3.6.4, we would set ndkVersion to 20.1.5948944, and update it together with the Android Gradle plugin (#29013). Would that work?

image

Also, I don't think it should be inside defaultConfig, only directly inside android. Or did you have any issues?

@friederbluemle
Copy link
Contributor

Also (in case I missed it somewhere in the long discussion) - The point of ndkVersion is to not automatically select whatever NDK is installed locally, but to ensure that a specific state of the repository is always built with the same exact tools.

@gedeagas
Copy link
Contributor Author

Yes, @friederbluemle, you are correct this pr is to ensure we all use the same version of NDK. And prevent people from using the different NDK version than we already specified on the React NativeGradle template file

We should use the same version of NDK on Gradle or Buck to ensure that a specific state of the repository is always built with the same exact tools.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@fkgozali
Copy link
Contributor

Hmm looks like this broke the test_android CI? Can you take a look?

https://app.circleci.com/pipelines/github/facebook/react-native/6463/workflows/0393edd6-4185-490b-a1e1-d7366a43fe83/jobs/169076

FAILURE: Build failed with an exception.

* Where:
Build file '/root/react-native/ReactAndroid/build.gradle' line: 251

* What went wrong:
Could not determine the dependencies of task ':ReactAndroid:packageReactNdkLibs'.
> Could not create task ':ReactAndroid:buildReactNdkLib'.
   > NDK is not installed

@gedeagas gedeagas requested a review from hramos as a code owner September 24, 2020 22:33
@gedeagas gedeagas changed the title Add an explicit NDK version to RNTester and ReactAndroid WIP: Add an explicit NDK version to RNTester and ReactAndroid Sep 24, 2020
@gedeagas
Copy link
Contributor Author

@fkgozali we probably need to wait for this one #29050. The Android dockerfile is already being migrated to use Side by Side NDK.

Compile Native Libs is successful
https://app.circleci.com/pipelines/github/facebook/react-native/6480/workflows/d71dd86d-5783-4d2c-b826-26b202a252af/jobs/169292

fkgozali referenced this pull request Oct 27, 2020
Summary:
This does a few things:
* Remove USE_CODEGEN flag so that TurboModule is enabled by default for RNTester
* Use the codegen output for Java/JNI spec files
* Remove the checked in com.facebook.fbreact.specs Java/JNI files

Changelog: [Changed][Android] RNTester now enables TurboModule by default using codegen.

Reviewed By: RSNara

Differential Revision: D24382083

fbshipit-source-id: 87e3e0581bac3287ef01c1a0deb070c1d7d40f2d
fkgozali added a commit to fkgozali/react-native that referenced this pull request Oct 27, 2020
Summary:
Pull Request resolved: facebook#30259

Before side-by-side NDK is supported by the Circle CI image, pending facebook#29050 & facebook#29987, this local.properties file forces Gradle to use the specified SDK/NDK path. We do this internally at FB CI as well.

This addresses the following failure:
https://app.circleci.com/pipelines/github/facebook/react-native/6851/workflows/9fd91d5d-3f05-4521-93fc-95abd5c84227/jobs/173735

Also, this fixed bad src copy for Buck-building: `//packages/react-native-codegen:setup_cli`

Changelog: [Internal]

Reviewed By: hramos

Differential Revision: D24577333

fbshipit-source-id: 93f11c3e3a3f699415739d0760ee10909eb748ed
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ShikaSD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@fkgozali
Copy link
Contributor

@gedeagas, @ShikaSD will merge this, but we need to adjust the config slightly to support FB CI.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gedeagas in 5b34c98.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 4, 2021
@gedeagas gedeagas deleted the android/ndk branch January 5, 2021 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants