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

[frb] : Add support for Dart/Flutter bindings - Part II #121

Merged
merged 28 commits into from
May 3, 2024

Conversation

erdemyerebasmaz
Copy link
Contributor

@erdemyerebasmaz erdemyerebasmaz commented Apr 29, 2024

Closes #9

This PR adds Dart/Flutter packages their build tools & generates Dart bindings.

There's a lot of boilerplate code, to ease review process I'd suggest you to go over these scripts:

ls-sdk-flutter/
-packages/flutter_breez_liquid/ffigen.yaml
-packages/flutter_breez_liquid/android/CMakeLists.txt
-packages/flutter_breez_liquid/windows/CMakeLists.txt
-packages/flutter_breez_liquid/linux/CMakeLists.txt
-scripts/**
-flutter_rust_bridge.yml
-justfile
-melos.yaml

PR TODO:

  • Explain the usages of
    • just & justfile scripts
    • Melos & it's build scripts
    • CMake scripts used for Android & Windows/Linux targets

Other changelist:

  • Add min profile for ls-sdk-flutter scripts 6691553
  • Add suitable crate-types are configured for target devices 66f6886
  • [ls-sdk-bindings] Use LsSdkError from ls-sdk::error 57d1d0c

TODO:

  • Apply rename changes
  • Only compile ls-sdk-core with build scripts

@erdemyerebasmaz erdemyerebasmaz added this to the v0.1.0 milestone Apr 29, 2024
@erdemyerebasmaz erdemyerebasmaz changed the title [Draft] Add Dart/Flutter packages & generate bindings [Draft] [frb] : Add support for Dart/Flutter bindings - Part II Apr 29, 2024
lib/Cargo.toml Outdated Show resolved Hide resolved
@erdemyerebasmaz erdemyerebasmaz force-pushed the dart_packages branch 2 times, most recently from 454ffe5 to db2e984 Compare April 30, 2024 15:18
@erdemyerebasmaz erdemyerebasmaz changed the title [Draft] [frb] : Add support for Dart/Flutter bindings - Part II [frb] : Add support for Dart/Flutter bindings - Part II May 2, 2024
@erdemyerebasmaz erdemyerebasmaz requested review from dangeross and ok300 May 3, 2024 10:40
@erdemyerebasmaz
Copy link
Contributor Author

@ok300 @dangeross Rebased & applied all the renaming/folder structure changes then generated bindings. Please take another look.

Ross has suggested moving flutter out of libs under packages and I agree with the notion but it'll have to wait because Flutter directory consists of build tools & packages.

If we move the whole directory under packages, we'll have Dart & Flutter package live under:

/packages/flutter/packages/breez_liquid
/packages/flutter/packages/flutter_breez_liquid

which I find non-ideal.

If making a folder & package name distinction is compatible with build scripts, I'll move build scripts elsewhere and generate the libraries under:

/packages/dart/ # for breez_liquid, the Dart only package
/packages/flutter/ # for flutter_breez_liquid, the Flutter wrapper around Dart package

on a separate PR.

Copy link
Contributor

@ok300 ok300 left a comment

Choose a reason for hiding this comment

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

Looks good, just a few NITs about the project renaming

  • a few instances of ls_sdk.* files
  • a few instances of breez-sdk-liquid instead of breez-liquid-sdk

Might be worth to do a full search-replace to make sure all occurrences are covered.

lib/flutter/breez_liquid_sdk/include/ls_sdk.h Outdated Show resolved Hide resolved
lib/flutter/packages/breez_liquid/pubspec.yaml Outdated Show resolved Hide resolved
lib/flutter/packages/flutter_breez_liquid/CHANGELOG.md Outdated Show resolved Hide resolved
lib/flutter/scripts/pubspec.yaml Outdated Show resolved Hide resolved
@ok300
Copy link
Contributor

ok300 commented May 3, 2024

I agree

/packages/dart/    # for breez_liquid, the Dart only package
/packages/flutter/ # for flutter_breez_liquid, the Flutter wrapper around Dart package

looks cleaner, if it's possible to achieve in a separate PR.

@erdemyerebasmaz erdemyerebasmaz requested a review from ok300 May 3, 2024 12:15
@erdemyerebasmaz
Copy link
Contributor Author

Looks good, just a few NITs about the project renaming

  • a few instances of ls_sdk.* files
  • a few instances of breez-sdk-liquid instead of breez-liquid-sdk

Might be worth to do a full search-replace to make sure all occurrences are covered.

Thanks, I believe everything is covered now.

I agree

/packages/dart/    # for breez_liquid, the Dart only package
/packages/flutter/ # for flutter_breez_liquid, the Flutter wrapper around Dart package

looks cleaner, if it's possible to achieve in a separate PR.

Yes, I plan to handle it on a separate PR.

Current folder structure is made compatible with changes in #130

@erdemyerebasmaz erdemyerebasmaz merged commit 1501362 into main May 3, 2024
@ok300 ok300 mentioned this pull request May 6, 2024
7 tasks
@erdemyerebasmaz erdemyerebasmaz deleted the dart_packages branch June 21, 2024 13:57
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.

Build system and binding generation
3 participants