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

Add binary to remove the need for uniffi-bindgen cli tool #122

Conversation

thunderbiscuit
Copy link
Collaborator

@thunderbiscuit thunderbiscuit commented Mar 24, 2022

WIP; I'm not as confident in my Rust as I am in my Kotlin. Feel free to tear apart my implementation or let me know where to improve.

This adds a binary called generate-bindings, which intends to be a drop-in replacement for the uniffi-bindgen tool from the perspective of the language bindings repos bdk-kotlin and bdk-swift.

In short, as part of the new Gradle plugin proposed in bitcoindevkit/bdk-kotlin#32 or simply in the build.sh file, we should be able to replace the line

uniffi-bindgen generate src/bdk.udl --no-format --out-dir ../jvm/src/main/kotlin --language kotlin

by the line

cargo run --bin generate-bindings -- --language kotlin --out-dir ../jvm/src/main/kotlin

This will ensure we're always using the same version of uniffi-rs for building the native libs as the version used to generate the bindings files.

@thunderbiscuit
Copy link
Collaborator Author

We might want to discuss how to merge the Python one into this to have only one generate binary, but for this PR I wanted to keep it as simple as possible, and because Python requires special care with the replacement of some lines in the bindings file, I decided to focus the work on just Kotlin and Swift.

@notmandatory
Copy link
Member

notmandatory commented Mar 28, 2022

I took a stab at rewriting this with the structopt dependency which makes it easy to get cli or env program options. See my fork here. I also simplified the env names @afilini used for the original generate python tool.

Using structopt we also get pretty help messages for free:

% cargo run --bin generate-bindings -- --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/generate-bindings --help`
generate-bindings 0.3.1
A tool to generate bdk-ffi language bindings

USAGE:
    generate-bindings [OPTIONS] --language <language> --out-dir <out-dir>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -l, --language <language>
            Language to generate bindings for [env: UNIFFI_BINDGEN_LANGUAGE=]  [possible values: kotlin, swift, python]

    -o, --out-dir <out-dir>
            Output directory to put generated language bindings [env: UNIFFI_BINDGEN_OUTPUT_DIR=]

    -p, --python-fixup-path <python-fixup-path>    Python fix up lib path [env: UNIFFI_BINDGEN_PYTHON_FIXUP_PATH=]

@thunderbiscuit
Copy link
Collaborator Author

Will review and test tomorrow. Looks good so far! As long as we're ok with adding the dependency, I think we get a much cleaner tool.

@thunderbiscuit
Copy link
Collaborator Author

thunderbiscuit commented Mar 30, 2022

Ok this is 10x better than mine. testedACK on @notmandatory's branch.

I tested with all 3 languages, including the Python path fix. Everything worked as expected. I'm not sure of the potential costs of adding the dependency so I'll leave this to others to chime in, but if we are ok with adding structopt then this is the way to go.

I wonder if we could hide the

println!("Python fix up lib path is {:?}", opt.python_fixup_path);

inside the if opt.language == Language::PYTHON statement so as not to show it when kotlin or swift is the chosen language.

@notmandatory
Copy link
Member

Thanks for testing, I'll make a new PR from my fork and move the python println! so it won't show for the other langs. I also want to see if I can have the structopt dependency only for the bin, since it's not required at all for the lib.

@thunderbiscuit
Copy link
Collaborator Author

Sounds good. When your PR comes in I'll close this one. I'll also start working on integrating this in the Gradle plugin for bdk-jvm and bdk-android.

@thunderbiscuit
Copy link
Collaborator Author

FYI after asking a question on the uniffi-rs repo about how to make sure the two parts of building the bindings libraries are always in sync version-wise, I got insights into how they're doing it. The answer is that it's a known issue for sure, and in fact their solution is to have a separate crate used exclusively for making sure the two versions are in sync. It's very similar to the idea here of having a binary that calls the uniffi-bindgen tool internally rather than relying on the command-line tool.

Their external crate is here.

@notmandatory notmandatory removed this from the Release 0.5.0 milestone Mar 31, 2022
notmandatory added a commit that referenced this pull request Apr 2, 2022
b207464 Update README.md with bdk-ffi-bindgen info (Steve Myers)
fca5d16 Add workspace and move bin to bdk-ffi-bindgen package (Steve Myers)
f4e097c Only print python fix up lib path if used (Steve Myers)
c66dfdd Use structopt to capture generate options (Steve Myers)
ce84872 Add binary to remove the need for uniffi-bindgen cli tool (thunderbiscuit)

Pull request description:

  This PR is based on the mozilla/application-services [embedded-uniffi-bindgen](https://github.com/mozilla/application-services/tree/main/tools/embedded-uniffi-bindgen) tool. The purpose is to keep the bdk-ffi and bdk-ffi-bindgen tool in sync with the same version of uniffi-rs.

  Fixes #124, this PR replaces #122.

  The `bdkffi` library code remains unchanged but the `bin/generate` and `bin/generate-bindings` bins are combined and put in a new workspace binary package called `bdk-ffi-bindgen`.  The `bdk-ffi-bindgen` binary uses the following options, defaults, and environment variables:

  ```shell
  % cargo run -p bdk-ffi-bindgen -- --help

  bdk-ffi-bindgen 0.1.0
  A tool to generate bdk-ffi language bindings

  USAGE:
      bdk-ffi-bindgen [OPTIONS] --language <language> --out-dir <out-dir>

  FLAGS:
      -h, --help       Prints help information
      -V, --version    Prints version information

  OPTIONS:
      -l, --language <language>
              Language to generate bindings for [env: BDKFFI_BINDGEN_LANGUAGE=]  [possible values: kotlin, swift, python]

      -o, --out-dir <out-dir>
              Output directory to put generated language bindings [env: BDKFFI_BINDGEN_OUTPUT_DIR=]

      -p, --python-fixup-path <python-fixup-path>    Python fix up lib path [env: BDKFFI_BINDGEN_PYTHON_FIXUP_PATH=]
      -u, --udl-file <udl-file>                      UDL file [env: BDKFFI_BINDGEN_UDL=]  [default: src/bdk.udl]

  ```

Top commit has no ACKs.

Tree-SHA512: fa1a1c097fe5d0e704d76078c10f82c466dad5d045c8c93d579c2d13c448c52fb6a4f99dfd3dbc46be30471477ae2d1f9264201e14bae7948b408c8e0b3c9b81
@notmandatory
Copy link
Member

Closing in favor of #126. But thanks for lighting the fire on this one!

@thunderbiscuit thunderbiscuit deleted the fix/generate-bindings-binary branch April 2, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants