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

Bump MSRV to 1.78 #12398

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Bump MSRV to 1.78 #12398

merged 4 commits into from
Sep 17, 2024

Conversation

comphead
Copy link
Contributor

@comphead comphead commented Sep 9, 2024

Which issue does this PR close?

According to https://releases.rs the 1.81 has been released recently

By policy we should have been migrating to 1.77 but this release had too many hotfixes, so it probably safer to go with
1.78 Rust compiler that has been released on May 2

Closes #.

Depends on #12402

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added development-process Related to development process of DataFusion core Core DataFusion crate substrait proto Related to proto crate labels Sep 9, 2024
@alamb
Copy link
Contributor

alamb commented Sep 9, 2024

1.78 was released on 2 May, 2024 it seems: https://releases.rs/docs/1.78.0/

that means to follow our policy of 6 months we need to wait for Nov 2, 2024 I think 🤔

I personally would be fine with a less stringent policy on MSRV (e.g. 3 months or 3 major rust versions or something) but as written I think this PR doesn't follow the policy

@comphead
Copy link
Contributor Author

comphead commented Sep 9, 2024

1.78 was released on 2 May, 2024 it seems: https://releases.rs/docs/1.78.0/

that means to follow our policy of 6 months we need to wait for Nov 2, 2024 I think 🤔

I personally would be fine with a less stringent policy on MSRV (e.g. 3 months or 3 major rust versions or something) but as written I think this PR doesn't follow the policy

Thanks @alamb the DF MSRV policy perhaps should be revised. it reads as 6 months which supports 3-4 most recent versions.

I feel now the Rust release cadences are shorter so most recent 4 versions, (1.78, 1.79, 1.80, 1.81) are fit into 4 months.

May be want to rephrase the policy saying supporting last 4 versions or 4 months whatever is longer?

@alamb
Copy link
Contributor

alamb commented Sep 9, 2024

May be want to rephrase the policy saying supporting last 4 versions or 4 months whatever is longer?

Sounds very reasonable to me

I suggest we propose that as a PR and make sure everyone is cool with it. (like we can post a note to the dev email list and slack)

It might be good to wait for the 42.0.0 (#11902) release to out before making the change too so we have some bake time

@comphead
Copy link
Contributor Author

comphead commented Sep 9, 2024

May be want to rephrase the policy saying supporting last 4 versions or 4 months whatever is longer?

Sounds very reasonable to me

I suggest we propose that as a PR and make sure everyone is cool with it. (like we can post a note to the dev email list and slack)

It might be good to wait for the 42.0.0 (#11902) release to out before making the change too so we have some bake time

Refined the policy #12402

@comphead
Copy link
Contributor Author

should we proceed with this PR?

@alamb
Copy link
Contributor

alamb commented Sep 12, 2024

I recommend waiting until tomorrow (after 42 release is cut - #11902) and then merging it in unless anyone objects

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

thanks for doing this

it would be nice if the MSRV was defined in one place

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.

Thanks @comphead -- I think this looks good to me and we have left it open enough for a discussion from my perspective. Let's do it

@alamb alamb changed the title minor: Bump MSRV to 1.78 Bump MSRV to 1.78 Sep 15, 2024
@alamb alamb added the api change Changes the API exposed to users of the crate label Sep 15, 2024
@alamb
Copy link
Contributor

alamb commented Sep 15, 2024

Marking PR as api change to get it a little more attention in the release notes

@alamb
Copy link
Contributor

alamb commented Sep 17, 2024

I merged up from main to resolve a conflict

@comphead
Copy link
Contributor Author

Thanks I'll merge once CI is done

@comphead comphead merged commit c7b38c8 into apache:main Sep 17, 2024
26 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate development-process Related to development process of DataFusion proto Related to proto crate substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants