fix: speficy the version of munge for msrv check #987
Conversation
| @@ -152,6 +152,8 @@ jobs: | |||
| # Some dependencies don't correctly specify a minimal version for their dependencies and will fail to build. | |||
| # So we update these transitive dependencies here. | |||
| cargo update tap faststr metainfo linkedbytes | |||
| # munge 0.4.2 will cause fail to use 1.7.1 so specify the correct version here. | |||
| cargo update -p munge --precise 0.4.1 | |||
There was a problem hiding this comment.
munge 0.4.2 uses edition = 2024 now (which is 1.85).
djkoloski/munge@02e7be5
BTW we are discussing the MSRV policy here: https://lists.apache.org/thread/d10ntkzvkbyvh806063jblhhxgvczoth
Currently, datafusion is 1.82 https://github.com/apache/datafusion/blob/0bd9083a0b8bff6d261449a92dcd4d110976774a/Cargo.toml#L70
Therefore, I think pinning the munge version here is the best choice now.
There was a problem hiding this comment.
Thanks to refer this! Is it time to upgrade the version so that we don't need to pin the version? The last version is 7 month ago. also cc @liurenjie1024
There was a problem hiding this comment.
We can't use a version newer than datafusion, otherwise the datafusion integration will break.
There was a problem hiding this comment.
Thanks for reminding this!
There was a problem hiding this comment.
We can't use a version newer than datafusion, otherwise the datafusion integration will break.
Sorry, this is wrong. As discussed in the mail thread
There was a problem hiding this comment.
Hi, I'm a bit confused why we need this. I remember that we are using minimal-version to generate Cargo.lock?
There was a problem hiding this comment.
But this still holds, so the current solution in the PR is the best we can do.
The last MSRV change was 7 months ago, I'm not sure why this holds.🤔 “At least three months” refers to either three months after the release of a new Rust version or three months after our last MSRV update?
Hi, I'm a bit confused why we need this. I remember that we are using minimal-version to generate Cargo.lock?
But we call cargo update faststr later and it will upgrade the munge.
There was a problem hiding this comment.
“At least three months” refers to either three months after the release of a new Rust version or three months after our last MSRV update
I believe it should be the former. Or better stated as: "DataFusion's supports the last 4 stable Rust minor versions released and any such versions released within the last 4 months." https://github.com/apache/datafusion#rust-version-compatibility-policy
Hi, I'm a bit confused why we need this. I remember that we are using
minimal-versionto generateCargo.lock?
Caused by cargo update faststr (you can see the comment in the ci script)
There was a problem hiding this comment.
Ok, let's fix tap / faststr / metainfo / linkedbytes from upstream instead. Would you like to create an issue to track this and leave it in the comments? I don't want to maintain too many cargo update -p xxx --precise xxx in our CI scripts.
That said, I'm open to merging this PR, but I’d like to fix them in a more maintainable way.
There was a problem hiding this comment.
Ok, let's fix tap / faststr / metainfo / linkedbytes from upstream instead. Would you like to create an issue to track this and leave it in the comments? I don't want to maintain too many cargo update -p xxx --precise xxx in our CI scripts.
Track in: #1000.
#704 fail in msrv check and I find that's because
cargo update faststrwill update the munge to0.4.2instead of0.4.1. The simple fix way is to specify the precise version of munge. But I'm not sure whether it's good practice here. Do you have any suggestions for this? cc @Xuanwo @xxchan