-
Notifications
You must be signed in to change notification settings - Fork 240
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
Build Android native libraries through Gradle #1152
Conversation
b30004c
to
1aa32f4
Compare
Ignore this PR for now - I've got some wrestling to do with Travis CI. Edit: CI is working correctly now. |
9a180d1
to
4395f8a
Compare
// Default configuration | ||
} | ||
full { | ||
externalNativeBuild.cmake.abiFilters 'armeabi', 'arm64-v8a', 'x86' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also be including armv7
or its picked up by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said to ignore this PR! ;)
I was unsure about this too, but empirically it seems to pick up armeabi-v7a
from the default configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can not hold a man for his curiosity :).
d808c39
to
204f748
Compare
Ready for review now! Before merging, though, we should merge tangrams/yaml-cpp#8 and use the normal Edit: This has been done now. |
de3855d
to
7afaf7e
Compare
Ping @tallytalwar & @hjanetzek for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good. Have some small questions, I have asked in this review.
Also we can probably squash a bunch of commits here.
cmake { | ||
targets 'tangram' | ||
arguments '-DPLATFORM_TARGET=android', | ||
'-DANDROID_TOOLCHAIN=clang', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang
is the default toolchain as per the docs, do we need to be explicit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang
became the default very recently - I would prefer to leave this here in case someone is using an NDK from before that change.
arguments '-DPLATFORM_TARGET=android', | ||
'-DANDROID_TOOLCHAIN=clang', | ||
'-DANDROID_STL=c++_shared' | ||
cppFlags '-std=c++14', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the docs, we might also need -latomic
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it couldn't hurt, though there are no runtime errors produced by using things from the <atomic>
headers so it's unclear whether it is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I saw when I build, it did not complain. But I would link atomic only because documentation is suggesting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
atomic
is linked in android.cmake
now.
assets.srcDirs = ['../../scenes'] | ||
} | ||
buildTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific issues here with the debug configuration? Based on b859576, it seems it was only temporary.
buildTypes { | ||
debug { | ||
// applicationIdSuffix ".debug" | ||
jniDebuggable true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should'nt the demo app be marked debuggable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point about the "debuggable" flags, I'm not sure of the impact of these flags on either the library or demo module under the new build system, but I can try both and see.
#make android-native-lib ANDROID_ARCH=x86_64 | ||
#make android-native-lib ANDROID_ARCH=mips | ||
#make android-native-lib ANDROID_ARCH=mips64 | ||
make android-sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So previously arm7l
was done during the default build process and other architecture were build only during deployment.
It seems this will build, everything again including arm7l (in other words, arm7l will be builds twice).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arm7l
target will be up-to-date when we encounter it the second time, so nothing needs to be done.
} | ||
debug { | ||
externalNativeBuild { | ||
cmake.arguments '-DCMAKE_BUILD_TYPE=Debug' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these take care of debugging for demo app and also debugging on the device, which was disabled here: b859576
7afaf7e
to
850a076
Compare
I've set up debug configurations for the library and demo app in a way that should allow debugging native code - that is, I've tested the same configuration in a minimal test project and was able to debug native code fro Android studio. However, I have not been able to successfully debug native code from Android Studio yet. Any further ideas are welcome! |
850a076
to
0ecb7c4
Compare
I could squash more of these commits together, but there are actually quite a lot of distinctly meaningful changes here, despite the low number of lines added. I'd rather have many commits that describe these changes in accurate detail. |
0ecb7c4
to
bc6f13f
Compare
~ Ping for review approval ~ |
- add jcenter() repository - update android-maven-gradle-plugin to 1.5 - update Android Gradle plugin to 2.2.3 - move 'buildscript.dependencies' to project Gradle config
- download NDK r13b
7c15c5c
to
ab5e4ec
Compare
This may let us use CI machines with more disk space
Also add debug flags for demo app debug configuration
ab5e4ec
to
52b16fe
Compare
@tallytalwar Can you check over the updated README and merge this when you get a chance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Merging.
Android Studio 2.2 and the associated Gradle components now include a mechanism to execute "external native builds" via CMake. This enables us to build all pieces of our Android SDK, including the native libraries, from a Gradle command. This also allows us to use the latest versions of the Android NDK, which are incompatible with our old Android build system.