Release planning

[rib]

Hey @MarijnS95, it would be good to know if there's a release being planned for the ndk and ndk-sys crates?

In particular an ndk-sys release would be really appreciated, since I'd like to be able to use AMotionEvent_getActionButton.

[MarijnS95]

We discussed this last week in rust-windowing/winit#2905 (comment), do we need a second issue to track this?

[MarijnS95]

Note that we can then also use https://github.com/MarijnS95/android-activity/compare/ndk-breaking-prep to apply the new breaking release to android-activity.

[rib]

I figured it's helpful having a specific issue to ask about this in the context of this repo.

I also don't always remember which miscellaneous thread we have side discussions on.

In the case of supporting AMotionEvent_getActionButton I'd also be able to use that independent of upstreaming corresponding Winit support.

I'm not sure exactly what you're planning to land before making an ndk release but maybe if you don't expect any ndk-sys changes it could be good if it might be possible to release that sooner.

[MarijnS95]

All the PRs open right now are supposed to be additive, and could eventually land shortly after a breaking release as patch releases.

However, one PR (#397) adds more API to the sys crate, with the unfortunate side-effect that it has anonymous enums whose types get relabeled to _bindgen_ty_xx where xx is now incremented for every other anonymous enum that follows in later headers. This would be a breaking change even if it is discouraged to use those types.

If there appears to be time pressure I can and will extract/cherry-pick just the ndk-sys changes for a release, though I'd like to release both crates in tandem.

Tangent: improving the situation

I'd appreciate if bindgen has a way to newtype such anonymous enums when all their contents share a common prefix, but have not yet found a way nor issue detailing this use-case. Perhaps it is time to open one, as that both improves ergonomics similar to the existing --newtype-enums as well as get rid of this automatic numbering clash.

Alternatively I could move the new headers to the end so that they don't affect existing enumerations, but that falls apart when a newer (additive/backwards-compatible!) NDK releases with more anonymous enums spread across the files.

[rib]

It would probably be OK to document that those types aren't covered by the crates semver - similar to how crates will explicitly document that they consider certain unstable APIs to fall outside of their semver.

Those types getting renamed shouldn't break the API for anyone.

Although they are technically public, they are conceptually intended to be private, and their only purpose is to be unique across the enum.

I suppose it would be neat if bindgen could somehow implement some kind of heuristic for naming those based on any common prefix it finds for all members of the enum and fallback to numbering when that fails.

Having a bit of a look at bindgen - have you already poked at rustified_enum? I wonder if that could help here - though that may lead to needing a large boilerplate change to the ndk crate to use generated rust-style enums for these types.

[rib]

At least the --rustified-enum option doesn't really help since that just moves the generated name to being the name of the Rust enum.

Seems like it should be possible to give bindgen a name for anonymous enums that could be based on a regex match for any of the members (first match wins) - or else some default behaviour that would let it clip a common prefix from all members and use that as a name.

[rib]

I probably shouldn't have got distracted looking at this last night but out of curiosity I ended up cooking up an experimental API for being able to deanonymize enums in bindgen via ParserCallbacks: https://github.com/rib/rust-bindgen/commits/rib/wip/deanonymize-enums

That has an initial proof of concept for being able to name an enum based on the names of all its variants, and shows how we could e.g. match and rename specific prefixes or else automatically infer a name by looking for a common prefix across all variants.

One additional thing that could be nice to have here is some way of telling bindgen to strip a prefix from the name of variants if an enum is automatically named based on finding a common prefix. The order of things would be a bit more fiddly for supporting that.

I'm probably not going to continue with this right now, but maybe it's be possible to upstream something like this that we could use here.

I guess it would involve creating a separate ndk-bindgen crate in the repo based on the bindgen library interface that can be used instead of the generate-bindings.sh script for generating bindings offline - since I can't really imagine a nice way of exposing this kind of thing via bindgen-cli.

[MarijnS95]

Yeah I would want to document those types to be exempt but it seems rather annoying if one has to use them (at least we're not using them publicly in any of the safe API, and they can't/won't be used as struct field types or method signatures in -sys either, exactly because they're... anonymous).

---

At least the --rustified-enum option doesn't really help since that just moves the generated name to being the name of the Rust enum.

Oh I didn't even expect this to look at constant names, only at typed enums... Cool I guess? Seems we can do the same with --newtype-enum (what we already have) and it follows the expected format though with the anonymous name.

I probably shouldn't have got distracted looking at this last night but out of curiosity I ended up cooking up an experimental API for being able to deanonymize enums in bindgen via ParserCallbacks: https://github.com/rib/rust-bindgen/commits/rib/wip/deanonymize-enums

I wish I had as much time as you... Cool idea but would like to run this from a script instead of implementing custom parser callbacks.

As for the rest, agreed with all these problems and I'd rather have the simplest solution: a CLI option that allows me to hardcode the expected prefix, as we already do for all other known enums (and they're very easy to find by searching for _bindgen_ty_x in the generated code). Then bindgen simply emits an enum with the given name (though CamelCased) and slurp all matching constants into it?

Understanding the prefix from a regex greatly complicates the problem though.

Let's at the very least ask this over at bindgen, so that we can have an upstream(able) solution.

---

For the rest of this planning, I opened some more (breaking) PRs to clean up other API bits. Think that's all for now, but the ndk has always been "that crate" that I end up finding a lot of (usually breaking) cleanups and fixes in right as I want to inch closer to a release.

[rib]

Yeah I would want to document those types to be exempt but it seems rather annoying if one has to use them (at least we're not using them publicly in any of the safe API, and they can't/won't be used as struct field types or method signatures in -sys either, exactly because they're... anonymous).

Yeah I'd think it's probably fine / reasonable to document that they are exempt - the name is clearly intended to be private and nothing should be expecting the name to be stable.

At least the --rustified-enum option doesn't really help since that just moves the generated name to being the name of the Rust enum.

Oh I didn't even expect this to look at constant names, only at typed enums... Cool I guess? Seems we can do the same with --newtype-enum (what we already have) and it follows the expected format though with the anonymous name.

Er, it does only relate to C/C++ enums, it's just that bindgen has a few different options for how to handle codegen for enums (including anonymous enums). I had half wondered enabling generation of rust enums would have avoided these awkward private types.

I probably shouldn't have got distracted looking at this last night but out of curiosity I ended up cooking up an experimental API for being able to deanonymize enums in bindgen via ParserCallbacks: https://github.com/rib/rust-bindgen/commits/rib/wip/deanonymize-enums

I wish I had as much time as you... Cool idea but would like to run this from a script instead of implementing custom parser callbacks.

nah, I don't have as much time as me either :) - I was just stupid and stayed up until 2am experimenting instead of sleeping.

As for the rest, agreed with all these problems and I'd rather have the simplest solution: a CLI option that allows me to hardcode the expected prefix, as we already do for all other known enums (and they're very easy to find by searching for _bindgen_ty_x in the generated code). Then bindgen simply emits an enum with the given name (though CamelCased) and slurp all matching constants into it?

Sure, but it's not a practical problem if we end up needing to write a tiny bindgen runner that uses the library API if it turns out to not be practical to expose the feature via bindgen-cli.

There are other features that aren't accessible via the bindgen-cli and in this kind of situation where we want to do something a bit more sophisticated then it's probably reasonable if it doesn't end up being possible via the command line tool.

Understanding the prefix from a regex greatly complicates the problem though.

Not quite sure what you're thinking here - but maybe imagining how it could be exposed via bindgen-cli?

I'm not sure it would be worth trying to expose this via the CLI - or at least I think it would be more practical to figure out what works for the problem by solving it with the bindgen library API first and then potentially afterwards it might be possible to think of how that could be made to also work with bindgen-cli.

Let's at the very least ask this over at bindgen, so that we can have an upstream(able) solution.

At some point I can probably iterate this and create a [draft] PR to look at upstreaming but I think the main thing first though would be to figure out exactly what would be wanted / needed for the ndk-sys use case (in terms of code we'd want generated in specific situations - not the specific interface for doing so).

For the rest of this planning, I opened some more (breaking) PRs to clean up other API bits. Think that's all for now, but the ndk has always been "that crate" that I end up finding a lot of (usually breaking) cleanups and fixes in right as I want to inch closer to a release.

Yeah I can see how that could happen - I think that's partly also why I was half hoping it might be possible to decouple ndk-sys and release that sooner.

[MarijnS95]

Probably the only thing that annoys me more is that we recreate these enumerations in various ways for the "safe wrappers" in the ndk crate. Perhaps to be able to add more impls, or to make them true (nonexhaustive) enums with the possibility of missing out on new variants if we're not scrutinizing NDK header updates.

Even worse, we have a totally mixed approach to each of these implementations. Some are repr(xx) enums with TryFrom/IntoPrimitive that we can conveniently return an Option/Result for from a getter function, others are pure Rust enums with an Unknown(u32) field because someone needed this in a Rusty struct field (where we could in hindsight have also stored a Result<SafeEnum, u32>): #407. I'm all ears for suggestions, but I might even think about reverting that PR and doing it with a Result as I really don't like the mixed approach nor the extra hand-written (error prone!) matches.

---

Not quite sure what you're thinking here - but maybe imagining how it could be exposed via bindgen-cli?

Yes. See for example --newtype-enum 'acamera_\w+': these are many enums that are selected at once. We could/should probably separate those out. And the prefix for an enum might include an underscore which we'd like to strip off for the final name. And personally I like it if the variants don't include the the name of the enum, that's just unnecessary repetition for a thing that's already namespaced in Rust (but not in C).

At some point I can probably iterate this and create a [draft] PR to look at upstreaming but I think the main thing first though would be to figure out exactly what would be wanted / needed for the ndk-sys use case (in terms of code we'd want generated in specific situations - not the specific interface for doing so).

Sure; not sure what the best way is to describe those needs though. I guess we:

• Want to get rid of _bindgen_tyX for anonymous enums;
• Have a common prefix for all its variants;
• (Alternatively) need a way (regex?) to select all enum variants and manually specify a common prefix / target enum type name;
• Bonus: strip off common prefix (fine to let the user provide that explicitly) for these and existing enums 😬.

---

Yeah I can see how that could happen - I think that's partly also why I was half hoping it might be possible to decouple ndk-sys and release that sooner.

I just really don't want to desync them when it comes to breaking changes. Furthermore we should land the example improvements and port them to android-activity examples as well: rust-mobile/rust-android-examples#7? Maybe android-activity can also use the ndk crate directly 😬

[spencercw]

where we could in hindsight have also stored a Result<SafeEnum, u32>

I'm not really sure this would be an improvement. Just because ndk doesn't know about a private format, doesn't make it an error.

Private formats can be used as inputs as well. For example, I can create a HAL_PIXEL_FORMAT_YV12 hardware buffer by passing HardwareBufferFormat::Unknown(ndk_sys::AHardwareBuffer_Format(0x32315659)) to HardwareBuffer::allocate. It would be quite weird if I had to construct an Err here.

[rib]

I'd guess that many / most? NDK enums should have an Unknown(u32) type variant that allows them to safely catch values that weren't known about at compile time - and it would be bad for them to get mapped to errors.

It would be nice if bindgen made it easy to generate these though.

Generally these enums are just mapped from int constants from the Java SDK which may get extended for new SDK API levels.

It's possible (likely even) that a binary built against one ndk version could later end up running on a device with a higher SDK API level that has introduced new variants and in most situations the added variants aren't going to represent errors - just new types of things that should be gracefully ignored at runtime by older code.

[MarijnS95]

@spencercw fair enough, I used Result for lack of an Either type, but we could make that into an explicit enum.

In other words, turning:

enum SomeConstant {
    X,
    Y,
    #[cfg(feature = "api-level-28")]
    Z,
    Unknown(u32),
}

// many many hand-written match statements with `cfg`s

back into:

#[derive(TryFrom/IntoPrimitive)]
#[repr(u32)]
#[nonehaustive?]
enum SomeConstant {
    X = 1,
    Y = 2,
    #[cfg(feature = "api-level-28")]
    Z = 20,
}

enum PossiblyUnknownSomeConstant {
    SomeConstant(SomeConstant),
    Unknown(u32),
}

(@rib regarding your last point, this is indeed odd if the user gets a value of Z but sees it as Unknown(20) when they compile their app without API 28 but run it on a device on or higher than that version... The intent of the cfg is mostly to prevent them from using it without adequately bumping API version, but it also bites the other way around)

It would be nice if bindgen made it easy to generate these though.

It can't because of Rust limitations afaik. I was really unhappy when reading the enum discriminant RFC that this wasn't thought of: having a catch-all for unknown discriminants. Hence my suggestion to pull it out some way instead of having to write many match statements over and over again.

---

What are your thoughts on this? Again, I mostly care about removing match and at the same time it's nice to be able to do SomeConstant as u32?

[spencercw]

The original version of this was simpler by taking advantage of #[num_enum(catch_all)]. The only reason we have the big match right now is because explicit discriminants on enums with fields weren't supported until Rust 1.66. Surely at some point we can bump the MSRV and simplify all of this?

[rib]

The original version of this was simpler by taking advantage of #[num_enum(catch_all)]. The only reason we have the big match right now is because explicit discriminants on enums with fields weren't supported until Rust 1.66. Surely at some point we can bump the MSRV and simplify all of this?

I also tend to think it should be reasonable to bump the MSRV and use #[num_enum(catch_all)]

Awkwardly though we're currently constrained from bumping the rust-version in android-activity and the ndk crates for the sake of being compatible with the rust-version of the Winit crate which is aims to support Debian on Linux with a really old version of Rust.

Imho I find it frustrating / unreasonable that Android-specific crates like this would be constrained by Linux / Debian specific needs but hopefully Winit will stop testing Android builds against their MSRV soon: rust-windowing/winit#3046

Also ref: rust-mobile/android-activity#103