chore: Use cargo-udeps to check unused dependencies #1364
Conversation
tottoto
force-pushed
the
use-cargo-udeps-to-check-unused-dependencies
branch
2 times, most recently
from
a80705d to
d43b432
Compare
tottoto
force-pushed
the
use-cargo-udeps-to-check-unused-dependencies
branch
from
5e2f0fd to
cd714f8
Compare
tottoto
force-pushed
the
use-cargo-udeps-to-check-unused-dependencies
branch
from
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.
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.
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.
Can I ask the purpose of favoring tokio-stream with unused additional dependencies over futures-core?
There was a problem hiding this comment.
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.
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.
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.
@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.
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.
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.
tottoto
force-pushed
the
use-cargo-udeps-to-check-unused-dependencies
branch
from
c563e3a to
bed1b28
Compare
Motivation
Checks unused dependencies more strictly.
Solution
Replaces cargo-machete with cargo-udeps, and fixes detected dependencies and features.