-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
chore: Use cargo-udeps to check unused dependencies #1364
chore: Use cargo-udeps to check unused dependencies #1364
Conversation
a80705d
to
d43b432
Compare
5e2f0fd
to
cd714f8
Compare
cd714f8
to
62d5f35
Compare
@@ -9,7 +9,7 @@ use std::{ | |||
}; | |||
use tokio::sync::mpsc::Receiver; | |||
|
|||
use tokio_stream::Stream; | |||
use futures_core::Stream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As tonic does not use tokio-stream except Stream which is the re-exporting of futures-core, we could use futures-core directly to reduce dependencies. This is detected by cargo-udeps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I would like to actually ditch all futures
crates and just use the tokio-stream
version. Though this is a bigger undertaking than what this PR wants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I ask the purpose of favoring tokio-stream
with unused additional dependencies over futures-core
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we'd like to move off of the futures crates (stream is complicated since we expected it to land in std by now) since they are not as maintained as the tokio ones. Its a bit of a complicated history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually, Stream
will go into std
so then we will only depend on tokio-stream
but for now we can't do that since progress has been slow there. That said, I would rather be positioned for that future than live with one extra dependency right now if that makes sense. In addition, we control tokio-stream so its better to depend on that then directly depend on another package if that makes sense. Happy to explain more over discord/email/what ever if you're interested 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your kind explanation!
we control tokio-stream so its better to depend on that then directly depend on another package if that makes sense
Thanks to this point mainly, I think I reached to understand why we should depend on tokio-steam
instead of futures-core
. I will investigate whether it is possible to configure that we should explicitly depend on tokio-stream
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LucioFranco We've already had some futures_core::Stream
in tonic's source code. Should we replace all the existing futures_core::Stream
with tokio_stream::Stream
in this pull request, or should it be done in the another pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that the cargo-udeps
checking will success by replacing all futures_core::Stream
with tokio_stream::Stream
! Therefore I think we should do it in this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should convert everything from futures crates over. I think I originally used it but would like to remove it but I have not had time. Feel free to start on that!
|
I updated your PR and with the commit from master that fixes it all. Should be good to go. |
Thanks! |
The msrv test is failed because of 9d9458d. |
…sion" This reverts commit 9d9458d.
c563e3a
to
bed1b28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! Thanks!
Motivation
Checks unused dependencies more strictly.
Solution
Replaces cargo-machete with cargo-udeps, and fixes detected dependencies and features.