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

Update jni to 0.21 #151

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Update jni to 0.21 #151

merged 3 commits into from
Nov 18, 2024

Conversation

Cyannide
Copy link

@Cyannide Cyannide commented Nov 9, 2024

Rust JNI 0.21 introduced some breaking changes.

The main reason for needing JNI 0.21 here is that android-activity 0.6.0 is based on it and the JNIEnv references are incompatible.

#63 and #22

@Cyannide Cyannide force-pushed the main branch 2 times, most recently from 811d5b0 to eb21840 Compare November 9, 2024 03:09
@Cyannide Cyannide marked this pull request as draft November 9, 2024 03:10
@Cyannide Cyannide marked this pull request as ready for review November 9, 2024 05:35
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! Could you squash all the JNI-related commits into one so it's cleanly separate from the Let's Encrypt certificate update?

rustls-platform-verifier/src/verification/android.rs Outdated Show resolved Hide resolved
@Cyannide
Copy link
Author

Thanks for picking this up! Could you squash all the JNI-related commits into one so it's cleanly separate from the Let's Encrypt certificate update?

Yep. Will get that done this morning.

@cpu
Copy link
Member

cpu commented Nov 11, 2024

@Cyannide Thanks for picking this up.

@complexspaces Do you think jni-rs/jni-rs#513 is a blocker for this?

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks again.

I think there's a couple clippy/rustfmt nits to sort out, but overall this seems 👍 It also looks like the diff in this branch ended up a little bit less involved than in #63, nice to see that.

Beyond getting CI passing I think we should wait for Complexspaces to weigh in w.r.t the upstream window-sys dep before merging.

rustls-platform-verifier/src/android.rs Show resolved Hide resolved
Breaking changes for Android.
@complexspaces
Copy link
Collaborator

complexspaces commented Nov 12, 2024

@complexspaces Do you think jni-rs/jni-rs#513 is a blocker for this?

It used to be a blocker (IMO) until it became clear that jni is essentially abandoned, therefore removing the better path of just holding out ~N months and solving the root cause. However now that a legitimate use case for the upgrade has come up (needing compatible types between crates), I don't see a reason not to do this. Additionally while it does cause lockfile bloat, its not nearly as bad as other cases because almost no one is using the jni crate not-for-Android. You won't get two windows-sys in your active build, the ancient 0.45 copy this brings in will just sit.

If someone is writing a Java extension that compiles on Windows, and maybe uses this crate: sorry 😅?

FWIW what do people think about dropping the Cargo bump commit currently here? It makes it slightly more annoying to do releases because there's no easy diff to open a branch on without that. We don't guarantee the main branch will be properly versioned for anyone using a git dependency.

I'm happy to give this a proper review/lookover tomorrow or Wednesday.

@complexspaces
Copy link
Collaborator

I'd love to see a maintained version of jni this crate could use, but I don't have the time or expertise to make that happen 😔

@djc
Copy link
Member

djc commented Nov 12, 2024

FWIW what do people think about dropping the Cargo bump commit currently here? It makes it slightly more annoying to do releases because there's no easy diff to open a branch on without that. We don't guarantee the main branch will be properly versioned for anyone using a git dependency.

Not sure I entirely understand the annoyance here? In other rustls projects we like to bump proactively in PRs that make incompatible changes, as a way of recording that incompatible changes have been made. It can be pretty hard to establish after the fact whether incompatible changes have been made once a substantial amount of changes have been made...

(That is, this is intended more as a help for the maintainers than it is for git branch consumers.)

@Cyannide
Copy link
Author

I'd love to see a maintained version of jni this crate could use, but I don't have the time or expertise to make that happen 😔

I'll take a look at the Windows side of things tomorrow to see what the issues there are to overcome. I'm also of the opinion that if someone is honestly building something on Windows that requires the JNI dependency, sorry.

@complexspaces
Copy link
Collaborator

FWIW, If I'm understanding you correctly, I don't think its really about overcoming issues. The jni developers have been presented two options, and responded to neither as of writing this:

  • Update windows-sys to a reasonable version.
  • Use windows-bindgen instead to call the very few functions it needs manually to avoid this problem in the future (I offered to implement it for them).

@cpu
Copy link
Member

cpu commented Nov 18, 2024

I'm happy to give this a proper review/lookover tomorrow or Wednesday.

gentle bump.

If it's going to take you a little bit to find time to review the meat of this branch maybe we could pull out the CI fix separately so I don't get emails about the cron build failing? 🙇

@complexspaces
Copy link
Collaborator

gentle bump.

Thanks! This looks good to go 👍.

I had looked over this last week but I just did it offhand and not in an actual GitHub review.

@cpu cpu merged commit a493f9a into rustls:main Nov 18, 2024
20 checks passed
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.

4 participants