-
Notifications
You must be signed in to change notification settings - Fork 4
Bringing bdk-dart binding #2
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
Bringing bdk-dart binding #2
Conversation
…CI, example update, and dylib loader; ignore local libs and test outputs
…e_bindings.sh; regenerate Dart bindings
….rs); generation now uses submodule UDL via local generator
- drop the outdated build.rs that looked for src/bdk.udl - sync Cargo deps with new bdk-ffi (bdk 2.2, uniffi 0.29.4, uniffi-dart main) - regenerate Cargo.lock to match the updated dependency graph
…s is a checkpoint
uniffi-bindgen.rs
Outdated
| match language { | ||
| Some(lang) if lang == "dart" => { | ||
| uniffi_dart::gen::generate_dart_bindings( | ||
| "bdk-ffi/bdk-ffi/src/bdk.udl".into(), | ||
| None, | ||
| Some(output_dir.as_str().into()), | ||
| library_path.as_str().into(), | ||
| true, | ||
| ) | ||
| .expect("Failed to generate dart bindings"); | ||
| } | ||
| _ => uniffi::uniffi_bindgen_main(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the cli usable, the wrapper should only unwrap those flags when they're present and otherwise fall to uniffi::uniffi_bindgen_main().
| match language { | |
| Some(lang) if lang == "dart" => { | |
| uniffi_dart::gen::generate_dart_bindings( | |
| "bdk-ffi/bdk-ffi/src/bdk.udl".into(), | |
| None, | |
| Some(output_dir.as_str().into()), | |
| library_path.as_str().into(), | |
| true, | |
| ) | |
| .expect("Failed to generate dart bindings"); | |
| } | |
| _ => uniffi::uniffi_bindgen_main(), | |
| if let Some(lang) = language { | |
| if lang == "dart" { | |
| match (library_path, output_dir) { | |
| (Some(lib), Some(out)) => { | |
| uniffi_dart::gen::generate_dart_bindings( | |
| "bdk-ffi/bdk-ffi/src/bdk.udl".into(), | |
| None, | |
| Some(out.as_str().into()), | |
| lib.as_str().into(), | |
| true, | |
| ) | |
| .expect("Failed to generate dart bindings"); | |
| return; | |
| } | |
| (Some(_), None) => { | |
| eprintln!("--out-dir is required when using --library; falling back to uniffi CLI"); | |
| } | |
| _ => {} | |
| } | |
| } | |
| } | |
| uniffi::uniffi_bindgen_main(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems more complicated 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason to use this approach? @Johnosezele
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it looks more verbose. The new if let Some(lang) = language { … } branch only calls uniffi_dart::gen::generate_dart_bindings when all flags are present... otherwise it falls back to standard cli, and we log a message if --out-dir is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand the central idea behind the comments now...
The key assumption is centered around whether other users or CI tools expect the uniffi-rs CLI interface on third party implementations
If possible, before the PR is out of draft, check out some third parties to help align expectations for how bdk-dart may be consumed 🤔
.github/workflows/ci.yml
Outdated
| run: dart format --output=none --set-exit-if-changed . | ||
|
|
||
| - name: Analyze | ||
| run: dart analyze --fatal-infos --fatal-warnings || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't || true hide analyzer failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep... it should be removed later... Keeping it in for now till the example has no errors...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanted to check back in on this, does the example still have errors or can we remove || true now?
Awesome work! I regenerated the bindings, ran dart test, and wired them into a macOS Flutter desktop demo and everything loaded correctly. I'll do a deeper look at the pr this week now too. |
|
The unifi-dart fixes have been merged, and we now point to main with the newest rev. We're ready for review and testing. |
uniffi-bindgen.rs
Outdated
| .iter() | ||
| .position(|arg| arg == "--library") | ||
| .and_then(|idx| args.get(idx + 1)) | ||
| .expect("specify the library path with --library"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this parsing happen inside lang == "dart" branch so --help or others dont panic here?
.github/workflows/ci.yml
Outdated
| - name: Analyze | ||
| run: dart analyze --fatal-infos --fatal-warnings || true | ||
|
|
||
| - name: Run tests (expected to fail until bindings fixed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we drop (expected to fail until bindings fixed) now?
|
made some additional comemnts/questions, looks like some of it was discussed already but just wanted to ask again or from a different angle of how I was thinking about it. another broad question that doesnt need to be addressed in this PR necessarily is is there a follow up pr where we support mobile (xcframework, etc), right now I think im only seeing linux/macOS support but let me know if I'm wrong. otherwise I think this pr looks really great, none of my questions are big blockers either, so good stuff and good job by you! |
true! xcframework, android AAR... isn't covered yet. I think it exceeds the scope of this pr. See a follow up issue here |
cool, totally agree doesn't need to be in the scope of this PR I just wanted to mention/ask about it |
fbd7d01 to
da17b49
Compare
This PR brings experimental Dart bindings generation using uniffi-dart. Once the latest fixes from chavic/bdk-fixes (the callback alignment work and the optional-sequence guard) are merged into main without regressions, bdk-dart will be ready for battle testing.
To see, generate, and test out the code run:
Regenerate the bindings using our helper script:
scripts/generate_bindings.shRun the Dart test suite
dart test