-
Notifications
You must be signed in to change notification settings - Fork 259
add msrvcheck #1328
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
add msrvcheck #1328
Conversation
c32f572 to
5e88c60
Compare
milenkovicm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @killzoner,
it would be great if we get a bit of documentation for this, whats expected behaviour if there is MSRV mismatch, will it fail the CI? do we check only datafusion or others as well?
|
Hey @milenkovicm thanks for the review! Right now we check only datafusion, as we need to make sure that there is a proper MSRV defined in the crate toml. Also I did a check with compat, but if we want stricter validation (like MSRV should be equal), I can change it accordingly |
9c98dab to
52ac288
Compare
73119e6 to
be66115
Compare
|
Example of failing check with MSRV downgraded: Current project MSRV: 1.85.0
thread 'main' panicked at src/main.rs:60:17:
package 'datafusion' has MSRV 1.86.0 not compatible with current project MSRV 1.85.0 |
I see, should it be other way around I wonder. We need to support MSRV which datafusion supports or older |
It's a bit counter intuitive yes, but apparently this is well this way See usage in Basically whenever we update a dependency (here |
|
all right, if I understand correctly, if datafusion is 1.85 we could decide to have ballista at 1.86 but not 1.84, as datafusion might not work in 1.84? |
That's my understanding yes |
|
thanks @killzoner |
Thank you for the detailed reviews |
Which issue does this PR close?
It's an automated way to track msrv with upstream
datafusion.Currently cargo checks for msrv when adding a new dependency on latest resolver (https://rust-lang.github.io/rfcs/3537-msrv-resolver.html), not for existing ones.
Rationale for this change
This is one of the items from #1271
What changes are included in this PR?
Add a CI check to make sure MSRV is aligned with upstream
datafusionprojects.Are there any user-facing changes?
No