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

chore(deps): Upgrade to ndk 0.9 and delete unused ndk-sys/ndk-context dependencies #1296

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Jun 17, 2024

Closes #1060
Closes #1061

The ndk crate received some marginal API upgrades, besides fixing soundness issues. Specifcally, ForeignLooper::add_fd_with_callback() now signifies that the incoming file descriptor is a BorrowedFd and the callback is executed on a different thread (where the looper is polled on the ThreadLooper) and must hence be Send..

It appears the ndk-sys and ndk-context crates are not used directly, hence they are removed from Cargo.toml here.

@MarijnS95 MarijnS95 requested a review from a team as a code owner June 17, 2024 21:51
let mut pipe: [RawFd; 2] = Default::default();
unsafe { libc::pipe(pipe.as_mut_ptr()) };
pipe
unsafe { pipe.map(|fd| OwnedFd::from_raw_fd(fd)) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be more convenient to use rustix here.

Copy link
Member

@amrbashir amrbashir Jul 17, 2024

Choose a reason for hiding this comment

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

let's keep what works for now, I'd rather migrate to something based on android-activity crate in the future.

src/android/mod.rs Show resolved Hide resolved
@amrbashir amrbashir requested a review from wusyong June 27, 2024 02:35
Copy link
Contributor

github-actions bot commented Jul 17, 2024

Package Changes Through fc4d54a

There are 1 changes which include wry with minor

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
wry 0.41.0 0.42.0

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

amrbashir
amrbashir previously approved these changes Jul 17, 2024
amrbashir
amrbashir previously approved these changes Jul 17, 2024
Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

Thank you, just waiting for tauri-apps/tao#955

@MarijnS95 MarijnS95 force-pushed the ndk-0.9 branch 2 times, most recently from 00f4d08 to 58263c1 Compare July 17, 2024 10:07
MarijnS95 added a commit to MarijnS95/tao that referenced this pull request Jul 17, 2024
Most changes were copied over from my `wry` PR at
tauri-apps/wry#1296.  Most notably a
`&ThreadLooper` is now passed to `wry::android_setup()` because we
have one, and it gives important safety guarantees when it comes to
registering callbacks on this looper, without (unnecessarily!) requiring
`Send`.  A thread-local requirement already exists for the `JNIEnv` that
is passed around anyway.

Also note that certain workarounds and illogical inverted passes
around `key_code()` handling are no longer needed, as the `ndk` crate
now passes an `enum` with the raw `i32` around so that the `.into()`
conversion for `i32` (the correct type) now becomes lossless.
MarijnS95 added a commit to MarijnS95/tao that referenced this pull request Jul 17, 2024
Most changes were copied over from my `wry` PR at
tauri-apps/wry#1296.  Most notably a
`&ThreadLooper` is now passed to `wry::android_setup()` because we
have one, and it gives important safety guarantees when it comes to
registering callbacks on this looper, without (unnecessarily!) requiring
`Send`.  A thread-local requirement already exists for the `JNIEnv` that
is passed around anyway.

Also note that certain workarounds and illogical inverted passes
around `key_code()` handling are no longer needed, as the `ndk` crate
now passes an `enum` with the raw `i32` around so that the `.into()`
conversion for `i32` (the correct type) now becomes lossless.
@MarijnS95 MarijnS95 changed the title chore(deps): Upgrade to ndk 0.9 and delete unused ndk-sys/context dependencies chore(deps): Upgrade to ndk 0.9 and delete unused ndk-sys/ndk-context dependencies Jul 17, 2024
Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

Could you also add a change file in .changes directory?

@MarijnS95 MarijnS95 force-pushed the ndk-0.9 branch 2 times, most recently from 19fa538 to 3773ee3 Compare July 18, 2024 09:03
MarijnS95 added a commit to MarijnS95/tao that referenced this pull request Jul 18, 2024
Most changes were copied over from my `wry` PR at
tauri-apps/wry#1296.  Most notably a
`&ThreadLooper` is now passed to `wry::android_setup()` because we
have one, and it gives important safety guarantees when it comes to
registering callbacks on this looper, without (unnecessarily!) requiring
`Send`.  A thread-local requirement already exists for the `JNIEnv` that
is passed around anyway.

Also note that certain workarounds and illogical inverted passes
around `key_code()` handling are no longer needed, as the `ndk` crate
now passes an `enum` with the raw `i32` around so that the `.into()`
conversion for `i32` (the correct type) now becomes lossless.
@MarijnS95 MarijnS95 requested a review from amrbashir July 18, 2024 09:03
.changes/ndk-0.9.md Outdated Show resolved Hide resolved
The `ndk` crate received some marginal API upgrades, besides fixing
soundness issues.  Specifcally, `ForeignLooper::add_fd_with_callback()`
now signifies that the incoming file descriptor is a `BorrowedFd` and
the callback is executed on a different thread (where the looper is
polled on the `ThreadLooper`) and must hence be `Send.`.

It appears the `ndk-sys` and `ndk-context` crates are not used directly,
hence they are removed from `Cargo.toml` here.
MarijnS95 added a commit to MarijnS95/tao that referenced this pull request Jul 18, 2024
Most changes were copied over from my `wry` PR at
tauri-apps/wry#1296.  Most notably a
`&ThreadLooper` is now passed to `wry::android_setup()` because we
have one, and it gives important safety guarantees when it comes to
registering callbacks on this looper, without (unnecessarily!) requiring
`Send`.  A thread-local requirement already exists for the `JNIEnv` that
is passed around anyway.

Also note that certain workarounds and illogical inverted passes
around `key_code()` handling are no longer needed, as the `ndk` crate
now passes an `enum` with the raw `i32` around so that the `.into()`
conversion for `i32` (the correct type) now becomes lossless.
amrbashir pushed a commit to tauri-apps/tao that referenced this pull request Jul 18, 2024
…956)

Most changes were copied over from my `wry` PR at
tauri-apps/wry#1296.  Most notably a
`&ThreadLooper` is now passed to `wry::android_setup()` because we
have one, and it gives important safety guarantees when it comes to
registering callbacks on this looper, without (unnecessarily!) requiring
`Send`.  A thread-local requirement already exists for the `JNIEnv` that
is passed around anyway.

Also note that certain workarounds and illogical inverted passes
around `key_code()` handling are no longer needed, as the `ndk` crate
now passes an `enum` with the raw `i32` around so that the `.into()`
conversion for `i32` (the correct type) now becomes lossless.
@amrbashir amrbashir merged commit 68413e8 into tauri-apps:dev Jul 18, 2024
14 checks passed
@amrbashir
Copy link
Member

thank you

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.

2 participants