Improving examples build configs and documentation. #155
Conversation
| The Android example app can be build with bazel: `bazel build | ||
| //examples:android-app`, and then install to a device with `adb install | ||
| bazel-bin/examples/android-app.apk` | ||
| You need to install the Android SDK 30.0.2 and the NDK 21.4.7075529. You need both `ANDROID_SDK_HOME` and `ANDROID_NDK_HOME` env variables set. |
There was a problem hiding this comment.
Is there any reason we require these versions? I don't remember the example app uses any new features that's only available in these versions.
There was a problem hiding this comment.
Yeah, the sdk version is hardcoded here:
djinni/bzl/android_configure.bzl
Line 27 in c7b0a39
The NDK versions here:
https://github.com/Snapchat/djinni/blob/c7b0a3978ebe34cc7b5e8323c0ecab5b75017ba7/bzl/android_configure.bzl#L37C17-L37C17
21.4.7075529 specifically is the latest update to that version.
A later NDK should be backwards compatible, however the bazel version specified to build this project only supports up to version 21, and warns (fiercely) that errors can be introduced by using a later NDK version.
There was a problem hiding this comment.
I just did a quick test and find that android example build still works if I complete remove the hard coded versions
--- a/bzl/android_configure.bzl
+++ b/bzl/android_configure.bzl
@@ -23,8 +23,6 @@ def _android_autoconf_impl(repository_ctx):
native.android_sdk_repository(
name="androidsdk",
path="{}",
- api_level=30,
- build_tools_version="30.0.2",
)
""".format(sdk_home)
@@ -33,8 +31,7 @@ def _android_autoconf_impl(repository_ctx):
ndk_rule = """
native.android_ndk_repository(
name="androidndk",
- path="{}",
- api_level=21,
+ path="{}"
)
""".format(ndk_home)
I think we probably should just remove these as we don't really use anything that requires a specific version.
There was a problem hiding this comment.
I just think we should avoid putting hard coded versions in here because they needlessly prevent people from building the code with whatever they already have in their system.
There was a problem hiding this comment.
I agree, that is a better solution.
Let me try that.
| @@ -53,7 +53,7 @@ android_library( | |||
| srcs = glob(["handwritten-src/java/com/dropbox/textsort/*.java"]), | |||
| custom_package = "com.dropbox.textsort", | |||
| manifest = "android/app/src/main/AndroidManifest.xml", | |||
| resource_files = glob(["android/app/src/main/res/**"]), | |||
| resource_files = glob(["android/app/src/main/res/**"], exclude=["android/app/src/main/res/.DS_Store"]), | |||
There was a problem hiding this comment.
Does this cause builds to fail?
There was a problem hiding this comment.
yeah, basically if Mac Os adds these .DS_Store files, then the glob is picking them up and failing the build.
There was a problem hiding this comment.
This is strange. Do you mean Macos will put files in a source tree that cause bazel based projects to fail?
There was a problem hiding this comment.
Exactly
Those files in the gitignore already:
https://github.com/Snapchat/djinni/blob/main/.gitignore#L1
So they do not get checked in by mistake, but still they can break the build process.
| @@ -428,6 +428,7 @@ | |||
| GCC_PRECOMPILE_PREFIX_HEADER = YES; | |||
| GCC_PREFIX_HEADER = "TextSort/TextSort-Prefix.pch"; | |||
| INFOPLIST_FILE = "TextSort/TextSort-Info.plist"; | |||
| IPHONEOS_DEPLOYMENT_TARGET = 12.0; | |||
There was a problem hiding this comment.
any reason for requiring 12.0?
There was a problem hiding this comment.
Latest XCode won't compile it without a min deployment target.
My reasoning was that iOS 12 is new enough to be supported by any modern xCode, and old enough so no apple supported xCode doesn't support it. (you have to go back to xcode 8 or 9 which are out of the map)
So I thought this was likely the most compatible one. However, open to suggestions.
There was a problem hiding this comment.
I think that min version should be iOS 11.
There was a problem hiding this comment.
Cool 11 is good.
Changed.
Upgrading the examples project configurations so they build on latest environments.
As well adding more information to the documentation so it is easier to set it up.