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

added support for x86_64-pc-windows-msvc 64-bit MSVC #336

Conversation

yellowHatpro
Copy link
Contributor

Description

Added support for support_added_x86_64-pc-windows-msvc_64-bit_MSVC target

Notes to the reviewers

  • added enum value for windows
  • I used a variable libName to denote the library name, which we are copying from target folder into the resource folder, because apparently in windows the dynamic library file e.g. .dll file (equivalent to .so file in linux) was generated by the name bdkffi.dll, which in case of Linux and mac was libbdkffi .

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • [] I've added tests for the new feature
  • [] I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@yellowHatpro
Copy link
Contributor Author

#240 fixes for x86_64-pc-windows-msvc_64-bit_MSVC

@yellowHatpro
Copy link
Contributor Author

@thunderbiscuit I was adding the support for Android emulator as well. I had a doubt going through the UniFfiAndroidPlugin file.
https://github.com/bitcoindevkit/bdk-ffi/blob/master/bdk-android/plugins/src/main/kotlin/org/bitcoindevkit/plugins/UniFfiAndroidPlugin.kt#L113 .
In this part, why all libraries that are to be generated are of type libbdkffi.so ?
In bdk-jvm, we were checking them according to OS, e.g. .dylib for Mac, .so for Linux. Why not here?

@thunderbiscuit
Copy link
Member

I think in the case of Android, they're all running on a Linux machine (Android being a sort of fork of Linux) and the emulator is the same. That's why all files required are of .so type.

@thunderbiscuit
Copy link
Member

Wondering if @darkvoid32 would be interested in testing out this PR? We basically need to verify that it builds and runs on Windows.

@fivetran-tangyetong
Copy link
Contributor

Yup, I can get to testing out this PR soon (few days?).

@thunderbiscuit
Copy link
Member

The way you'd build and test is by doing the following:

cd ./bdk-jvm/
./gradlew buildJvmLib
./gradlew test

@fivetran-tangyetong
Copy link
Contributor

Seems like running ./gradlew buildJvmLib causes this error to pop up:

$ ./gradlew buildJvmLib --stacktrace
    Updating crates.io index
error: failed to select a version for the requirement `uniffi = "^0.23.0"`
candidate versions found which didn't match: 0.22.0, 0.21.1, 0.21.0, ...
location searched: crates.io index
required by package `bdk-ffi v0.27.1 (D:\Coding\Work\BDK\bdk-ffi\bdk-ffi)`

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring project ':lib'.
> Could not create task ':lib:buildJvmLib'.
   > Could not create task ':lib:buildJvmBinaries'.
      > Process 'command 'cargo'' finished with non-zero exit value 101

* Try:
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Exception is:
org.gradle.api.ProjectConfigurationException: A problem occurred configuring project ':lib'.
        at org.gradle.execution.TaskNameResolver.getExistingTask(TaskNameResolver.java:116)
        at org.gradle.execution.TaskNameResolver.access$000(TaskNameResolver.java:32)
        at org.gradle.execution.TaskNameResolver$MultiProjectTaskSelectionResult.collect(TaskNameResolver.java:177)
		...

@yellowHatpro
Copy link
Contributor Author

@darkvoid32 hi, can you confirm on which toolchain you are currently on?
you can tell the result of this command: rustup show

@fivetran-tangyetong
Copy link
Contributor

This is what I get:

Default host: x86_64-pc-windows-msvc
rustup home:  C:\Users\tangy\.rustup

installed targets for active toolchain
--------------------------------------

aarch64-linux-android
armv7-linux-androideabi
i686-linux-android
x86_64-apple-darwin
x86_64-linux-android
x86_64-pc-windows-msvc
x86_64-unknown-linux-gnu

active toolchain
----------------

stable-x86_64-pc-windows-msvc (default)
rustc 1.59.0 (9d1b2106e 2022-02-23)

@yellowHatpro
Copy link
Contributor Author

Umm, I'm not sure of this step, but we have to ig link the build.gradle.kts of bdk-jvm first, I am not sure if it's a step or not, but I definitely got a message regarding this when I clicked on the build file in bdk-jvm module. Did you do something similar?

@fivetran-tangyetong
Copy link
Contributor

Looks like I didn't get that message, just let me know what step it was in case I missed it! Hopefully didn't miss a step while building the project.

@thunderbiscuit
Copy link
Member

we have to ig link the build.gradle.kts of bdk-jvm first

If I understand correctly what you mean here, I think this would be just an IntelliJ/Android Studio IDE link that would not affect the build if you build from your command line.

@darkvoid32 I'm not sure why your Rust is not able to find uniffi-rs 0.23... I wonder if they maybe don't release for certain targets? @notmandatory any ideas?

@yellowHatpro
Copy link
Contributor Author

@darkvoid32 could it be due to rustc / cargo version?
the version of your rustc is 1.59 while the current stable is 1.68

@thunderbiscuit
Copy link
Member

By the way there is a bug in rust-miniscript with the 1.68 release of the Rust compiler... Dark if you can, use 1.67

@fivetran-tangyetong
Copy link
Contributor

Thanks for catching the super outdated rust version, updating rustup to version 1.67.0 fixed the issue. I can compile and run the tests now.

$ ./gradlew test
    Finished release-smaller [optimized] target(s) in 0.48s

> Task :lib:generateJvmBindings
    Finished dev [unoptimized + debuginfo] target(s) in 0.41s
     Running `D:\Coding\Work\BDK\bdk-ffi\target\debug\uniffi-bindgen.exe generate src/bdk.udl --language kotlin --out-dir ../bdk-jvm/lib/src/main/kotlin --no-format`
JVM bindings file successfully created

> Task :lib:test

JvmLibTest > sqliteWalletSyncGetBalance PASSED

JvmLibTest > memoryWalletSyncGetBalance PASSED

JvmLibTest > memoryWalletNewAddress PASSED

BUILD SUCCESSFUL in 26s
11 actionable tasks: 6 executed, 5 up-to-date

@thunderbiscuit
Copy link
Member

Big step forward! I'll add this to the agenda for the dev call next week and we can look at merging for the next release. Thanks for the hard work @yellowHatpro and the review @darkvoid32!

@yellowHatpro
Copy link
Contributor Author

Glad it worked ⚡(≧∇≦)ノ

@thunderbiscuit thunderbiscuit force-pushed the support_added_x86_64-pc-windows-msvc_64-bit_MSVC branch from 3a03306 to 90606b2 Compare June 5, 2023 14:10
@thunderbiscuit thunderbiscuit self-requested a review June 5, 2023 14:10
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 90606b2.

@thunderbiscuit thunderbiscuit merged commit 90606b2 into bitcoindevkit:master Jun 5, 2023
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