-
Notifications
You must be signed in to change notification settings - Fork 110
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: Bump MSRV from 1.64 to 1.66 #431
Conversation
f90453c
to
a138446
Compare
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. |
a138446
to
f60b9b6
Compare
I knew you'd call that out 😬 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 |
f60b9b6
to
a4d9525
Compare
ndk/src/hardware_buffer_format.rs
Outdated
#[non_exhaustive] | ||
#[allow(non_camel_case_types)] | ||
pub enum HardwareBufferFormat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added non_exhaustive
here, but still on the fence whether that makes sense given that the Unknown
type is not opaque. There will be ambiguity when we add new variants in non-breaking releases (assuming we can even generate them non-breaking in ndk-sys
in the first place).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmmm, it's a bit thorny yeah.
It conceptually makes sense to be non_exhaustive to allow extension without a breaking release but the Unknown()
catch all kind of serves a similar purpose to the non_exhaustive
.
Imagining having an exhaustive match of all know formats something like:
match format {
Foo => { /* stuff */ },
// <snip> exhaustive list of all (currently) known format
Unknown(fmt) => { log::warn!("unknown format {fmt}") },
fmt => { panic!("really unknown format {fmt}") }
}
the #[non_exhaustive]
leads you to effectively have two catch all cases, and in this case a "non-breaking" release that would add a new format would suddenly cause the code above to panic.
It feels like maybe non_exhaustive isn't necessary but it could be considered semver compatible to extend the enum anyway because it doesn't break the API when the enum inherently has exhaustive coverage of all possible values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: your really unknown format
in reality is ndk crate mapped a new format that we don't match on yet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But anyway, having the two cases is effectively why one might want to make Unknown
as opaque so that it becomes impossible to match on... Or to have Unknown(u32)
without non_exhaustive
so that additions (that might have previously ended up in Unknown(u32)
next to actual private/vendor values) will fail to compile rather than causing ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it again, so that we don't have this duplicate ambiguity. Going forward we'll either (== mutually exclusive) have:
#[num_enum(catch_all)] Unknown(variant)
publicly available;#[non_exhautive]
, and no publicly available catch-all (or a non-opaque one);- Neither of the two, mainly for "simpler"
enum
s that are unlikely to be extended and only used as function argument (i.e. user just needs to pass us a value, no need tomatch
on it).
(Added this point mainly because we have a lot of these already, but I'm still leaning towards 2. for them.)
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. |
a4d9525
to
02a8d56
Compare
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. 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 `enum`s 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.
02a8d56
to
8d0648d
Compare
It is older than 6 months and now required for the
toml_edit
dependency (used bynum_enum_derive
). At the same timenum_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 manualmatch
blocks when implementing conversions forHardwareBufferFormat
when also having anUnknown(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 acatch_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
viaTryFromPrimitive
is desired to provoke upstream issue reports and quick turnaround on new values. Same forenum
s that are used to pass values into functions: it is desired to only pass known values (by thisndk
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.