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 minimum rust version to 1.72 #8997

Merged
merged 8 commits into from
Feb 1, 2024
Merged

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jan 25, 2024

Which issue does this PR close?

Closes #8743

NOTE: Update to update to MSRV 1.72 (was 1.71)

Rationale for this change

  1. the new version of env_logger requires rust 1.71: Update env_logger requirement from 0.10 to 0.11 #8944
  2. Current MSRV of 1.70 which was released more than 6 months ago: https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1700-2023-06-01 (ages in Rust time)

MSRV 1.72 is still 5 months ago: in 2023-08-24 https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1720-2023-08-24

Most recent rust is 1.75: released end of Dec 2023
https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1750-2023-12-28

What changes are included in this PR?

Update Minimum rust version to 1.72

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

@github-actions github-actions bot added documentation Improvements or additions to documentation core Core DataFusion crate substrait labels Jan 25, 2024
@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions labels Jan 25, 2024
@@ -27,7 +27,7 @@ homepage = { workspace = true }
repository = { workspace = true }
license = { workspace = true }
authors = { workspace = true }
rust-version = "1.70"
rust-version = "1.71"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we take this opportunity to set this as same as workspace? e.g.

https://github.com/apache/arrow-datafusion/blob/7a5f2054305fd92852b589473afbc9bb034379d7/datafusion/physical-plan/Cargo.toml#L29

Unless its preferable to keep these separate? (even though they currently are the same)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an excellent idea. I vaguely (but perhaps incorrectly) remember something related to releasing was tricky with MSRV -- @andygrove do you remember? Would it be ok to switch the sub crates to use the workspace version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 9280b7a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I found out again. The reason the version is replicated is that cargo msrv verify doesn't support workspace versions

Thus if you do cd datafusion/core && cargo msrv verify, it fails like

Fetching index
Unable to find key 'package.rust-version' (or 'package.metadata.msrv') in '/Users/andrewlamb/Software/arrow-datafusion/Cargo.toml'

I have fixed that

Copy link
Contributor

Choose a reason for hiding this comment

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

Tracking issue for that in cargo msrv: foresterre/cargo-msrv#590

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Jefffrey -- I have added an issue ref as a comment so we don't forget it in the future

@ozankabak
Copy link
Contributor

Should we upgrade to 1.75 to avoid two upgrades? The latest version of one of our dependencies, ahash, requires 1.75 and we will hit problems sooner or later

@alamb
Copy link
Contributor Author

alamb commented Jan 31, 2024

Should we upgrade to 1.75 to avoid two upgrades? The latest version of one of our dependencies, ahash, requires 1.75 and we will hit problems sooner or later

Going up to the latest stable would be good from my perspective.

I think we should decide what our policy is and document it. @comphead 's recent change #9071 documents what the test is but not what the policy is (aka when we will update the value).

@Ted-Jiang also filed #8743

Here is my proposal -- I'll a "MSRV policy" ticket with proposal, and we can have a larger community discussion around that.

@alamb
Copy link
Contributor Author

alamb commented Jan 31, 2024

Ok, after some more consideration, I propose we change this PR to update to 1.72 to close @Ted-Jiang's #8743 and hopefully unblock ahash updates, and then we have the larger discussion about MSRV policy with the wider community via #9082

@alamb alamb changed the title Update minimum rust version to 1.71 Update minimum rust version to 1.72 Jan 31, 2024
let mut hasher = SEED.build_hasher();
other.hash(&mut hasher);
let o = hasher.finish();
let s = SEED.hash_one(self);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy failed on this for me with the updated MSRV so I did what it told me

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @alamb let me know if we need a review for MSRV policy or dscussion

@alamb
Copy link
Contributor Author

alamb commented Jan 31, 2024

lgtm thanks @alamb let me know if we need a review for MSRV policy or dscussion

Thanks @comphead -- let's have the MSRV policy discussion on #9082

Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

👍

@comphead comphead merged commit 1f70823 into apache:main Feb 1, 2024
23 checks passed
@alamb alamb deleted the alamb/update_msrv branch February 1, 2024 16:37
@Omega359
Copy link
Contributor

Omega359 commented Feb 5, 2024

@alamb You missed the cli Dockerfile unfortunately - it's still 1.70 and won't build without bumping up the version.

@alamb
Copy link
Contributor Author

alamb commented Feb 5, 2024

@alamb You missed the cli Dockerfile unfortunately - it's still 1.70 and won't build without bumping up the version.

Will fix.

Update: #9135

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate documentation Improvements or additions to documentation logical-expr Logical plan and expressions physical-expr Physical Expressions substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should update rust to 1.72
5 participants