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

replace futures with futures-util #288

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Dec 4, 2024

This slightly reduces depedency tree.

This slightly reduces depedency tree.
Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

Seams that this change broke the msrv test 😢

We should go little bit deeper to understand what's happen and if is possible to leave the same msrv or we should bump it

@mati865
Copy link
Contributor Author

mati865 commented Dec 4, 2024

The failure is unrelated. futures crate pulls in futures-util anyway, so the only thing that changes here is using code directly instead of reexport from futures. I'm pretty sure any PR would fail like that.

I don't know this test setup, but to me it looks like toolchain in version 1.83 has generated lockfile in version 4 (this is the new default), but version 4 is supported only by toolchains 1,78+.

@la10736
Copy link
Owner

la10736 commented Dec 5, 2024

The failure is unrelated. futures crate pulls in futures-util anyway, so the only thing that changes here is using code directly instead of reexport from futures. I'm pretty sure any PR would fail like that.

I don't know this test setup, but to me it looks like toolchain in version 1.83 has generated lockfile in version 4 (this is the new default), but version 4 is supported only by toolchains 1,78+.

Maybe you're right. Yesterday it was midnight and I was too tired to check msrv on main branch: I just trusted the weakly CI... But there is something wired here: the weakly CI was run 3 days ago and didn't find any issues.

Now I run again the CI on main to check if there is something new.

@la10736
Copy link
Owner

la10736 commented Dec 5, 2024

Now I run again the CI on main to check if there is something new.

Ok, now the CI is failed also in main in the same way. I guess that a new cargo hack version was released in these days.

I can merge this PR because the issue is un related

Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

Thanks

LGTM

@la10736 la10736 merged commit 9a78bfb into la10736:master Dec 5, 2024
1 of 2 checks passed
@mati865 mati865 deleted the push-lzmnyxtqpsyz branch December 5, 2024 17:14
@mati865
Copy link
Contributor Author

mati865 commented Dec 5, 2024

No worries, thanks for the quick response.

@mati865
Copy link
Contributor Author

mati865 commented Dec 5, 2024

I don't know this test setup, but to me it looks like toolchain in version 1.83 has generated lockfile in version 4 (this is the new default), but version 4 is supported only by toolchains 1,78+.

I've made a quick look at I'm certain this is the case. Pinning the toolchain in MSRV job to anything lower than 1.83 will avoid the problem.

@la10736
Copy link
Owner

la10736 commented Dec 5, 2024 via email

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.

2 participants