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.

Sign up for GitHub

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

Jump to bottom

Bump uniffi to 0.26 #451

Merged
merged 3 commits into from
Feb 6, 2024
Merged

Bump uniffi to 0.26 #451

merged 3 commits into from
Feb 6, 2024

Conversation

reez
Copy link
Collaborator

@reez reez commented Jan 24, 2024 •
edited
Loading

Description

Bump uniffi to 0.26 because the release adds support for things like:

  • async methods
  • non_exhaustive enums

Notes to the reviewers

versions of note

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

@reez reez force-pushed the u26 branch 4 times, most recently from 58b75d3 to d1ef620 Compare January 24, 2024 20:20
@tnull
Copy link

tnull commented Jan 25, 2024

Could consider using the new android_cleaner flag for the Android build, see lightningdevkit/ldk-node@3345ade

@reez reez force-pushed the u26 branch 4 times, most recently from 337b62b to 8c87e8c Compare January 25, 2024 20:19
@reez
Copy link
Collaborator Author

reez commented Jan 25, 2024

Could consider using the new android_cleaner flag for the Android build, see lightningdevkit/ldk-node@3345ade

Thanks, you rule!

@reez reez marked this pull request as ready for review January 25, 2024 22:06
@reez reez self-assigned this Jan 25, 2024
@reez
Copy link
Collaborator Author

reez commented Feb 5, 2024

(Just rebased on master to resolve new cargo.lock conflict)

thunderbiscuit
thunderbiscuit requested changes Feb 6, 2024
View reviewed changes
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.

Looks good to me. Just a few nits down below.

bdk-android/lib/build.gradle.kts
@@ -19,7 +19,7 @@ repositories {
}

android {
compileSdk = 31
compileSdk = 33
Copy link
Member

@thunderbiscuit thunderbiscuit Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we update the targetSdk to 33 as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure! updated, see line 26 for the targetSdk = 33 change

Copy link

Choose a reason for hiding this comment

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

Note that updating targetSdk might not be necessary, while compileSdk is necessary due to the SystemCleaner changes in UniFFI 0.26.

bdk-android/lib/build.gradle.kts Outdated
@@ -44,7 +44,7 @@ android {
}

dependencies {
implementation("net.java.dev.jna:jna:5.8.0@aar")
implementation("net.java.dev.jna:jna:5.12.0@aar")
Copy link
Member

Choose a reason for hiding this comment

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

Small inconsistency between files, where some use 5.12 and some use 5.14.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks! changed.

.github/workflows/cont_integration.yml
@@ -56,7 +56,7 @@ jobs:
run: cargo clippy --all-targets --features "uniffi/bindgen-tests" -- -D warnings

- name: "Test"
run: CLASSPATH=./tests/jna/jna-5.8.0.jar cargo test --features uniffi/bindgen-tests
run: CLASSPATH=./tests/jna/jna-5.14.0.jar cargo test --features uniffi/bindgen-tests
Copy link
Member

Choose a reason for hiding this comment

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

See below comment on 5.14.

Copy link
Collaborator Author

@reez reez Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(line 62 below is changed now)

bdk-jvm/lib/build.gradle.kts Outdated
@@ -59,7 +59,7 @@ tasks.withType<Test> {
dependencies {
implementation(platform("org.jetbrains.kotlin:kotlin-bom"))
implementation("org.jetbrains.kotlin:kotlin-stdlib-jdk7")
implementation("net.java.dev.jna:jna:5.8.0")
implementation("net.java.dev.jna:jna:5.12.0")
Copy link
Member

Choose a reason for hiding this comment

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

I think this one should say 5.14.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to 5.14 now, thanks!

thunderbiscuit
thunderbiscuit approved these changes Feb 6, 2024
View reviewed changes
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 50f102b.

@thunderbiscuit thunderbiscuit merged commit 50f102b into bitcoindevkit:master Feb 6, 2024
17 checks passed
@reez reez deleted the u26 branch February 6, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnull tnull tnull left review comments

@thunderbiscuit thunderbiscuit thunderbiscuit approved these changes

Assignees

@reez reez

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reez @tnull @thunderbiscuit