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

bump Android NDK to r14b #19893

Closed
wants to merge 4 commits into from
Closed

Conversation

dulmandakh
Copy link
Contributor

@dulmandakh dulmandakh commented Jun 26, 2018

Bump Android NDK to r14b.

NDK tool chain is set to GCC 4.9 (in Application.mk file), because clang fails to build RN.

Below are notable changes to r14b

GCC is no longer supported. It will not be removed from the NDK just yet, but is no longer receiving backports.

and

NDK_TOOLCHAIN_VERSION now defaults to Clang.

CI: https://circleci.com/gh/dulmandakh/react-native/573

Test Plan:

Builds and runs as before

Release Notes:

[ANDROID] [ENHANCEMENT] [NDK] - bump version to r14b

@dulmandakh dulmandakh changed the title Bump Android NDK to r13b bump Android NDK to r13b Jun 26, 2018
@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 Jun 26, 2018
@dulmandakh
Copy link
Contributor Author

RN community, let's make it compile with clang 😄

@react-native-bot react-native-bot added ✅Test Plan Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Jun 26, 2018
@gengjiawen
Copy link
Contributor

gengjiawen commented Jun 26, 2018

This pr #19536 bump ndk to 16, r13b maybe too old.

@dulmandakh
Copy link
Contributor Author

IMHO, NDK 13 requires almost no change in source code and still builds the source code. I see it as a small, but non-disruptive step in supporting newer NDK versions and 64bit CPUs.

@dulmandakh dulmandakh changed the title bump Android NDK to r13b bump Android NDK to r14b Jun 27, 2018
@dulmandakh
Copy link
Contributor Author

dulmandakh commented Jun 27, 2018

Bumped NDK to r14b, which became possible with 8ce758f.

CI is OK https://circleci.com/gh/dulmandakh/react-native/605

I couldn't build with versions above 14, and I'm not proficient enough in C++ to fix build errors.

CC: @hramos

@gengjiawen
Copy link
Contributor

gengjiawen commented Jun 27, 2018

I still prefer to the latest ndk, since it's for all cpu arch, not just arm64. And it can easily installed via android studio.

@dulmandakh
Copy link
Contributor Author

@gengjiawen me too, if you read through revision history NDK is not adding support for CPU architectures but removing them.

NDK is switched from GCC to Clang, GNU C++ standard library to Clang's libc++ and unified headers therefore it might require some code changes. For example, folly github page states that it requires GCC to build. If merged, we'll land clang, libc++ and unified headers to play with.

I think it's better to have small incremental changes rather than nothing or disruptive big change.

@dulmandakh
Copy link
Contributor Author

Please review @hramos @mdvacca

@gengjiawen
Copy link
Contributor

@ulmandakh I think the removed abi won't has effect for this project, since they removed the abi is no one used or deprecated as you can see https://github.com/android-ndk/ndk/wiki/Changelog-r17. I agree with we should small incremental changes, but I think the latest ndk is okay.

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Jul 3, 2018

Please review and merge @fkgozali @hramos @mdvacca . Once merged I would like help with landing Unified Headers introduced in NDK r14 and default in r15.

@facebook-github-bot
Copy link
Contributor

@dulmandakh I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@dulmandakh dulmandakh deleted the bump-ndk-r13b branch August 22, 2018 03:31
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. 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.

4 participants