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

Update MSRV for better compatibility #14

Merged
merged 5 commits into from
Nov 17, 2024

Conversation

siblingsofthevoid
Copy link
Contributor

This PR fixes #12 in preparation for #13, which required a few updates to Cargo.toml and allowed to remove the rust-toolchain file, since rust-version is now part of Cargo.toml.

This PR also fixes the clippy issues that popped up along the way.
To properly update, some dependencies may need some additional work due to API changes (e.g. cookie), but I'll gladly do a full dependency update/upgrade pass in another PR, so that this one can stay smaller.

I expected this to be optional, but when testing without these changes, cargo ended up complaining about several dependencies needed for #13 seemingly not being available for Rust 1.56.

fixes #12

@siblingsofthevoid siblingsofthevoid changed the title Update MSRV and dependencies Update MSRV for better compatibility Sep 30, 2024
@edward-shen
Copy link
Owner

edward-shen commented Nov 8, 2024

Hey, thanks for the work and sorry for the late reply --- I've been on vacation and just got some time to take a look at things.

For some context, the reason I pinned things to 4.0.0 and Rust 1.59 is to ensure that we have maximal compatibility so long as the world permits it. It looks like the world no longer permits it, so it's time to update.

Going one level deeper, the reason why I cared about maximal going so far back in support is because this is a security crate and I'd want to cover as many folks as possible, even those that have a glacially slow update cycle for enterprise ™️ reasons. That's my line of thought, but it's not set in stone. I can't tell if this is helpful or not, so I'm happy to be convinced otherwise.

Assuming no changes on that mindset, I would have liked to see the minimal rust toolchain to 1.72 and do the following:

[dependencies]
actix-web = "4.3.1"

[dev-dependencies]
actix-web = "=4.3.1"
actix-http = "=3.3.1"

These versions of actix-web, actix-http, and rust-toolchain seem to work fine with ahash 0.8. But if I'm convinced otherwise (I'd like to hear your thoughts!), your changes are then fine.

Whether or not I'm convinced to remove all the pinning, if you're going to make changes to the msrv, please also update the .github/workflows/ci.yml file to reflect your changes, and to add to the CHANGELOG.md, creating a 0.9 section. The rest looks solid otherwise!

@siblingsofthevoid
Copy link
Contributor Author

I didn't have a specific reason to use 1.81 specifically, so I'll go have a test and see if it works together with actix-web and actix-multipart. I just looked at actix's MSRV and it
seems to be 1.72 as written here, so 1.72 seems fine to me.

Beyond that I'll get to work on the changes in a bit.

@siblingsofthevoid
Copy link
Contributor Author

Sorry for the late fix, i had a few urgent personal things get in the way
This should be the changes as requested, seems there are no issues, so It's all good from my side ^^

@edward-shen edward-shen merged commit 800b36a into edward-shen:master Nov 17, 2024
1 check passed
@edward-shen
Copy link
Owner

Thanks for your contributions!

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

Successfully merging this pull request may close these issues.

MSRV should be updated
2 participants