⚙ D5188 CMakeLists.txt changes to compile native rust library on android

marcin created this revision.Sep 20 2022, 9:43 AM

Herald added subscribers: • abosh, atul, ashoat. · View Herald TranscriptSep 20 2022, 9:43 AM

Update message, remove blob client from include directories

marcin added a child revision: D5176: Implement wrapper around blob client C++ bindings for blob upload process.Sep 20 2022, 9:51 AM

Harbormaster failed remote builds in B12300: Diff 16887!Sep 20 2022, 9:53 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 20 2022, 9:57 AM

Harbormaster failed remote builds in B12301: Diff 16888!

Install rust target platforms

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 20 2022, 1:19 PM

Harbormaster failed remote builds in B12315: Diff 16903!

Fix linting process

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 20 2022, 1:46 PM

Harbormaster failed remote builds in B12317: Diff 16908!

fix invalid dependency

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 20 2022, 1:58 PM

Harbormaster failed remote builds in B12318: Diff 16909!

varun commandeered this revision.Oct 13 2022, 6:11 PM

varun edited reviewers, added: marcin; removed: varun.

add env var to CMakeLists.txt to help cargo locate the correct archiver

built the android app successfully in nix and normal environment

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 13 2022, 6:24 PM

Harbormaster failed remote builds in B12777: Diff 17549!

the prebuilt NDK binaries are in a different location on linux machines. this change should resolve the CI issues.

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 13 2022, 9:05 PM

Harbormaster failed remote builds in B12784: Diff 17557!

add some logs

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 13 2022, 10:09 PM

Harbormaster failed remote builds in B12788: Diff 17562!

hopefully last changes

Herald added a reviewer: • jon. · View Herald TranscriptOct 18 2022, 8:54 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 18 2022, 9:05 PM

Harbormaster failed remote builds in B12857: Diff 17647!

fix case sensitivity issue on CI machine

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 18 2022, 9:31 PM

Harbormaster failed remote builds in B12859: Diff 17649!

Harbormaster completed remote builds in B12859: Diff 17649.Oct 19 2022, 1:21 AM

varun requested review of this revision.Oct 19 2022, 1:21 AM

clean up a little now that everything works

Harbormaster failed remote builds in B12861: Diff 17652!Oct 19 2022, 1:38 AM

more cleanup

varun added inline comments.Oct 19 2022, 2:07 AM

native/android/app/CMakeLists.txt
43–51	`Rust_CARGO_TARGET` is used by Corrosion
53	the path to the archiver is case sensitive, so we convert the system name to lowercase (e.g. Darwin -> darwin)
54	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	Make the NDK Version available in CMake

Harbormaster completed remote builds in B12862: Diff 17653.Oct 19 2022, 2:16 AM

varun added 1 blocking reviewer(s): tomek.Oct 19 2022, 2:27 AM

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.

This revision now requires changes to proceed.Oct 19 2022, 10:09 AM

Looks ok, but I'm not really sure how to review it. @marcin and @jon should do a better review.

native/android/app/CMakeLists.txt
43–51	Are we sure this contains all the possible targets?

marcin added inline comments.Oct 19 2022, 11:45 AM

native/android/app/CMakeLists.txt
43–51	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.

include the native_rust_library directory in CMakeLists.txt

In D5188#159469, @marcin wrote:

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.

good point, @marcin . the latest update on this diff fixes the discrepancy between iOS and Android

Harbormaster completed remote builds in B12875: Diff 17666.Oct 19 2022, 8:01 PM

• jon requested changes to this revision.Oct 20 2022, 12:25 AM

• jon added inline comments.

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

This revision now requires changes to proceed.Oct 20 2022, 12:25 AM

varun requested review of this revision.Oct 20 2022, 2:15 PM

varun added inline comments.

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?