Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial FFI #34

Closed
wants to merge 6 commits into from
Closed

Initial FFI #34

wants to merge 6 commits into from

Conversation

clehner
Copy link
Contributor

@clehner clehner commented Oct 5, 2020

This PR adds an initial FFI (#10) for C, Java, Android, and Flutter.

Structure

C shared library

Cargo builds the library as a shared/dynamic library ("cdylib" crate type), alongside the normal Rust library type ("rlib"). On Linux or BSD this produces a file "libssi.so"; on Mac/Darwin it will be "libssi.dylib", and on Windows it would be "libssi.dll". For this shared library, we export functions and/or data structures, and use cbindgen to generate a corresponding C header file. For testing, we have a simple C program load the shared library and call a method on it.

Java/JNI

For Java, we use Java's traditional standard FFI system, JNI. We export C functions like for C, but using JNI's special method signatures and types. The jni crate makes this easier. We specify Java classes and methods in Java for the types exported from the library, and load their implementations from the shared library. We build class files from the Java source files, and bundle those into a JAR. For testing, we have included a main function in the Java source and run that from the JAR file, with the shared library.

Android AAR

For Android, we build a AAR (Android Archive). AAR is like JAR but including the shared libraries. The AAR file includes our class files, along with the ssi shared library built for Android's four supported architectures. To cross-compile the library, we use Cargo's cross-compilation toolchains from rustup, but we also need the Android SDK & NDK for some build tools.

Flutter plugin

The Flutter plugin is a Dart/Flutter package with native platform integration. The included plugin is largely based on a template, and supports Android and iOS. I have only implemented/tested the Android part. For Android, the plugin reuses the cross-compiled shared libraries from the AAR, but it uses the C interface rather than the JNI one, as it is loaded by Dart rather than Java. We have some Dart code defining types and functions that are loaded from the shared library. This plugin adds a lot of files; I looked into removing the example directory but it seems that Flutter needs it somehow during build. There is also a test file in Dart for the Flutter plugin; this uses the shared library built for the host system.

Usage

The Makefile allows for building and testing the FFI artifacts.
For example, you can use make target/ssi.aar to build the Android Archive (AAR file).

The top-level make target is called "test". It defers to targets that build the FFI artifacts and perform simple tests as described previously. If you run make with no argument, it runs that target and tries to build and test all the FFI artifacts (not the Rust library itself, we still use cargo build and cargo test for that - although maybe that should be included in the Makefile as well). If a test passes, it is not re-run unless the dependent files are updated.

The CI workflow is updated to build and test the FFI artifacts. It also installs dependencies, like the Android SDK/NDK, Flutter, and JDK. I added some caching actions, to speed up CI runs, since Android NDK is a very large download, and cross-compiling all the Android targets from scratch takes a while. A full run takes about 20 minutes; with caching it is about 10 minutes.

I also added some markdown files for the new FFI directories. I added a FFI.md which links to them all. When we have a readme we could consider merging that into it.

Limitations and next considerations

This does not export any of the actual library API, only a function to return the library version, so you can check that the FFI is working. So this PR is more about the build system and setup. A more complete FFI will involve either exporting all the library's functions and data structures, or designing an API specifically for application use. Also it will need to be documented. I have placed the initial FFI implementation in src/lib.rs, but when they are more fully implemented, it might make more sense to put them in separate files or separate directories.

Building on macOS or Windows is not yet fully supported. Some code assumes the shared library ends in ".so", but on Mac and Windows it will be ".dylib" and ".dll", respectively. The iOS component of the Flutter plugin is untested but should be straightforward to get going.

I started adding a WASM target using wasm-pack, but have not got that fully working yet so have not included it in this PR.

The Makefile uses some GNU Make features; later I would like to consider making it more portable.

@clehner clehner requested a review from sbihel October 5, 2020 22:54
Copy link
Member

@sbihel sbihel left a comment

Choose a reason for hiding this comment

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

I'm really not familiar with this stuff. It's a bit more messy than I was expecting (e.g. somehow I thought bindings were just magically happening with an attribute). But I'm sure we'll find a way to decouple things (like macros to expose the API).

Maybe we could use Docker for the CI, and also to make the building more portable.

Anyway, it looks more than good enough!

## Flutter

target/test/flutter.stamp: flutter/lib/ssi.dart target/release/libssi.so | target/test
cd flutter && LD_LIBRARY_PATH=$(PWD)/flutter \
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why but if you fix the LD_LIBRARY_PATH by removing the redundant /flutter, you can remove the example dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @sbihel. I'm unable to reproduce what you are suggesting here. Can you provide an excerpt of make building that target with that change, and/or without the example dir?

Copy link
Member

@sbihel sbihel Oct 7, 2020

Choose a reason for hiding this comment

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

Now it complains that

embedding. It's being deprecated in favor of Android embedding v2. Follow the
steps at

https://flutter.dev/go/android-project-migration

to migrate your project.

It's fine for now to keep the example directory, I'll look into removing it later, it doesn't really matter.

@clehner clehner mentioned this pull request Oct 8, 2020
@clehner
Copy link
Contributor Author

clehner commented Oct 8, 2020

This functionality will go in didkit instead: spruceid/didkit#1. I am closing this PR now but will leave the branch present in this repo for reference until the functionality is implemented in didkit.

@clehner clehner closed this Oct 8, 2020
@clehner
Copy link
Contributor Author

clehner commented Oct 10, 2020

I merged the first three commits (up to 9dd0d8c) as they should be useful in general.

clehner added a commit to spruceid/didkit that referenced this pull request Oct 16, 2020
@clehner clehner deleted the ffi branch October 20, 2020 20:51
@clehner
Copy link
Contributor Author

clehner commented Oct 20, 2020

Branch deleted, as functionality now merged into DIDKit in spruceid/didkit#2

@clehner
Copy link
Contributor Author

clehner commented Dec 2, 2020

For reference, here is the WIP I had for WASM on this branch:

From e84226ae5e108c2d427e8d5ab85b9f38115fb3b7 Mon Sep 17 00:00:00 2001
From: "Charles E. Lehner" <charles.lehner@spruceid.com>
Date: Mon, 5 Oct 2020 16:55:00 -0400
Subject: [PATCH] WIP: add WASM target

---
 Cargo.toml |  8 ++++++++
 Makefile   | 16 ++++++++++++++++
 src/lib.rs | 18 ++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/Cargo.toml b/Cargo.toml
index 49cb93b..c644634 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -18,6 +18,14 @@ jsonwebtoken = "7"
 ring = "0.16"
 jni = "0.17"
 
+[target.'cfg(target_arch = "wasm32")'.dependencies]
+wasm-bindgen = "0.2"
+js-sys = "0.3"
+wasm-bindgen-test = "0.3"
+
+#[target.'cfg(target_arch = "wasm32")'.dev-dependencies]
+#[dev-dependencies]
+
 [workspace]
 members = [
   "did-resolve",
diff --git a/Makefile b/Makefile
index eb477e1..0c38de9 100644
--- a/Makefile
+++ b/Makefile
@@ -2,6 +2,7 @@
 
 .PHONY: test
 test: target/test/c.stamp \
+	target/test/wasm-node.stamp \
 	target/test/java.stamp \
 	target/test/aar.stamp \
 	target/test/flutter.stamp
@@ -32,6 +33,21 @@ target/test/c.stamp: target/cabi-test target/release/libssi.so | target/test
 target/cabi-test: c/test.c target/ssi.h
 	$(CC) -Itarget $< -ldl -o $@
 
+## WASM
+
+.PHONY: install-wasm
+install-wasm:
+	rustup target add wasm32-unknown-unknown
+	cargo install wasm-pack
+
+target/wasm-node: $(RUST_SRC)
+	wasm-pack build -t nodejs -d $@
+	touch $@
+
+target/test/wasm-node.stamp: target/wasm-node
+	wasm-pack test --node
+	touch $@
+
 ## Java
 
 JAVA_SRC=$(wildcard java/*/*.java java/*/*/*.java java/*/*/*/*.java)
diff --git a/src/lib.rs b/src/lib.rs
index e680a90..753c38e 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -37,6 +37,24 @@ pub extern "system" fn Java_com_spruceid_ssi_getVersion(env: JNIEnv, _class: JCl
         .into_inner()
 }
 
+#[cfg(target_arch = "wasm32")]
+mod wasm {
+    use super::*;
+    use js_sys::JsString;
+    use wasm_bindgen::prelude::*;
+    use wasm_bindgen_test::*;
+
+    #[wasm_bindgen]
+    pub fn wasm_ssi_get_version() -> JsString {
+        VERSION.into()
+    }
+
+    #[wasm_bindgen_test]
+    fn test_wasm_ssi_get_version() {
+        assert!(wasm_ssi_get_version().length() > 0);
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
-- 
2.29.2

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.

None yet

2 participants