Skip to content

Update build script to Android Studio 3.0.0 #41

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

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Update build script to Android Studio 3.0.0 #41

merged 1 commit into from
Nov 21, 2017

Conversation

ggfan
Copy link
Contributor

@ggfan ggfan commented Nov 20, 2017

Update the android build tool version to 3.0.0 according to:
https://developer.android.com/studio/build/gradle-plugin-3-0-0-migration.html

@ggfan
Copy link
Contributor Author

ggfan commented Nov 20, 2017

@adarshf may you have a look too? thanks

@rschiu
Copy link

rschiu commented Nov 20, 2017

lgtm

README.md Outdated
@@ -32,7 +32,7 @@ Refer to README.md under its directory
Pre-requisites
--------------
- Android N device, API >= 24
- Android Studio 2.2 beta1 or better
- Android Studio 3.0 better (this is a must in order to build the sample here)
Copy link

Choose a reason for hiding this comment

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

Just say the following (no need to repeat that this is a requirement):
- [Android Studio 3.0](https://developer.android.com/studio/index.html) or higher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

README.md Outdated
@@ -32,7 +32,7 @@ Refer to README.md under its directory
Pre-requisites
--------------
- Android N device, API >= 24
Copy link

Choose a reason for hiding this comment

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

"A device running Android 7.0 (API level 24) or higher."

README.md Outdated
@@ -32,7 +32,7 @@ Refer to README.md under its directory
Pre-requisites
--------------
- Android N device, API >= 24
- Android Studio 2.2 beta1 or better
- Android Studio 3.0 better (this is a must in order to build the sample here)
- Android NDK
* [NDK-r12](https://github.com/android-ndk/ndk/wiki), compile as is
Copy link

Choose a reason for hiding this comment

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

Users would simply get the NDK through the SDK manager, no? It may be easier to point users here for the NDK components: https://developer.android.com/studio/projects/add-native-code.html#download-ndk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed.

README.md Outdated
@@ -41,17 +41,14 @@ Test Matrix
------------
| Andrid Studio Version | cmake in SDK| NDK | Result |
Copy link

Choose a reason for hiding this comment

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

I would get rid of this table. We already say that users should use AS 3.0+, and the link I provide in the comment above describes how to install CMake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for open source project, like to get to what we tested with. Mainly there are so many combinations and users would always have different combinations etc., when problem happens, we would know at the time what was tested with, so we could go that configuration to make sure it is still working, then debugging the new combinations

README.md Outdated

Known Issue:
- r13-beta layer source needed to be updated
- Studio Beta cmake 3.6.3133135 having issues with r13-beta

Getting Started
---------------
1. [Download Android Studio](http://tools.android.com/download/studio/canary)
Copy link

Choose a reason for hiding this comment

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

I would remove the first two steps, as you've already included them as prerequisites.

@@ -2,7 +2,7 @@ apply plugin: 'com.android.application'

android {
compileSdkVersion 24
buildToolsVersion '25.0.0'
buildToolsVersion '26.0.2'
Copy link

Choose a reason for hiding this comment

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

please remove

// force debug layer lib is used in packing
compile project(path: ':layerlib', configuration: 'debug')
// compile project(path: ':layerlib', configuration: 'debug')
compile project(':layerlib')
Copy link

Choose a reason for hiding this comment

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

implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah

// force debug layer lib is used in packing
compile project(path: ':layerlib', configuration: 'debug')
// compile project(path: ':layerlib', configuration: 'debug')
Copy link

Choose a reason for hiding this comment

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

If you leave this in here, perhaps include a comment describing why specifying the variant is no longer required. Feel free to point to this link: https://developer.android.com/studio/build/gradle-plugin-3-0-0-migration.html#variant_dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

@@ -17,7 +17,7 @@ android {
publishNonDefault true

compileSdkVersion 24
buildToolsVersion '25.0.0'
buildToolsVersion '26.0.2'
Copy link

Choose a reason for hiding this comment

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

please remove all buildToolsVersion properties.

@@ -13,7 +13,9 @@ android {

ndk.abiFilters 'armeabi-v7a', 'arm64-v8a', 'x86', 'x86_64'
externalNativeBuild {
cmake.arguments '-DANDROID_TOOLCHAIN=clang', '-DANDROID_STL=c++_static'
cmake.arguments '-DANDROID_TOOLCHAIN=clang',
Copy link

Choose a reason for hiding this comment

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

Does this actually work with single quotes? I remember not using double quotes (") being an issue, but I may be mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

works fine for me. double quota is for macro expansion. Single quota is "take as is"

@ggfan ggfan merged commit 2e5750d into master Nov 21, 2017
@ggfan ggfan deleted the update-as-3.0 branch January 14, 2020 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants