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

Upgrade to Android NDK r20 [ci full] #1916

Merged
merged 1 commit into from Oct 2, 2019
Merged

Upgrade to Android NDK r20 [ci full] #1916

merged 1 commit into from Oct 2, 2019

Conversation

eoger
Copy link
Contributor

@eoger eoger commented Oct 1, 2019

Fixes #1029.
Fixes #1913.
Fixes #1912.

This will need thorough testing as it might break stuff subtly on different Android architectures.

@eoger eoger force-pushed the upgrade-ndk branch 3 times, most recently from dbf88c4 to de73733 Compare October 1, 2019 22:02
@eoger eoger requested a review from a team October 2, 2019 15:08
@eoger eoger marked this pull request as ready for review October 2, 2019 15:08
@eoger
Copy link
Contributor Author

eoger commented Oct 2, 2019

Asking for review: Even though this is not required right now to unblock people's work, I believe we could avoid a lot of pain in the future and pay a little bit of technical debt by using a newer NDK.

Copy link
Contributor

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This looks good, and works for me locally.

docs/howtos/setup-android-build-environment.md Outdated Show resolved Hide resolved
libs/build-all.sh Outdated Show resolved Hide resolved
@@ -20,7 +20,7 @@ fi

if [[ -z "${ANDROID_NDK_ROOT}" ]]; then
echo "Could not find Android NDK:"
echo 'Please install the Android NDK r15c and then set ANDROID_NDK_ROOT.'
echo 'Please install Android NDK r20 and then set ANDROID_NDK_ROOT.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check that it's r20 somehow? Seems very likely people will miss the memo to update this...

Probably worth sending out an email too, tbh.

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 actually check a bit further down the file :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea on the email!

Copy link
Contributor

@vladikoff vladikoff left a comment

Choose a reason for hiding this comment

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

Tested and seems to be fine:

Android 10 - Essential PH1
Kindle Fire
x86 emulator Android 6
x86 emulator Android 5

@eoger eoger merged commit 4fe30da into master Oct 2, 2019
@eoger eoger deleted the upgrade-ndk branch October 2, 2019 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants