Release planning

Question

Release planning

rib opened this issue a year ago · 30 comments

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.

Answer 1 · 2023-08-08T22:10:59.000Z

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

Answer 2 · 2023-08-08T22:12:15.000Z

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.

Answer 3 · 2023-08-09T20:23:25.000Z

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.

Answer 4 · 2023-08-09T21:41:12.000Z

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.

Answer 5 · 2023-08-09T22:09:48.000Z

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.

Answer 6 · 2023-08-09T22:29:04.000Z

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.

Answer 7 · 2023-08-10T09:16:15.000Z

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.

Answer 8 · 2023-08-10T13:40:45.000Z

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.

Answer 9 · 2023-08-10T15:00:25.000Z

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.

Answer 10 · 2023-08-26T20:17:18.000Z

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 😬

Answer 11 · 2023-08-27T10:13:55.000Z

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.

Answer 12 · 2023-08-27T11:46:44.000Z

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.

Answer 13 · 2023-08-31T10:19:55.000Z

@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?

Answer 14 · 2023-09-02T11:19:46.000Z

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?

Answer 15 · 2023-09-06T00:12:41.000Z

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

Answer 16 · 2023-09-06T00:20:27.000Z

As expressed before num_enum would translate these to match statements anyway because enum discriminants can AFAIK not deal with catch-all's: what would its discriminant value be, and what if we have a non_exhaustive enum that would at some point treat that value as a valid variant?

It seems num_enum just needs the discriminant values to be able to generate match tables, after which they can "just be gone" before being passed to rustc?

I'd love to be proven wrong here.

@rib this isn't the place to take a jibe at winit for their MSRV policy.

Answer 17 · 2023-09-06T00:27:05.000Z

@rib this isn't the place to take a jibe at winit for their MSRV policy.

In so far as providing context for why we're currently unable to bump the msrv >= 1.66 it was relevant

Answer 18 · 2023-09-06T00:38:38.000Z

As expressed before num_enum would translate these to match statements anyway because enum discriminants can AFAIK not deal with catch-all's: what would its discriminant value be, and what if we have a non_exhaustive enum that would at some point treat that value as a valid variant?

I'm not sure I follow what the concern is here.

It sounds OK that num_enum could translate these to match statements, it just saves having to write the matching manually?

Answer 19 · 2023-09-06T00:57:47.000Z

In the short term it seems ok to stick with the hand written match and anticipate using num_enum to simplify later.

Generally having enums like:

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

sounds like the right way to go.

Code needs to generally be able to gracefully / safely handle the possibility that it is running on a newer version of Android than was targeted at compile time and since extended values likely don't represent error conditions then a catch all Unknown(u32) seems appropriate.

Answer 20 · 2023-09-06T11:26:15.000Z

I'm not sure I follow what the concern is here.

It sounds OK that num_enum could translate these to match statements, it just saves having to write the matching manually?

It does translate to match statements, but the original enum discriminants are left in place which bump the MSRV to 1.66 for having them on an enum with non-unit variant per the RFC.

Unfortunately that RFC was only cooked up with FFI in mind. We're not passing these enums over an FFI boundary, ~~so those discriminants are useless to us (only needed to drive num_enum's derive macro)~~ EDIT: See below, num_enum makes use of these discriminants for efficiency.

For example, when Unknown(u32) exists we can no longer write SomeConstant::X as u32 to cheaply extract the discriminant, and have to use casts based on the defined layout: https://doc.rust-lang.org/std/mem/fn.discriminant.html#accessing-the-numeric-value-of-the-discriminant

Turns out num_enum uses exactly this to access the discriminant, unless the variant was the catch_all: https://docs.rs/num_enum_derive/0.7.0/src/num_enum_derive/lib.rs.html#45-48 (and the standard as cast for exhaustive enums).

Code needs to generally be able to gracefully / safely handle the possibility that it is running on a newer version of Android than was targeted at compile time and since extended values likely don't represent error conditions then a catch all Unknown(u32) seems appropriate.

There was no debate whether or not unknown-to-the-ndk values should be treated as unrecoverable errors. Rather, the discussion has all this time been about how to represent them. Since catch_all with arbitrary enum discriminants requires an MSRV bump or the manual match statements we have now, my preference went out to having a Result-like Either type (tailored to HardwareBufferFormat) so that we could keep using num_enum.

However, I've just brought up the same discussion over at drm-rs, and the ndk crate seems to be taking the same lackluster approach to "invalid" enum variants. For example, the event code converts unknown Keycode/Source variants to the Unknown variant, which already has an explicit value within Android's NDK, making the caller unable to distinguish this variant from "Android really returned 0" vs "the NDK crate doesn't map this value yet and turned >0 into what appears to be 0". In other words, our use of TryFromPrimitive+IntoPrimitive is not invariant.

The problem with Unknown(u32) however that has been raised (over at drm-rs) is that there is now the possibility for ambiguity. When we release a crate upgrade that turns a previously Unknown(3) into ThirdVariant = 3, in the bindings, someone may be relying on that (matching on Unknown(3)) in their match statement and get unexpected behaviour. On the other hand this is most likely only an issue with #[non_exhaustive] where the downstream crate is forced to have a catch-all match arm (or they added the catch-all arm for other reasons). Otherwise rustc will simply tell them to add the new ThirdVariant to their match statement.

Answer 21 · 2023-09-09T15:27:44.000Z

Not to take away from the discussion on how to best represent enums, but toml_edit just bumped its MSRV to 1.66 and is breaking our CI. After just reading https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html I'm slowly leaning towards committing them as well, as long as there's an automated renovatebot setup that does automated "lockfile maintenance" every week.

Answer 22 · 2023-09-10T11:44:22.000Z

For reference Winit is now testing the Android backend with 1.69 since merging rust-windowing/winit#3046 so I think we can revisit bumping the rust-version for android-activity and the ndk[-sys] crates.

I think bumping to at least 1.68 makes a good deal of sense for all Android crates.

Answer 23 · 2023-09-12T08:49:41.000Z

@rib I don't just blindly want to bump the MSRV "because Winit now allows it", for lack of a written-down policy of our own. On the other hand I also don't want to delay Rust updates too harshly, when it quickly becomes obvious that there are useful features in newer releases.

For the time being 1.66 perfectly suits our needs: toml_edit, enum discriminants, OwnedFd and friends via std::os::fd. Maybe you need more on the android-activity side though?

Regarding the enums above, I'm still unhappy that #[num_enum(catch_all)] will turn our enums into a "fat" enum (one field for the discriminant, one field for the same discriminant in Unknown(u32) fashion), but there's no way around it other than embracing Result which I will still maintain in a few areas.

As discussed over at drm-rs there seemed to be a preference to treat unknown values as opaque (i.e. not visible to the caller) on a #[non_exhaustive] enum, so that non-breaking crate updates can add new variants (and missing variants are more likely reported by users), and to prevent callers from using and relying on hardcoded/unknown constants.

For most cases (such as DataSpace which I'm mapping now) such an approach suffices, while still giving the caller access to unknown values via TryFromEnumError<Enum>. For HardwareBufferFormat, which can have private / vendor-specific values), an Unknown(u32) via num_enum(catch_all) is probably the most desirable approach over hand-writing the match statements and cfg guards.

Answer 24 · 2023-09-13T21:27:17.000Z

I have now bumped the MSRV for the ndk crate to 1.66 in #431. With that I've also reverted @spencercw's #407 PR back to its inception where num_enum(catch_all) was used. For this type it makes sense because unknown variants are "common" / expected.

One other thought/offer: since bindgen already generates newtyped enums, we could also export/re-expose those instead of re-wrapping them in enums. As shown in #407 (comment) the caller can still match on those and create their own invalid values. At the same time it doesn't go out of sync when we regenerate ndk-sys. We can't add documentation nor cfg statements nor impl functions (nor a custom pretty-print debug), however.

Answer 25 · 2023-09-22T18:29:58.000Z

I have mostly all the stuff resolved for the winit, so it'd be nice for android stuff to get released/updated for the next release, since it won't be a beta.

Answer 26 · 2023-09-22T18:38:10.000Z

@kchibisov I figured this would get asked right as I'm 2 days into a trip and not near a computer for some time.

Answer 27 · 2023-09-22T18:47:51.000Z

@MarijnS95 unfortunate, but no rush. Wanted to say that there's a desire in winit to do it in the upcoming weeks.

Answer 28 · 2023-09-22T18:50:44.000Z

@kchibisov perfect, if you have a bit less than 2 weeks I'll get everything in order as soon as I return.

Answer 29 · 2023-09-22T18:53:43.000Z

Yeah, don't worry.

Answer 30 · 2023-10-07T21:43:24.000Z

I have just opened PRs for all the planned breaking changes that were still on my mind, and will merge them in (assuming there's very likely no objections) before pushing out the actual release. Apologies for the slowdown/inconvenience.

However, as mentioned in rust-windowing/winit#2686 (comment) the raw-window-handle 0.6 release should trickle down into this breaking ndk release as well: #434 (assuming winit will want to have it for 0.29).