librustzcash
librustzcash copied to clipboard
`zcash_client_sqlite`: Figure out how to build SQLite in a way that is compatible with Rust, or require downstreams to do so
The ECC wallet team opened https://github.com/mozilla/rust-android-gradle/issues/105 because they were having issues using rust-android-gradle with the Android emulator on x86_64 machines. My subsequent investigation (https://github.com/mozilla/rust-android-gradle/issues/105#issuecomment-1481879021 and below) indicates that this is actually an incompatibility between SQLite and Rust:
- SQLite uses
long doubleto preserve as much precision as possible in various places, primarily ASCII-to-float andprintf. This causes it to depend on compiler builtins using 80-bit or 128-bit floats where support is available. - Rust does not support floats larger than
double, so does not implement a bunch of "standard" compiler builtins related to them.
This problem went unnoticed on Android for a long time because:
- In NDK 22 and below,
libgccis linked for unwinding purposes, and it happens to provides these compiler builtin symbols, so there were no linker problems. - In NDK 23 and above,
libgccis absent, but Rust for some reason provides the needed symbols foraarch64-linux-androidtargets.
We currently use SQLite via the rusqlite crate's bundled flag, which automatically compiles and bundles SQLite using whatever C compiler the cc toolchain picks up. Technically we shouldn't do this here; it should be up to the user of zcash_client_sqlite to choose how it links SQLite. In any case, the current setup means that the C compiler assumes these symbols will be available, but Rust doesn't provide them. On desktop I assume we just get the symbols from the C compiler itself, but on Android they came from the NDK which no longer provides them.
We need to figure out how to set up rusqlite to build SQLite in such a way that the necessary symbols are always provided. This may mean removing the bundled feature flag and pushing the problem onto downstream users, that have the necessary context to better solve the issue.
On desktop I assume we just get the symbols from the C compiler itself, but on Android they came from the NDK which no longer provides them.
On a desktop, libgcc (strictly, libgcc_s.so.1) is typically dynamically linked. It is one of the few libraries we dynamically link to in zcashd, despite statically linking most of the dependencies.
We currently use SQLite via the
rusqlitecrate'sbundledflag, which automatically compiles and bundles SQLite using whatever C compiler thecctoolchain picks up. Technically we shouldn't do this here; it should be up to the user of zcash_client_sqlite to choose how it links SQLite.
I agree, we shouldn't do that. The system cc could be broken in various ways, and is not pinned. Even though the SQLite build can't/shouldn't affect consensus compatibility, the app that uses zcash_client_sqlite might want to ensure a deterministic build or otherwise control build options.
On the other hand, the description of the bundled option says:
This is a good option for cases where linking to SQLite is complicated, such as Windows.
The plight of a beleaguered dev who upgrades zcash_client_sqlite and is suddenly faced with having to figure out how to link SQLite on Windows, fills me with sympathy and solidarity — so I propose that we add a non-default bundled feature of zcash_client_sqlite that enables the bundled feature of rusqlite, and document its limitations.
Ah, the cc crate used to build the bundled SQLite will pay attention to CC and CFLAGS environment variables (which can be configured per-platform): https://github.com/rust-lang/cc-rs#external-configuration-via-environment-variables
I verified that cargo clean; CC=clang cargo check works for zcash_client_sqlite, and that cargo clean; CC=this-compiler-does-not-exist cargo check fails as you'd expect.
If we figure out what CFLAGS are needed, this approach could solve both the missing compiler builtins and the nondeterministic build issues, while still delegating to rusqlite / libsqlite3-sys to build the correct version of SQLite for its bindings.
@str4d @daira some new idea or solution for this problem? We just started to have this problem building zingo-mobile for Android...
cc: @zancas @AloeareV
We ended up solving this in the Android SDK with a build script, much the same way as others have: https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/88058c63461f2808efc953af70db726b9f36f9b9/backend-lib/build.rs
Reopening because even with the above fix, we might still want to enable downstreams to control whether SQLite is bundled or not.