Feat: Android support#158
Conversation
|
This looks nice, can you rebase this? |
55272e4 to
378a3bc
Compare
|
Changelog:
|
a2c959f to
0f3252f
Compare
|
I reduced a lot the code and i think this is the concise version to support android |
|
|
alexanderwiederin
left a comment
There was a problem hiding this comment.
From what I understand the ubuntu boost packages don't ship with the cmake config that core needs when cross-compiling.
Am I right that you got it working on nix locally? If so, can we stick with with only supporting the nix build for android?
Edit: see the comment below |
|
Correction, I changed my mind... The idea is that the |
This commit presents the minimal work needed for this crate to be able to be compiled for android, this is minimal to avoid bloating too much the build process, which would make it hard to review and maintain. The rest of the work will be done on nix side, which provides better ergonomics for such job.
e9e6806 to
479401e
Compare
This commit presents the rest of the work to provide android builds, the packaging and compiling is done trough nix for simplicity
|
Okay the android build being exclusive for nix made things easier for this PR, the changes on build.rs are minimal... I included some inline doc comments so the reviewer can follow why each line were added. I added a section on the readme explaining the android build but its not that explanatory in a level that one can learn how to build this lib to android, do you guys think thats needed ? I can extend the docs to include instructions for one to reproduce it on a downstream |
There was a problem hiding this comment.
Great to see this working.
If we land the hand-written bindings PR (#163) first, the changes in build.rs should become simpler. I would suggest we wait for it.
| assert!( | ||
| Path::new(&toolchain_file).exists(), | ||
| "Android NDK toolchain file not found at {toolchain_file}. \ | ||
| Check that ANDROID_NDK_HOME points to a valid NDK installation" | ||
| ); |
There was a problem hiding this comment.
I think asserts are for faults in logic, not environment variables. I think a !path.exists() { panic!(...) } or .expect() would read more naturally here.
| .arg(format!("-DCMAKE_ANDROID_ARCH_ABI={abi}")) | ||
| .arg(format!("-DCMAKE_SYSTEM_VERSION={api_level}")) |
There was a problem hiding this comment.
Do we really have to pass these in a second time after lines 96 and 97?
| }; | ||
| androidComposition = androidPkgs.androidenv.composeAndroidPackages { | ||
| platformVersions = [ "34" ]; | ||
| ndkVersions = [ "27.2.12479018" ]; |
There was a problem hiding this comment.
27.2.12479018 is duplicated on line 94. I would suggest we make it a variable.
| name = rustVersion; | ||
| sha256 = "sha256-ks0nMEGGXKrHnfv4Fku+vhQ7gx76ruv6Ij4fKZR3l78="; |
There was a problem hiding this comment.
The sha256 here is already defined at the top of the flake as part of rustToolchain. Extract it into a rustSha256 variable alongside rustVersion and reference it in both places. Otherwise a toolchain bump requires updating two identical hashes.
| config.allowUnfree = true; | ||
| }; | ||
| androidComposition = androidPkgs.androidenv.composeAndroidPackages { | ||
| platformVersions = [ "34" ]; |
There was a problem hiding this comment.
Can we add a comment above this?
# platformVersions is the SDK tooling version, not the minimum API level.
# The NDK target floor is set via ANDROID_API_LEVEL in build.rs (default 24).
| android-aarch64 = mkAndroidPackage "aarch64-linux-android"; | ||
| android-armv7 = mkAndroidPackage "armv7-linux-androideabi"; | ||
| android-x86_64 = mkAndroidPackage "x86_64-linux-android"; |
There was a problem hiding this comment.
in build.rs, the android_abi() function handles four targets, but here only three are exposed. Is that intentional?
| name: Cross-Compile for Android (${{ matrix.package }}) | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: false |
| Android cross-compilation requires [Nix](https://nixos.org/). | ||
|
|
||
| Nix package outputs bundle the exact NDK version, Rust toolchains with Android | ||
| targets, Boost, and cmake in a single reproducible build drying out the support |
There was a problem hiding this comment.
what do you mean "drying" out the support?
| cargo b | ||
| ``` | ||
|
|
||
| ### Android Cross-Compilation |
There was a problem hiding this comment.
Worth adding a note that the output targets Android API 24+ (Nougat) minimum, so people know the minimum Android version their app must support.
This pr contain 3 commits:
This is just a PoC, the main commit is the third one and the CI job containing the android building at that point should run fine.
For more context read #156