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

ndk-sys: Regenerate against NDK r23 #178

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

MarijnS95
Copy link
Member

Android 12 reached platform stability today, including the first stable release of NDK r23.


Will add some of the new API surface to our safe wrappers in a separate PR. Perhaps it's a good idea to regenerate against identical (r22.1, beta 3) headers first but with newer bindgen, to make the diff more clear? Not that it matters for squash-merges unless we put that in a separate PR too, LMK.

At the same time we might want to start thinking about guarding symbols based on a user-selected version. We could for example have a feature version per NDK or per SDK/API version so that there is a clear up-front contract for the programmer that they can only call certain certain functions if at least some version feature is enabled; that's how the various gtk-rs bindings do it. They have the advantage of directly querying that exact version from pkg-build though, and running into linker errors immediately if things don't match.

@msiglreith
Copy link
Contributor

I'm generally fine with the PR as is.

At the same time we might want to start thinking about guarding symbols based on a user-selected version. We could for example have a feature version per NDK or per SDK/API version so that there is a clear up-front contract for the programmer that they can only call certain certain functions if at least some version feature is enabled; that's how the various gtk-rs bindings do it. They have the advantage of directly querying that exact version from pkg-build though, and running into linker errors immediately if things don't match.

Is this feature guarding on a per iterm level or generating having different. seperately generated bindings per API version? Does Android NDK provide fine grained metadata to do this?

@MarijnS95
Copy link
Member Author

Is this feature guarding on a per iterm level or generating having different. seperately generated bindings per API version? Does Android NDK provide fine grained metadata to do this?

I don't think we can apply this to bindgen code, and afaik the version constraints are only specified in documentation comments. The idea was to only apply this to the handwritten wrappers in ndk (not ndk-sys), but it's not really an idea since we already have this:

https://github.com/rust-windowing/android-ndk-rs/blob/a8c13855f4c651496e26c203fa22d9639b6cb77c/ndk/Cargo.toml#L23-L30

I don't exactly remember why I was suggesting this if we clearly already have it, maybe I was looking at functions that were not guarded by these yet. Will have to re-check, but I'm definitely using it on the new code.

Copy link
Contributor

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

I think this needs a CHANGELOG entry as well. Technically, some parts seem breaking API changes even though it may affect types no-one uses anyway (e.g ACameraMetadata_entry__bindgen_ty_1)

Android 12 reached platform stability today, including the first stable
release of NDK r23.
@MarijnS95
Copy link
Member Author

I think this needs a CHANGELOG entry as well.

Added, thanks!

Technically, some parts seem breaking API changes even though it may affect types no-one uses anyway (e.g ACameraMetadata_entry__bindgen_ty_1)

Let's perform a minor release for this. Even though the NDK itself is backwards-compatible, these files generated with a newer bingen definitely are not. Might want to give it some time while we add the new APIs, though.

@MarijnS95 MarijnS95 mentioned this pull request Aug 28, 2021
@msiglreith msiglreith merged commit e8765fe into rust-mobile:master Aug 31, 2021
@MarijnS95 MarijnS95 deleted the regenerate-sys branch August 31, 2021 10:39
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