search

rust-mobile / ndk

Rust bindings to the Android NDK
Apache License 2.0
1.11k stars 110 forks source link

ndk: Bump MSRV from 1.64 to 1.66 #431

Closed MarijnS95 closed 11 months ago

MarijnS95 commented 11 months ago

It is older than 6 months and now required for the toml_edit dependency (used by num_enum_derive). At the same time num_enum relies on arbitrary enum discriminants (for discriminated enums with tuple/struct variants) introduced by Rust 1.66 in order to implement #[num_enum(catch_all)]. This feature comes in use to replace the manual match blocks when implementing conversions for HardwareBufferFormat when also having an Unknown(u32) to catch valid private/vendor values that won't ever be described inside the NDK. This change effectively reverts #407 to its initial state, where a catch_all implementation was used. CC @spencercw.

For the latter the intent is however to use this feature sparingly. In most APIs new values are few and far between, so treating these as an Err via TryFromPrimitive is desired to provoke upstream issue reports and quick turnaround on new values. Same for enums that are used to pass values into functions: it is desired to only pass known values (by this ndk crate) into those, anything else should similarly be reported and added upstream. In these cases a #[non_exhaustive] allows us to do so with a tiny non-breaking patch release.

rib commented 11 months ago

Just for reference, I'm still expecting to bump the rust-version for android-activity to 1.68 considering that 1.68 bumped what NDK version is used to build the standard library and considering that I depend on cargo-ndk across multiple projects, including android-activity (which effectively has an MSRV of 1.68).

It should be fine having a rust-version of 1.66 here if that's enough for what you need atm, but in case there's anything you'd consider taking advantage of in 1.68 it's maybe worth considering jumping ahead a little further. 1.68 is also > 6 months old.

MarijnS95 commented 11 months ago

I knew you'd call that out :grimacing: and no, no plans/needs for 1.68 currently. Users can figure that out themselves, there's no need to force MSRV to 1.68 to get them to take advantage of the latest cargo-ndk.

rib commented 11 months ago

I knew you'd call that out

and I know you knew I knew you knew I'd say something ;-p

yeah, I was really just clarifying that I'm still at least planning to bump the android-activity crate to 1.68 - but you probably already assumed that.

I don't really see a pressing need for this crate to bump past 1.66 - except that all things being equal I'd pick 1.68 for the sake of aligning with the rust release that bumped what ndk version is used for building the standard library.