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

What is the MSRV? #571

Open
atouchet opened this issue Oct 7, 2024 · 9 comments
Open

What is the MSRV? #571

atouchet opened this issue Oct 7, 2024 · 9 comments

Comments

@atouchet
Copy link
Contributor

atouchet commented Oct 7, 2024

I was looking at this repo and noticed that the minimum Rust version tested in CI is 1.74, the minimum version listed in Cargo.toml is 1.65, and the minimum version listed in the Readme and changelog is 1.63. Is there a reason for these discrepancies?

@clarfonthey
Copy link
Contributor

clarfonthey commented Oct 8, 2024

Cargo.toml and CHANGELOG.md conflicting is due to the fact that MSRV will be bumped on the next release, but said release hasn't been published yet.

As for CI, it appears that the issue is that the MSRV of the various dependencies, specifically allocator-api2, is actually higher than the MSRV needed for hashbrown itself to compile. I guess that perhaps this should be fixed; cc @Amanieu. I would assume that the one listed in CI is the most accurate.

Will poke around to see if Cargo actually has any open issues for linting this, since it feels like there should be a warning for having an MSRV lower than your dependencies. It's likely this will be fixed once the MSRV-aware resolver is stable.

@cuviper
Copy link
Member

cuviper commented Oct 9, 2024

I've added CI checks in #572.

@Amanieu
Copy link
Member

Amanieu commented Oct 10, 2024

We're likely to release 0.16 soon to deal with #570, so this is a good opportunity to significantly bump the MSRV. I don't have any preference for a particular version, but @cuviper might.

@clarfonthey
Copy link
Contributor

I know you expressed that you were okay bumping up all the way to 1.80.0, and at least when I was working on refactoring the code, I noticed that NullPtr::add was stabilised in… 1.80.0. So, that's one note in favour of that.

@cuviper
Copy link
Member

cuviper commented Oct 10, 2024

My MSRV preference is probably more conservative than most -- I usually advocate for at least a year, which right now would be Rust 1.73 (2023-10-05). A little tighter than that could be 1.75.0 (2023-12-28), which is significant as the current Rust toolchain in RHEL 8 and 9, and also Ubuntu 20.04, 22.04, and 24.04. Debian stable still has 1.63, but they do have rustc-web now on 1.78.

In general, I don't think it should be bumped for no reason, and the example of NonNull::add seems like a weak motivation. Stuff like additional const power would be more motivating to me.

@clarfonthey
Copy link
Contributor

clarfonthey commented Oct 10, 2024

Right, I mostly mentioned it to point out that there are things that could be directly useful even in the previous stable version and so keeping us back further will disrupt that. A lot of them in particular will directly affect the kinds of unsafe code that are used in the library and how we interact with them.

But obviously, yes, that one example could be easily to write manually, and it's not a massive motivating factor.

I think that setting one year back is a good starting point though, and we can talk more about it as things come up. We're not even using all the features the current, much older MSRV offers, so, that's worth fixing.

@tnull
Copy link

tnull commented Nov 5, 2024

While I opened a separate issue for bumping the MSRV on a patch release over at #585 (which breaks our builds with ~no options left as dependencies had already upgraded to the now-yanked 0.15.0), I want to voice our MSRV requirements as hasbrown users (directly as well as through multiple dependencies that are out-of-our control):

We want and have to support Debian Bookworm, i.e., enforce an MSRV policy of 1.63.0 for now.

Given that the most-recent bump also violates the MSRV policies of other projects (e.g., indexmap who also advertise 1.63.0), it would be great if the bump could be reconsidered, or at least would happen as part of a new minor release, not a patch release as it seems was originally planned over at #570 (comment)

@cuviper
Copy link
Member

cuviper commented Nov 5, 2024

As the indexmap maintainer -- it's a little unfortunate to have its MSRV forced up indirectly, but I also don't expect to keep supporting old versions forever. As I said above, my usual rule of thumb is to support at least a year old, and even Rust 1.65 is now two years old. When building for Debian 12, you can still lock it back to indexmap v2.5.0 which uses hashbrown v0.14, as long as you refrain from using any new features.

(Please don't publish to crates.io with a max-limited dependency though!)

@s-arash
Copy link

s-arash commented Nov 11, 2024

FWIW, I've opened a similar issue on allocator-api2: zakarumych/allocator-api2#20

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

6 participants