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

chore: Use cargo-udeps to check unused dependencies #1364

Merged

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented Apr 19, 2023

Motivation

Checks unused dependencies more strictly.

Solution

Replaces cargo-machete with cargo-udeps, and fixes detected dependencies and features.

@tottoto tottoto force-pushed the use-cargo-udeps-to-check-unused-dependencies branch 2 times, most recently from a80705d to d43b432 Compare April 19, 2023 10:41
@tottoto tottoto marked this pull request as draft April 19, 2023 10:56
@tottoto tottoto force-pushed the use-cargo-udeps-to-check-unused-dependencies branch from 5e2f0fd to cd714f8 Compare April 19, 2023 13:51
@tottoto tottoto force-pushed the use-cargo-udeps-to-check-unused-dependencies branch from cd714f8 to 62d5f35 Compare April 19, 2023 14:08
@tottoto tottoto marked this pull request as ready for review April 19, 2023 14:26
@@ -9,7 +9,7 @@ use std::{
};
use tokio::sync::mpsc::Receiver;

use tokio_stream::Stream;
use futures_core::Stream;
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Member

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 😄

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@tottoto tottoto Apr 24, 2023

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?

Copy link
Collaborator Author

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.

Copy link
Member

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!

@tottoto
Copy link
Collaborator Author

tottoto commented Apr 24, 2023

cargo-deny is failed due to duplicate version of regex-syntax. It will be fixed in #1373.

@LucioFranco
Copy link
Member

I updated your PR and with the commit from master that fixes it all. Should be good to go.

@tottoto
Copy link
Collaborator Author

tottoto commented Apr 25, 2023

Thanks!

@tottoto
Copy link
Collaborator Author

tottoto commented Apr 25, 2023

The msrv test is failed because of 9d9458d. std::task::ready (and core::task::ready) was stabilized at 1.64.0, but our msrv is 1.60.

@tottoto tottoto requested a review from LucioFranco April 25, 2023 11:20
@tottoto tottoto force-pushed the use-cargo-udeps-to-check-unused-dependencies branch from c563e3a to bed1b28 Compare May 17, 2023 17:26
Copy link
Member

@LucioFranco LucioFranco left a 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!

@LucioFranco LucioFranco added this pull request to the merge queue Jun 22, 2023
Merged via the queue into hyperium:master with commit c9c4acb Jun 22, 2023
@tottoto tottoto deleted the use-cargo-udeps-to-check-unused-dependencies branch June 22, 2023 13:18
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