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 failures in CI #1033

Closed
wants to merge 2 commits into from
Closed

Fix MSRV failures in CI #1033

wants to merge 2 commits into from

Conversation

esheppa
Copy link
Collaborator

@esheppa esheppa commented Apr 22, 2023

The MSRV check is failing in CI due to the dev-dependencies allowing version 1 of serde_json and serde_derive, however versions of serde_derive above 1.0.156 aren't compatible with an MSRV of 1.38.0. As a fix for now, I've locked in the versions of the serde_* dev dependencies

@djc
Copy link
Member

djc commented Apr 22, 2023

What are you trying to do here?

@esheppa esheppa changed the title add cargo clean Fix MSRV failures in CI Apr 22, 2023
@esheppa
Copy link
Collaborator Author

esheppa commented Apr 22, 2023

Should have put more detailed comments, updating now. Just trying to figure out why the MSRV check is failing in 0.4.x CI. Should be solved now

remove caching

lock in serde versions in dev deps to avoid msrv issues
@esheppa
Copy link
Collaborator Author

esheppa commented Apr 22, 2023

It looks like only serde_derive needs to be locked at a specific version here, but serde_json may move to use that version at some point

@LingMan
Copy link
Contributor

LingMan commented Apr 22, 2023

Wouldn't it be better to cargo update -p serde_derive --precise 1.0.156 in the MSRV job? That would fix CI without forcing everyone to use old versions of serde. You could even re-enable the serde feature in the test.

Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

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

@esheppa are you still working on this PR? If so, should it be put back to a "Draft"?

Otherwise, LGTM. (Full Disclosure: I didn't look into the versioning in-depth, just taking OPs comments at face value)

@esheppa
Copy link
Collaborator Author

esheppa commented Apr 30, 2023

@jtmoon79 yes still keen to get this merged if the CI failures are still happening as its a bit awkward to merge with any failed tests.

@LingMan we could also do this, however these are only for dev dependencies so we are not forcing anyone to use old versions unless they are running the tests locally. One risk with the update approach is that a test might make use of a feature not available in those versions, while this would immediately be caught by CI, it is less ergonomic.

That said, re-enabling the serde feature is a good idea. I've added in 07749dc

@pitdicker
Copy link
Collaborator

Now that the MSRV is raised to 1.56 in #1053 we would no longer need a workaround.

@esheppa
Copy link
Collaborator Author

esheppa commented May 13, 2023

Thanks - looks like this is not needed which is ideal!

@esheppa esheppa closed this May 13, 2023
@esheppa esheppa deleted the msrv-ci-fix branch May 13, 2023 13:10
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.

5 participants