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

Port to Ffigen #25

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Port to Ffigen #25

wants to merge 8 commits into from

Conversation

JoschuaL
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

arctic-alpaca commented Mar 24, 2021

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

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

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
Copy link
Contributor Author

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

@arctic-alpaca
Copy link
Contributor

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.

script = [
"""
cd ${CARGO_MAKE_WORKSPACE_WORKING_DIRECTORY}/packages/${CARGO_MAKE_CRATE_FS_NAME}
dart run ffigen
Copy link
Owner

Choose a reason for hiding this comment

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

maybe this should be flutter pub run ffigen?

script = [
"""
cd ${CARGO_MAKE_WORKSPACE_WORKING_DIRECTORY}/packages/${CARGO_MAKE_CRATE_FS_NAME}
dart run ffigen
Copy link
Owner

Choose a reason for hiding this comment

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

same, try change it to

flutter pub run ffigen

@JoschuaL
Copy link
Contributor Author

JoschuaL commented Mar 25, 2021

@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
Copy link
Contributor

arctic-alpaca commented Mar 25, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

- '/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 #25 (comment)

@shekohex shekohex mentioned this pull request Jan 10, 2022
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.

3 participants