-
Notifications
You must be signed in to change notification settings - Fork 821
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 MSRVs to be accurate #6742
base: main
Are you sure you want to change the base?
Conversation
… few and purposefully break arrow-flight msrv
@@ -122,24 +122,20 @@ jobs: | |||
uses: ./.github/actions/setup-builder | |||
- name: Install cargo-msrv | |||
run: cargo install cargo-msrv | |||
- name: Downgrade arrow dependencies | |||
run: cargo update -p ahash --precise 0.8.7 |
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.
ahash
's MSRV now is 1.60, so we don't need this downgrade anymore.
The MSRV check failed, and although the output was too long to see what exactly the error was, I'm going to update the purposefully-broken package's MSRV to see if things pass now. |
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.
Thank you so much @itsjunetime for cleaning this up 🙏
The basic pattern looks very promising to me
The CI is currently failing https://github.com/apache/arrow-rs/actions/runs/11876059759/job/33093978528?pr=6742
I ran the check locally
cargo msrv verify --manifest-path object_store/Cargo.toml
And the actual appears to be this
│ error: package `tokio v1.39.1` cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.64.0 │
- name: Check object_store | ||
working-directory: object_store | ||
run: cargo msrv --log-target stdout verify | ||
- name: Check all packages |
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.
This is awesome ❤️
.github/workflows/rust.yml
Outdated
echo "Checking package '$dir'" | ||
cargo msrv verify --manifest-path "$dir" || exit 1 | ||
done | ||
# If no packages are using the workspace's rust-version, then it's out of date |
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.
Given that cargo msrv
can't currently check a crate that uses the workspace MSRV, maybe we should just remove the workspace MSRV in favor of explicit versions in all the crates 🤔
I took the liberty of pushing some commits to this branch to get this PR unstuck and hopefully ready to merge
|
object_store/Cargo.toml
Outdated
@@ -24,7 +24,7 @@ readme = "README.md" | |||
description = "A generic object store interface for uniformly interacting with AWS S3, Google Cloud Storage, Azure Blob Storage and local files." | |||
keywords = ["object", "storage", "cloud"] | |||
repository = "https://github.com/apache/arrow-rs/tree/master/object_store" | |||
rust-version = "1.64.0" | |||
rust-version = "1.70.0" |
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.
I'm surprised by this given we had CI checking this previously?
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.
I double checked, and indeed you are right that cargo msrv
still passes with 1.64 (but it requires downgrading tokio)
Not sure what is going on. Pushed a change to revert
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.
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.
I agree -- I think it was my mistake to push it to 1.64. Let's put it back to 1.62.1
I struggle to approve this as a number of these crates were being tested with MSRV and were passing (e.g. arrow-flight)... Am I missing something? |
Yes - everything was passing before, but it simply wasn't testing every crate in this repo and we also had multiple MSRVs set too high (which msrv testing wouldn't catch, since we don't try to verify that the MSRV is set as low as it can go) For example, In the same vein, the Besides these sorts of things, the other main change is updating the workspace's MSRV to 1.70 (from 1.62) since it had just fallen out-of-date with what the minimum MSRV is within the workspace. Since the workspace MSRV is now 1.70, all the crates that previously had their MSRV explicitly set to This PR fixes the above issues by:
|
I wonder if we should even have per-crate MSRVs or rather opt for a workspace-wide one. To me, the different ranges are close to impossible to keep track off. |
I think it would make sense for a single MSRV for arrow and a single MSRV for object_store (likely the same) |
Which issue does this PR close?
Closes #6741
What changes are included in this PR?
This changes MSRVs of a lot of crates in this repo to accurately reflect their actual MSRV, as they've fallen out of date.
There's an MSRV check in CI, but it currently only checks a few specific crates instead of all of them. It also doesn't have any way to tell if the workspace's
rust-version
field has fallen out of date due to all other crates in the workspace using a specifically different MSRV. This fixes both of those issues by checking everyCargo.toml
that exists in this repo and also making sure at least one of them usesrust-version = { workspace = true }
.The MSRVs that these were changed to were all found by running
cargo msrv find --manifest-path $cargo_toml_path
for eachCargo.toml
in the repo. If it failed due to a dependency that we could downgrade, I downgraded it as low as ourCargo.toml
allowed it to go to see if that would allow us to get the MSRV lower.Rationale for this change
The repo is easier to use and manage if this task is automated
Also, at time of writing, the
arrow-flight/gen
crate has a broken MSRV. This is to ensure the CI checks that I added actually work. I'll change it back once CI has verified it's bad.Are there any user-facing changes?
No; these crates already wouldn't compile with their current (incorrect) MSRVs so we aren't affecting users at all with these changes.