Skip to content

fix: speficy the version of munge for msrv check #987

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 24, 2025
Merged

fix: speficy the version of munge for msrv check #987

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@xxchan xxchan Feb 23, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ZENOTME ZENOTME Feb 23, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@xxchan xxchan Feb 23, 2025

Choose a reason for hiding this comment

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

We can't use a version newer than datafusion, otherwise the datafusion integration will break.

Copy link
Contributor Author

@ZENOTME ZENOTME Feb 23, 2025

Choose a reason for hiding this comment

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

Thanks for reminding this!

Copy link
Member

@xxchan xxchan Feb 24, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@Xuanwo Xuanwo Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, I'm a bit confused why we need this. I remember that we are using minimal-version to generate Cargo.lock?

Copy link
Contributor Author

@ZENOTME ZENOTME Feb 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@xxchan xxchan Feb 24, 2025

Choose a reason for hiding this comment

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

“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-version to generate Cargo.lock?

Caused by cargo update faststr (you can see the comment in the ci script)

Copy link
Member

@Xuanwo Xuanwo Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ZENOTME ZENOTME Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

- name: Setup MSRV Rust toolchain
uses: ./.github/actions/setup-builder
with:
Expand Down
Loading