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

remove unnecessary dependencies #1711

Merged
merged 5 commits into from Mar 30, 2023

Conversation

vsuryamurthy
Copy link
Contributor

Hello,

I came across some unnecessary dependencies in each crate. These dependencies are however used in other crates (or) indirectly pulled into the crate.
In this PR, I used re_viewer, as an example, to show the unnecessary dependencies. I am wondering if there is a reason why these dependencies are kept as it is. Based on your thoughts, we can see how to proceed with the other crates.

@vsuryamurthy vsuryamurthy changed the title remove unnecessary dependencies from re_viewer remove unnecessary dependencies Mar 26, 2023
@emilk emilk added the 🧑‍💻 dev experience developer experience (excluding CI) label Mar 27, 2023
@emilk
Copy link
Member

emilk commented Mar 27, 2023

Hi, and thanks for your PR!

It is very easy to forget to remove a crate once the code relying on it is gone, and I think that's what have happened here.

In the cases where something else is going on we should have a clarifying comment about it.

@vsuryamurthy
Copy link
Contributor Author

Hi, and thanks for your PR!

It is very easy to forget to remove a crate once the code relying on it is gone, and I think that's what have happened here.

In the cases where something else is going on we should have a clarifying comment about it.

I completely agree. It does not change much in the sense that it is pulled indirectly. However it is cleaner. I see more such unused dependencies over the entire set of crates.

@emilk
Copy link
Member

emilk commented Mar 27, 2023

Feel free to clean up further!

@vsuryamurthy
Copy link
Contributor Author

I removed more dependencies. Seems like this is it!

@emilk
Copy link
Member

emilk commented Mar 28, 2023

Nice! Are you using cargo-udeps to find these?

@vsuryamurthy
Copy link
Contributor Author

vsuryamurthy commented Mar 29, 2023

I use cargo machete, basically what cargo-udeps uses under the hood.
There are more dependencies that cargo machete shows actually but those are either internal ones or looks like they are used.

@vsuryamurthy
Copy link
Contributor Author

@emilk I will be on a break until the end of the week. You can decide what the next steps are with this PR!
P.s: The check fail is not my fault (I guess!)
P.s.s: I was wrong about cargo machete being used under the hood of cargo udeps. It is a different tool but much faster.

@emilk
Copy link
Member

emilk commented Mar 29, 2023

P.s: The check fail is not my fault (I guess!)

I think you need to update the Cargo.lock file, e.g. with cargo update --workspace

@vsuryamurthy
Copy link
Contributor Author

Yep yep, I see I missed that!

@emilk emilk merged commit 118630c into rerun-io:main Mar 30, 2023
17 checks passed
@emilk
Copy link
Member

emilk commented Mar 30, 2023

Thank you!

@vsuryamurthy vsuryamurthy deleted the remove_unnecessary_dependencies branch April 4, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 dev experience developer experience (excluding CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants