Skip to content

Bump MSRV to 1.78 - #12398

Merged
comphead merged 4 commits into
apache:mainfrom
comphead:dev1
Sep 17, 2024
Merged

Bump MSRV to 1.78#12398
comphead merged 4 commits into
apache:mainfrom
comphead:dev1

Conversation

@comphead

@compheadcomphead commented Sep 9, 2024

Copy link
Copy Markdown
Contributor

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-actionsgithub-actionsBot added development-process Related to development process of DataFusion core Core DataFusion crate substrait Changes to the substrait crate proto Related to proto crate labels Sep 9, 2024
@alamb

alamb commented Sep 9, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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

alamb commented Sep 9, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

should we proceed with this PR?

@alamb

Copy link
Copy Markdown
Contributor

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

@findepifindepi left a comment

Copy link
Copy Markdown
Member

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

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
Contributor

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

@alamb

Copy link
Copy Markdown
Contributor

I merged up from main to resolve a conflict

@comphead

Copy link
Copy Markdown
ContributorAuthor

Thanks I'll merge once CI is done

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api changeChanges the API exposed to users of the cratecoreCore DataFusion cratedevelopment-processRelated to development process of DataFusionprotoRelated to proto cratesubstraitChanges to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@comphead@alamb@findepi