rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
273 stars 178 forks source link

Bumping MSRV of getrandom 0.2 #286

Closed josephlr closed 1 year ago

josephlr commented 1 year ago

A few issues and PRs (#279 #281) have brought up the idea of increasing the MSRV of this crate. Currently the MSRV is 1.34, and I've seen 3 features we might want to use which are only present in later versions:

When raising the MSRV, we should keep in mind that rand 0.8 and rand_core 0.6 have an MSRV of 1.36, and they are our largest users. Other top crates that directly depend on us include:

Also, we can always use rustversion to have APIs that require a newer version of Rust without increasing our MSRV, provided we can implement the newer API in terms of one of the older APIs.

I would propose: raising the MSRV to 1.36 so we can use MaybeUninit inside of our crate.

For example,

pub fn getrandom_uninit(buf: &mut [MaybeUninit<u8>]) -> Result<&[u8], Error> {
  // Unconditionally use MaybeUninit
  // ...
}

#[rustversion::since(1.51)]
#[inline]
pub fn getrandom_array<const N: usize>() -> Result<[u8; N], Error> {
  let mut arr: [MaybeUninit<u8>; N] = [MaybeUninit;;uninit(); N];
  let s: &mut [u8; N] = getrandom_uninit(&mut arr)?.try_into().unwrap();
  Ok(*s)
}
notgull commented 1 year ago

I think getrandom_array is a superfluous addition to this crate that is out of its scope. It should belong in crates like rand.

Personally, I don't mind a bump from 1.34 to 1.36. It's only two versions, and they were released long enough ago that it shouldn't really matter. However, I would mind a bump to 1.51. I maintain some crates that aim to target the current distro version of Debian Stable, which is 1.48. If getrandom bumps to 1.51 it would no longer build here.

This is a pretty hotly contended issue in the Rust community, see matklad/once_cell#201

(async-io has an MSRV of 1.46, but it shouldn't matter since it's only a testing dependency anyhow).

briansmith commented 1 year ago

<*mut T>::cast: added in 1.38 (Released 2019-09-26)

This feature is trivial to polyfill, IIUC, so it's not very compelling to bump MSRV for it. Though, I wouldn't object to it.

I would propose: raising the MSRV to 1.36 so we can use MaybeUninit inside of our crate.

I think that's extremely uncontroversial given the data you looked up, especially regarding rand's current MSRV. I suggest bumping MSRV to 1.36 for now so the MaybeUninit stuff can move forward, and decide separately whether to bump it further. In particular, I don't think the decision of "1.36" vs "newer" should block the MaybeUninit stuff. It would probably be smart to get the MaybeUninit stuff landed and shipped in a release before bumping the MSRV further for other features.

The CI/CD configuration and ensuring tests cover all the different code paths gets really messy really fast when there are optional features, so I would recommend avoiding the use of optional features to try to allow people to opt into newer language features like const generics.

briansmith commented 1 year ago

If we make the MSRV 1.37 or higher, instead of 1.36, then we can use MaybeUninit<u8> in FFI type declarations, which would allow us to remove several as *mut u8 casts from the code (possibly to be) added in PR #291; see https://github.com/rust-lang/rust/blob/master/RELEASES.md#libraries-27.

newpavlov commented 1 year ago

If we make the MSRV 1.37 or higher, instead of 1.36, then we can use MaybeUninit in FFI type declarations

I don't think there is enough merit in it, the casts are trivially safe, having *mut MaybeUninit<u8> in FFI signatures of external functions would be somewhat surprising, and it would affect rand's MSRV.

briansmith commented 1 year ago
briansmith commented 1 year ago
newpavlov commented 1 year ago

I don't think that target support influences crate's MSRV. You obviously can not build crate for a target with a toolchain which does not anything about it. We already have several targets which were introduced after Rust 1.34 was released.

josephlr commented 1 year ago

I don't think that target support influences crate's MSRV. You obviously can not build crate for a target with a toolchain which does not anything about it. We already have several targets which were introduced after Rust 1.34 was released.

I think @briansmith just meant that we could shorten the code in #303 by using

#[cfg(all(target_family = "wasm", target_os = "unknown"))]

instead of

#[cfg(all(any(target_arch = "wasm32", target_arch = "wasm64"), target_os = "unknown"))]

but I agree, we can live without it.

josephlr commented 1 year ago

With #291 and #305 increasing the MSRV to 1.36, and the fact that rand 0.8 has an MSRV of 1.36 (making it harder to raise the MSRV further), can we close this issue?

briansmith commented 1 year ago

I don't object to closing the issue, as I don't think increasing the MSRV even requires an issue. :)