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

Adopt a MSRV policy #181

Open
alamb opened this issue Apr 26, 2021 · 9 comments
Open

Adopt a MSRV policy #181

alamb opened this issue Apr 26, 2021 · 9 comments
Labels
arrow Changes to the arrow crate

Comments

@alamb
Copy link
Contributor

alamb commented Apr 26, 2021

Note: migrated from original JIRA: https://issues.apache.org/jira/browse/ARROW-11605

With all our crates now supporting stable Rust, we can decide on a Minimum Supported Rust Version, so that we don't introduce breakage to people relying on older Rust versions.

We could:

  • Determine what the earliest Rust version that compiles is (at least 1.39 due to async in DF)
  • Use this version in CI
  • Decide on, and document, a policy for how we update versions

This might mean that when there's fresh new changes landing in Stable, we'd likely hold off on them until those changes meet our MSRV.

Thoughts [~Dandandan] [~alamb] [~jorgecarleitao] [~andygrove] [~paddyhoran] [~sunchao]?

@alamb alamb added the arrow Changes to the arrow crate label Apr 26, 2021
@alamb
Copy link
Contributor Author

alamb commented Apr 26, 2021

Comment from Andrew Lamb(alamb) @ 2021-02-18T10:40:55.550+0000:

[~nevi_me] I don't have any well formed thoughts here -- it sounds like a reasonable idea to me. 

@dbr
Copy link
Contributor

dbr commented Jul 19, 2021

This would be nice to have - for example from arrow 4.2 to 4.3 the required Rust version jumped from 1.51 to 1.52 (as noted in #456), but wasn't mentioned in release notes, and was a little surprising in a point release

@alamb
Copy link
Contributor Author

alamb commented Jul 19, 2021

Thanks @dbr -- what would work best for you? Only changing the required rust version on major updates?

@dbr
Copy link
Contributor

dbr commented Jul 21, 2021

Practically, as long the requirement changes are noted in the CHANGELOG.md, I'd be happy!

The main inconvenience with the 4.3 release to me was just I wasn't sure what had happened - I ran cargo update and got a strange error, and the changelog didn't mention anything significant. Had I not found #456 it would have taken a bit of poking around to realize exactly what happened

I think it makes more logical sense for minimum-Rust-version changes to be major bumps (since the change makes previously compiling code start erroring, practically the same as any other breaking change), but I know there is a lot of debate around this and I don't have any especially strong opinions on it!

@alamb
Copy link
Contributor Author

alamb commented Jul 22, 2021

@dbr -- got it -- as it turns out I was somewhat surprised by the min change too (I didn't realize that something we backported used a new API in rust 1.53).

In order to properly implement such a version policy, we would probably need to pin the rust version used in the arrow-rs crate's CI system (otherwise we effectively get the latest rust whenever it is released, and we often scramble to fix new clippy lints and what not)

Feel free to submit a PR if you have a moment -- maybe using something like https://github.com/influxdata/influxdb_iox/blob/main/rust-toolchain.toml to pin to rust 1.53?

dbr added a commit to dbr/arrow-rs that referenced this issue Jul 29, 2021
- Test against both latest stable and 1.53 as a MSRV apache#181
- Split tests which require nightly rust into separate file for clarity
dbr added a commit to dbr/arrow-rs that referenced this issue Jul 29, 2021
Sets default to MSRV version when running 'cargo ...' locally
@eitsupi
Copy link
Contributor

eitsupi commented Jul 8, 2023

Hi, I saw a post that arrow-rs currently only builds with Rust 1.70 (r-universe-org/help#268) (I haven't tried to follow up).

Is there any chance that a PR configured to check MSRV on CI would be acceptable? Used here.
https://github.com/PRQL/prql/blob/1276b7c36d882f819b982c4369cb71e40122b88c/.github/workflows/test-all.yaml#L130-L147

This was useful in recently lowering the MSRV of r-polars to 1.65. (pola-rs/r-polars#280)

@alamb
Copy link
Contributor Author

alamb commented Jul 8, 2023

I personally think having a CI that verified a MSRV would be very valuable -- and we can have the discussion about "what should the MSRV be" after that is in place.

cc @tustvold

@eitsupi
Copy link
Contributor

eitsupi commented Jul 8, 2023

Thanks for your answer!
I checked MSRV and open an issue for setting CI #4487.

@alamb
Copy link
Contributor Author

alamb commented Feb 12, 2024

Related discussion in DataFusion: apache/datafusion#9082

apache/datafusion#9082 (comment):

FWIW the vague policy I have been following in arrow-rs is at least 2-3 versions, but to only bump this when actually necessary. The major justification for a lower MSRV might be packaging systems that lag master, e.g. those in Linux distributions, I am not sure how common such a distribution mechanism is for Rust packages though.

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
Projects
None yet
Development

No branches or pull requests

3 participants