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

Bump jni-sys to 0.4.0 and jni to 0.22 #433

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Oct 4, 2023

@MarijnS95
Copy link
Member Author

Expected a local compiler error, but upon closer inspection jni is only used in a doc-example. Let's double-check that the CI catches this, though we'll then block on jni-rs/jni-rs#478.

@MarijnS95 MarijnS95 added the impact: breaking API/ABI-breaking change label Oct 4, 2023
@MarijnS95 MarijnS95 changed the title Bump jni-sys to 0.4.0 Bump jni-sys to 0.4.0 and jni to 0.22 Oct 4, 2023
@MarijnS95
Copy link
Member Author

It's not here yet, but I've prepared this PR for jni 0.22 (except possible Rust code changes) so that it should™ be as trivial as re-running the CI to get this merged, when that release happens.

@rib
Copy link
Contributor

rib commented Oct 5, 2023

Cool, I'd like to make a jni release soon but if I'm honest I'm not currently expecting it to happen before the 0.5 release of android-activity or winit 0.29.

@MarijnS95
Copy link
Member Author

Sure, just keep in mind that that's going to be a hassle for the community as the ndk won't be able to make such a breaking release for a while until winit does... We seem to be on a ± yearly breaking release schedule.

@rib
Copy link
Contributor

rib commented Oct 5, 2023

Surely the jni / jni-sys crates are only internal deps for the ndk crate so you should be ok to bump later?

I intentionally don't expose any jni types for android-activity so it wouldn't affect semver to bump.

@MarijnS95
Copy link
Member Author

Surely the jni / jni-sys crates are only internal deps for the ndk crate

This PR is marked as breaking for a reason 😉

@rib
Copy link
Contributor

rib commented Oct 5, 2023

Surely the jni / jni-sys crates are only internal deps for the ndk crate

This PR is marked as breaking for a reason 😉

where do the ndk / ndk-sys crates currently expose jni/jni-sys types? that doesn't sound good to me and maybe we can try to remove any exposure?

@rib
Copy link
Contributor

rib commented Oct 5, 2023

We seem to be on a ± yearly breaking release schedule

It could be nice if we could do more frequent releases and maybe not worry too much whether every release necessarily gets picked up by Winit.

@MarijnS95
Copy link
Member Author

where do the ndk / ndk-sys crates currently expose jni/jni-sys types? that doesn't sound good to me and maybe we can try to remove any exposure?

Only jni-sys types. Quite a few JNIEnv/jobjects in constructors. jni should only be used for a doc-example.

It could be nice if we could do more frequent releases and maybe not worry too much whether every release necessarily gets picked up by Winit.

We could, but it's annoying to have a mismatch between all crates that are so intertwined with windowing systems.

@rib
Copy link
Contributor

rib commented Oct 10, 2023

where do the ndk / ndk-sys crates currently expose jni/jni-sys types? that doesn't sound good to me and maybe we can try to remove any exposure?

Only jni-sys types. Quite a few JNIEnv/jobjects in constructors. jni should only be used for a doc-example.

I was pondering for a second whether we could do a 0.3.x release of the jni-sys crate with the semver trick. That's a tempting solution but really the JNIEnv type completely changed between jni-sys 0.3 and 0.4.

That's awkward in the sense that for 99% of things (and probably all the ndk crate usage) you only care about passing around an opaque pointer and probably don't ever dereference the JNIEnv pointer type.

@MarijnS95
Copy link
Member Author

@rib yeah we don't look at the innards of the type. Having a type-safe definition does help things not getting mixed up, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: breaking API/ABI-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants