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 MSRVs to be accurate #6742

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

itsjunetime
Copy link
Contributor

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 every Cargo.toml that exists in this repo and also making sure at least one of them uses rust-version = { workspace = true }.

The MSRVs that these were changed to were all found by running cargo msrv find --manifest-path $cargo_toml_path for each Cargo.toml in the repo. If it failed due to a dependency that we could downgrade, I downgraded it as low as our Cargo.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.

@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate object-store Object Store Interface labels Nov 17, 2024
@@ -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
Copy link
Contributor Author

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.

@itsjunetime
Copy link
Contributor Author

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.

Copy link
Contributor

@alamb alamb left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome ❤️

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

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 🤔

@alamb alamb added development-process Related to development process of arrow-rs and removed development-process Related to development process of arrow-rs labels Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate object-store Object Store Interface parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some MSRVs are inaccurate
2 participants