Skip to content

Add Android packaging: xtask build-android-* emit the C FFI as an NDK cdylib - #17

Open
meinharrd wants to merge 3 commits into
mainfrom
android-ffi-export
Open

Add Android packaging: xtask build-android-* emit the C FFI as an NDK cdylib#17
meinharrd wants to merge 3 commits into
mainfrom
android-ffi-export

Conversation

@meinharrd

Copy link
Copy Markdown

Implements #16 — the Android export freedom-browser-android needs to swap its embedded Kubo node for freedom-ipfs (solardev-xyz/freedom-browser-android#3).

What

  • cargo xtask build-android-arm64 / build-android-x86_64 / build-android-all cross-compile freedom-ipfs-mobile with cargo-ndk at API 26, producing target/<triple>/release/libfreedom_ipfs_mobile.so per ABI. Matching make build-android-* targets and a README packaging section.
  • The crate type is overridden per-build with cargo rustc --crate-type cdylib (ant's pattern), so Cargo.toml keeps rlib + staticlib for the iOS/desktop slices and only the .so is emitted for Android.
  • Linked explicitly with -Wl,-z,max-page-size=16384 so the artifact loads on 16 KB-page Android 15+ devices regardless of cargo-ndk/NDK defaults.

DNS audit (Android has no /etc/resolv.conf)

No library changes needed — every production resolution path already avoids system DNS config:

  • DNSLink/IPNS: Cloudflare DoH over HTTPS
  • libp2p dial paths (light-DHT, Bitswap TCP/WS): ResolverConfig::cloudflare() pinned explicitly, never from_system_conf
  • delegated routing / HTTP providers (reqwest): getaddrinfo, which bionic backs

The resolv-conf crate remains in the lockfile only as an unexercised transitive dep of hickory-resolver.

Verification

Built on Linux x86_64 with NDK r27 + cargo-ndk 4.1.2, both ABIs:

  • 26 MB .so per ABI
  • all 45 freedom_ipfs_* symbols from ffi/include/freedom_ipfs.h exported (llvm-nm -D --defined-only)
  • all LOAD segments aligned to 0x4000 (llvm-readelf -l)

The JNI shim intentionally stays in the consumer (freedom-browser-android), mirroring the ant split: this repo owns the C ABI + header, the app owns *_jni.c.

… cdylib

freedom-browser-android consumes the same C ABI as iOS
(ffi/include/freedom_ipfs.h) but as a shared library loaded via a JNI
shim in the app, so packaging is a per-ABI libfreedom_ipfs_mobile.so
instead of the xcframework.

- xtask build-android-arm64 / build-android-x86_64 / build-android-all
  cross-compile freedom-ipfs-mobile with cargo-ndk (API 26). The
  crate-type is overridden to cdylib per-build via cargo rustc, so
  Cargo.toml keeps rlib+staticlib for the iOS and desktop slices, and
  the .so is linked with max-page-size=16384 for 16 KB-page Android
  15+ devices regardless of cargo-ndk/NDK defaults.
- Matching make build-android-* targets and a README packaging section.

No library-code changes were needed: all production name-resolution
paths already avoid /etc/resolv.conf (absent on Android) — DNSLink/
IPNS use Cloudflare DoH, the libp2p DNS transports pin
ResolverConfig::cloudflare() instead of system config, and plain HTTP
resolves via getaddrinfo.

Closes #16
@meinharrd

Copy link
Copy Markdown
Author

Review notes — no correctness bugs found (the cargo-ndk invocation shape, the --crate-type cdylib override, and the ensure_cargo_ndk error paths all verified out, including an empirical parse test against cargo-ndk 4.1.2). Items to fix, most significant first:

  1. Use the existing run() helper in build_android (xtask/src/main.rs). The new rustup and cargo-ndk invocations hand-roll the spawn → status.success()bail! pattern that run(&mut Command, label) (around line 1492) already encapsulates and that the rest of the file uses (lipo, codesign, etc.). Both blocks are drop-in conversions:

    run(Command::new("rustup").args(["target", "add", target]),
        &format!("rustup target add {target}"))?;

    (build_xcframework's rustup/cargo section has the same hand-rolled pattern — worth converting it in the same sweep.)

  2. Move the 16 KB page-size flag to .cargo/config.toml instead of (or in addition to) the per-invocation -C link-arg:

    [target.aarch64-linux-android]
    rustflags = ["-C", "link-arg=-Wl,-z,max-page-size=16384"]
    [target.x86_64-linux-android]
    rustflags = ["-C", "link-arg=-Wl,-z,max-page-size=16384"]

    Today xtask is the only Android build path, but any future path that bypasses it (a CI job calling cargo-ndk directly, a dev's ad-hoc build) would silently emit a .so that fails to load on 16 KB-page Android 15+ devices. cargo-ndk v3+ sets the linker via CARGO_TARGET_<T>_LINKER, not RUSTFLAGS, so config-level target rustflags apply cleanly.

  3. Drop the redundant #[command(name = "build-android-arm64")] — clap's default kebab-case rename already produces exactly that name (matching BuildXcframework etc. in the same enum). Only the x86_64 variant needs its attribute (default would be build-android-x8664). Keep it only if you prefer the visual symmetry.

Minor, non-blocking: the artifact check hardcodes target/<triple>/release/…, so a set CARGO_TARGET_DIR or non-root cwd makes a successful build report "missing" — but that matches the file-wide convention (staticlib() does the same), so only worth touching in a file-wide fix. Also noting there's no CI coverage for the Android build, so breakage surfaces only on local builds.

…lap attr

- build_android and build_xcframework's rustup/cargo invocations now go
  through the existing run() helper instead of hand-rolling
  spawn/status/bail.
- The 16 KB max-page-size link flag moves from a per-invocation rustc
  arg to target-scoped rustflags in .cargo/config.toml, so build paths
  that bypass xtask (direct cargo-ndk, CI) also emit Android 15+
  compliant artifacts. cargo-ndk sets the linker via
  CARGO_TARGET_<T>_LINKER, so the two compose.
- Drop the redundant #[command(name)] on BuildAndroidArm64 — clap's
  kebab-case default already produces build-android-arm64; only the
  x86_64 variant needs the explicit name.

Re-verified after the rustflags move: both ABIs rebuild with all 45
freedom_ipfs_* symbols and 0x4000 LOAD alignment.
@meinharrd

Copy link
Copy Markdown
Author

All three items addressed in 34df282:

  1. build_android uses the run() helper, and build_xcframework's rustup/cargo blocks were converted in the same sweep.
  2. The 16 KB page-size flag moved to target-scoped rustflags in .cargo/config.toml (removed from the xtask invocation), so direct cargo-ndk/CI builds are covered too.
  3. Redundant #[command(name)] dropped from BuildAndroidArm64; kept only on the x86_64 variant where the default rename wouldn't match.

Since moving rustflags invalidates the build cache, both ABIs were rebuilt from scratch to re-verify: 45 freedom_ipfs_* symbols exported, all LOAD segments at 0x4000. The CI-coverage note is fair — happy to add a cross-compile job in a follow-up if wanted.

@meinharrd

Copy link
Copy Markdown
Author

Review

Overview

Adds build-android-arm64 / build-android-x86_64 / build-android-all xtask commands (plus Makefile targets and README docs) that cross-compile freedom-ipfs-mobile with cargo-ndk at API level 26, emitting a cdylib .so per ABI without touching the crate's rlib + staticlib declaration used by iOS/desktop. A new .cargo/config.toml pins -Wl,-z,max-page-size=16384 for both Android targets, and build_xcframework is refactored to reuse the existing run helper.

Correctness

  • cargo rustc --crate-type cdylib approach is sound — it overrides the lib target's crate types for this build only, and #[no_mangle] symbols from dependency rlibs are exported into the cdylib's dynamic symbol table. The PR verified all 45 freedom_ipfs_* symbols with llvm-nm.
  • #[command(name = "build-android-x86_64")] is necessary and correct — clap's default kebab-casing of BuildAndroidX8664 would produce build-android-x8664.
  • The build_xcframework refactor is behavior-preserving, including the IPHONEOS_DEPLOYMENT_TARGET env var. Nice cleanup.
  • Relative target/... path check assumes the xtask runs from the workspace root; if invoked from a subdirectory, cargo would still build (workspace discovery) but the lib.exists() check would spuriously fail. build_xcframework already makes the same assumption (PathBuf::from("target/ios-xcframework")), so this is consistent with existing convention, not a new defect.

Main risk: the page-size flag can be silently dropped

Target-scoped rustflags in .cargo/config.toml are ignored entirely whenever RUSTFLAGS or CARGO_ENCODED_RUSTFLAGS is set in the environment — cargo doesn't merge them, env wins. The config comment says the flags "compose cleanly" with cargo-ndk's linker env var, which is true, but a CI job or developer shell exporting something as common as RUSTFLAGS="-D warnings" would drop the 16 KB alignment with no error, producing a .so that fails to load on Android 15+ 16 KB-page devices — exactly the failure mode this PR exists to prevent, and one that only surfaces at app runtime on specific hardware.

Suggestion: have build_android verify the artifact after building — e.g. parse llvm-readelf -l (or read program headers directly) and bail! if any LOAD segment alignment is below 0x4000. That turns a silent runtime failure on user devices into a build error, and also guards against future NDK/cargo-ndk behavior changes. (Cheaper alternative: soften the config comment and note the RUSTFLAGS caveat in the README.)

Test coverage / CI

  • The repo has ios-xcframework.yml and electron-addon.yml workflows, but this PR adds no Android CI, so the build can rot unnoticed. GitHub's ubuntu-latest runners ship an NDK, so a workflow that installs cargo-ndk, builds both ABIs, and asserts the symbol list against ffi/include/freedom_ipfs.h plus the 0x4000 alignment (i.e., automating the PR body's manual verification) would be a natural follow-up — fine as a separate PR, but worth tracking in Android support: export the C FFI as an NDK cdylib (libfreedom_ipfs_mobile.so) #16.

Minor points

  • ensure_cargo_ndk requiring explicit installation rather than auto-running cargo install is a good call and well-commented. One nit: the error prints stderr before stdout with no separator, which can read oddly; cosmetic only.
  • rustup target add will fail for non-rustup toolchains, but the xcframework path has the same behavior — consistent.
  • API level 26 (-P 26) vs "NDK r26+" in the README are two different "26"s; the code comment clarifies the API level tracks minSdk, but a reader skimming the README could conflate them. Consider "NDK r26 or newer" wording to disambiguate.
  • Docs, Makefile, and README are consistent with each other and with the printed artifact path.

Security

  • No concerns. No network fetches or auto-installs are introduced (deliberately, per the comment), the DNS audit in the PR body is a description of existing behavior rather than a code change, and the new build flags only affect ELF layout.

Verdict

Solid, well-scoped PR that follows the repo's xtask/Makefile/README conventions, with unusually thorough verification notes. The one change I'd request before merge is the post-build 16 KB-alignment check (or at minimum documenting the RUSTFLAGS override caveat), since the current setup can silently lose the flag that is the PR's core guarantee. Android CI can follow separately.

RUSTFLAGS / CARGO_ENCODED_RUSTFLAGS in the environment silently
replace the target rustflags in .cargo/config.toml, which would drop
the max-page-size link-arg and produce a .so that only fails at app
runtime on 16 KB-page Android 15+ hardware. build_android now parses
the ELF program headers of the built artifact (no external tool
needed) and fails the build when any LOAD segment aligns below
0x4000; unit tests cover the parser and both verdicts on synthetic
ELFs. The config.toml comment and README now state the env-override
caveat instead of claiming clean composition, plus the review's
cosmetic nits: stderr/stdout separator in the cargo-ndk error and
'NDK release r26 or newer' wording to disambiguate from API level
26.
@meinharrd

Copy link
Copy Markdown
Author

Requested change addressed in 3dd26f8build_android now verifies the built artifact's LOAD alignment by parsing the ELF program headers directly (no llvm-readelf dependency) and fails the build if any segment aligns below 0x4000, with the error pointing at the RUSTFLAGS-override cause. Unit tests cover the parser and both verdicts on synthetic ELFs (8 xtask tests green); both ABIs rebuilt through the new check successfully.

Also in the same commit: the .cargo/config.toml comment now states the env-replaces-config caveat instead of claiming clean composition, README says "NDK release r26 or newer" to disambiguate from API level 26 and documents the verification, and the cargo-ndk error output got stderr/stdout separators.

For the CI follow-up I'll note it on #16 so it isn't lost.

@flotob flotob left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One correctness issue remains in the Android packaging path. Otherwise the latest head looks sound, including the 16 KB ELF alignment verification.

Validation performed:

  • cargo test --workspace: passed
  • cargo test -p xtask: 8 passed
  • cargo fmt --all -- --check: passed
  • cargo clippy -p xtask --all-targets -- -D warnings: passed
  • GitHub iOS XCFramework workflow: passed

I could not independently rerun the Android cross-build because cargo-ndk/NDK is not installed locally.

Comment thread xtask/src/main.rs
&format!("cargo ndk -t {target}"),
)?;

let lib = PathBuf::from("target")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Respect Cargo's configured target directory

This hardcodes target/<triple>/release/..., although Cargo may write the artifact beneath CARGO_TARGET_DIR or [build] target-dir. It also fails when xtask is launched from a workspace subdirectory: Cargo finds the workspace and builds successfully, but this relative lookup checks the caller's directory and then incorrectly reports the .so missing. Please resolve Cargo's target_directory (for example through cargo metadata) or pass an explicit absolute --target-dir to cargo-ndk.

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.

2 participants