This differential introduces changes necessary to compile native rust library on Android
Details
Details
Build android app
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
the prebuilt NDK binaries are in a different location on linux machines. this change should resolve the CI issues.
| native/android/app/CMakeLists.txt | ||
|---|---|---|
| 43–51 ↗ | (On Diff #17653) | Rust_CARGO_TARGET is used by Corrosion |
| 53 ↗ | (On Diff #17653) | the path to the archiver is case sensitive, so we convert the system name to lowercase (e.g. Darwin -> darwin) |
| 54 ↗ | (On Diff #17653) | This is a path so it's difficult to split across multiple lines. There are a bunch of lines in this file that exceed 80 characters so I'm assuming we're not applying that rule as strictly here |
| native/android/app/build.gradle | ||
| 503 ↗ | (On Diff #17653) | Make the NDK Version available in CMake |
Comment Actions
Changes look good to me. Just a quick question - when I was compiling android locally I had to include "native_rust_library/src/lib.rs.h" instead of "lib.rs.h" in C++. this would cause discrepancy between iOS and android. @varun did you try to include lib.rs.h in c++ file with changes from this diff? In the meantime I will try to compile android with this diff locally to see which header needs to be included.
| native/android/app/CMakeLists.txt | ||
|---|---|---|
| 43–51 ↗ | (On Diff #17653) | According to docs yes: https://cmake.org/cmake/help/latest/variable/CMAKE_ANDROID_ARCH_ABI.html. I did a quick check and mips architecture devices are not used as mobile phones. |
Comment Actions
good point, @marcin . the latest update on this diff fixes the discrepancy between iOS and Android
| native/android/app/CMakeLists.txt | ||
|---|---|---|
| 54 ↗ | (On Diff #17666) | we should only "fallback" to using the explicitly passed NDK_VERSION if CMAKE_ANDROID_NDK_VERSION is not available. This seems to be available in the google supplied cmake versions, and in cmake 3.20+ |
| 54 ↗ | (On Diff #17666) | We should check to see if ANDROID_HOME is set, reading blindly from ENV makes me nervous. |
| 76 ↗ | (On Diff #17666) | generally the namespace is for the project or organization |
| native/android/app/CMakeLists.txt | ||
|---|---|---|
| 54 ↗ | (On Diff #17666) | CMAKE_ANDROID_NDK_VERSION is not available. I checked that first |
| 54 ↗ | (On Diff #17666) | ANDROID_HOME is set as part of our dev environment setup. It's also available on the CI machine |
| 76 ↗ | (On Diff #17666) | @marcin is there a reason why we chose android for the namespace here or can i change it to comm? |
| native/android/app/CMakeLists.txt | ||
|---|---|---|
| 76 ↗ | (On Diff #17666) | Changing to comm is absolutely fine. "Some" namespace name was required and android was my first thought , but any will work. |
Comment Actions
Looks fine to me.
Not a big fan of the triple logic, but I'm not sure of a better way to do it