-
Notifications
You must be signed in to change notification settings - Fork 817
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
Enable unused_crate_dependencies Rust lint, remove unused dependencies #6804
base: main
Are you sure you want to change the base?
Conversation
The PR is expected to fail because (1) i didn't remove all the dependencies flagged by the lint and (2) there are false-positives.
Any idea why
locally |
object_store is not a member of the crate workspace as it is released separately |
Thanks, that explains why it fails on CI. |
Marking this as a draft as there appear to still be unresolved issues, feel free to mark ready for review when you would like me (or someone else) to take another look |
And disable the check since it feels impossible/infeasible to configure it the right way.
I changed the approach. Instead of enabling lint in the cargo.toml, i switched to enaling it on specific cargo invocations as part of CI. Why? Because it feels cargo lacks necessary information to make this check work well in all cases. It apparently does not distinguishes dependencies when compiling tests and examples, or tests with different feature flags. I couldn't find a way to make it work, without adding a lot of suppressions ( |
cc @alamb |
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 @findepi
Sorry for the delay in reviewing
I reviewed the CI job output and it looks good to me (all the jobs ran well):
https://github.com/apache/arrow-rs/actions/runs/12116683077/job/33777545429?pr=6804
cargo clippy -p "$mod" -- -D unused_crate_dependencies | ||
cargo clippy -p "$mod" --all-features -- -D unused_crate_dependencies | ||
cargo clippy -p "$mod" --no-default-features -- -D unused_crate_dependencies | ||
- name: Clippy arrow-schema |
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.
Nit- the names are slightly inconsistent:
- name: Clippy arrow-schema | |
- name: Clippy arrow-schema with all features |
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'd go opposite direction, ie remove "with all features" as now this tests various combinations: "all features", "default features", "no features"
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.
done
prost = { version = "0.13.1", default-features = false, features = ["prost-derive"] } | ||
# For Timestamp type | ||
prost-types = { version = "0.13.1", default-features = false } | ||
tokio = { version = "1.0", default-features = false, features = ["macros", "rt", "rt-multi-thread"] } | ||
tokio = { version = "1.0", default-features = false, features = ["macros", "rt", "rt-multi-thread"], optional = true } |
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'll plan to merge this tomorrow unless there are any other comments. I hope to make the RC around Thursday |
Which issue does this PR close?
Fixes #6796
Rationale for this change
Removing unused dependencies is expected to improve compile times.
What changes are included in this PR?
unused_crate_dependencies
Rust lint for all cratesAre there any user-facing changes?
no