search

shekohex / flutterust

Flutter + Rust = :heart:
Apache License 2.0
699 stars 56 forks source link

Port to Ffigen #25

Open JoschuaL opened 3 years ago

JoschuaL commented 3 years ago

With this the port from dart-bindgen to ffigen should be done.

Only thing i see as slightly problematic is the fact that in the generated bindings, store_dart_post_cobject takes a int.

This shouldn't be a big problem, as any safety guarantees on darts side are out of the window when calling rust anyways, so weather its passing a Pointer or just the address shouldn't matter (to my knowledge, darts pointer class is just a shim over a int anyways)

the second thing would be the fact that ffigen exposes char* as Pointer, where as flutter itself uses Pointer for string related operations. Again, casting them should not be much of a problem, since safety guarantees don't exist in the first place, but its additional friction when using the generated bindings. There seems to be ongoing discussions about this though, and the story will probably improve in the future as dart ffi matures.

JoschuaL commented 3 years ago

the current problem seems to be that the ci cant properly run ffigen and the generated files are not present once the flutter build step runs, the generated bindings aren't present. I'm not sure how to fix this.

arctic-alpaca commented 3 years ago

I'm pretty sure the ffigen task isn't run by cargo make, at least it doesn't appear in the CI logs. This command only runs task android https://github.com/shekohex/flutterust/blob/d7243f2cea5908d9a859d26a3ff65e7e72b69b9f/.github/workflows/ci.yml#L93-L99 I think you would want 

name: Run cargo make ffigen 
   if: matrix.os == 'ubuntu-latest' 
   uses: actions-rs/cargo@v1 
   continue-on-error: false 
   with: 
     command: make 
     args: build

or something similar ( args: ffigen). Same for ios.

JoschuaL commented 3 years ago

ah, i thought the field in the name would be the command that is run. Never mind then.

arctic-alpaca commented 3 years ago

I got the Android part of the CI running with this commit (this is based on your changes here). Flutter SDK wasn't in cargo make's environment/path and the llvm path needed to be set with regard to the CI environment.

I'm not sure what' happening in the iOS part of the CI and I don't have access to a mac to debug this locally. Check out this run to see what's happening.

JoschuaL commented 3 years ago

@arctic-alpaca the IOS build seems to fail because of some defines in it (or probably rather in one of the headers) clashes with a definition in the build-in headers of the macos/ios c++ distribution. Thats probably because apple can't help themselves but deviate from upstream clang whenever they get the chance, so they probably define some stuff they need without a #pragma once.

Maybe I can fix that by tweaking the cbindgen settings a bit. But goof to see that it can be made to work in generall.

PS: Can i somehow pull in code from other forks into this PR? Or do I have to go in and copy paste the commit?

arctic-alpaca commented 3 years ago

PS: Can i somehow pull in code from other forks into this PR? Or do I have to go in and copy paste the commit?

I don't think pulling the code is possible, copy paste shouldn't be an issue since my changes are based on your last commit here. Copying over the changed files should suffice.

Regarding the iOS issue, I think this might be related to allo-isolate since it also can't find DartCObject but I'm not sure where to go from there.

JoschuaL commented 3 years ago

So the new Problem is that ffigen can't find the llvm libraries on the build server. Again, the fix is probably easy, but I have no idea where the libraries would be aside from the places ffigen already searches.

arctic-alpaca commented 3 years ago

- '/home/runner/work/_temp/llvm/10.0/' added to packages/adder_ffi/pubspec.yaml and packages/scrap_ffi/pubspec.yaml worked in my case, as mentioned in https://github.com/shekohex/flutterust/pull/25#issuecomment-806491213