Skip to content
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

Bumping MSRV of getrandom 0.2 #286

Closed
josephlr opened this issue Oct 6, 2022 · 10 comments
Closed

Bumping MSRV of getrandom 0.2 #286

josephlr opened this issue Oct 6, 2022 · 10 comments
Assignees

Comments

@josephlr
Copy link
Member

josephlr commented Oct 6, 2022

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:

  • MaybeUninit: added in 1.36 (Released 2019-07-04)
  • <*mut T>::cast: added in 1.38 (Released 2019-09-26)
  • Const Generics: Added in 1.51 (Released 2021-03-25)

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
Copy link

notgull commented Oct 6, 2022

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
Copy link
Contributor

<*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
Copy link
Contributor

briansmith commented Oct 14, 2022

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
Copy link
Member

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
Copy link
Contributor

@briansmith
Copy link
Contributor

@newpavlov
Copy link
Member

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
Copy link
Member Author

josephlr commented Oct 22, 2022

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
Copy link
Member Author

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants