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

Fix msrv check by pinning home dependency #605

Merged
merged 5 commits into from
Dec 17, 2024
Merged

Conversation

nicklan
Copy link
Collaborator

@nicklan nicklan commented Dec 17, 2024

What changes are proposed in this pull request?

hdfs-native depends on which which depends on home. home recently updated and bumped their msrv to 1.81. This pins us to the previous version.

How was this change tested?

Check that the msrv test now passes

@nicklan nicklan changed the title see if trace says anything Fix msrv check Dec 17, 2024
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.45%. Comparing base (d058bca) to head (6688236).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #605   +/-   ##
=======================================
  Coverage   83.45%   83.45%           
=======================================
  Files          74       74           
  Lines       16877    16877           
  Branches    16877    16877           
=======================================
  Hits        14084    14084           
  Misses       2135     2135           
  Partials      658      658           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nicklan nicklan changed the title Fix msrv check Fix msrv check by pinning home dependency Dec 17, 2024
@nicklan nicklan marked this pull request as ready for review December 17, 2024 21:44
@OussamaSaoudi-db
Copy link
Collaborator

Seems like we'd need to manually update this if we want to stay up to date with hdfs/home. Is this a problem if we forget or leave it at this version?

Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

LGTM! also is there a way to get better output in the CI job? failing was just showing 'exited with code 1'

@nicklan
Copy link
Collaborator Author

nicklan commented Dec 17, 2024

Seems like we'd need to manually update this if we want to stay up to date with hdfs/home. Is this a problem if we forget or leave it at this version?

Yeah, if hdfs-native updates and needs a newer version of home we'll either have to pin an older version of that crate or bump our msrv. We can deal with that as needed.

@nicklan
Copy link
Collaborator Author

nicklan commented Dec 17, 2024

LGTM! also is there a way to get better output in the CI job? failing was just showing 'exited with code 1'

Yeah, there are some options. I didn't enable them here since I think in general checking locally should repo things as long as we remember to cargo update

Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

just needed to do the exact same change in delta-rs ...

@zachschuermann
Copy link
Collaborator

LGTM! also is there a way to get better output in the CI job? failing was just showing 'exited with code 1'

Yeah, there are some options. I didn't enable them here since I think in general checking locally should repo things as long as we remember to cargo update

Lol yea the 'remember to cargo update' is what i didn't do..

@OussamaSaoudi-db
Copy link
Collaborator

Yeah, if hdfs-native updates and needs a newer version of home we'll either have to pin an older version of that crate or bump our msrv. We can deal with that as needed.

Ahh so it will fail compilation and tell us we need to update home. That's good. thx!

@nicklan nicklan merged commit 43511d7 into delta-io:main Dec 17, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants