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

Enable unused_crate_dependencies Rust lint, remove unused dependencies #6804

Merged
merged 9 commits into from
Dec 11, 2024

Conversation

findepi
Copy link
Member

@findepi findepi commented Nov 26, 2024

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?

  • enable unused_crate_dependencies Rust lint for all crates
  • remove unused dependencies flagged by that lint

Are there any user-facing changes?

no

@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate object-store Object Store Interface parquet-derive labels Nov 26, 2024
@findepi
Copy link
Member Author

findepi commented Nov 26, 2024

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.
However, it failed in an unexpected way https://github.com/apache/arrow-rs/actions/runs/12027104363/job/33527467696?pr=6804

Run cargo clippy -- -D warnings
  cargo clippy -- -D warnings
  shell: sh -e {0}
  env:
    RUSTFLAGS: -C debuginfo=1
    RUST_BACKTRACE: 1
error: failed to parse manifest at `/__w/arrow-rs/arrow-rs/object_store/Cargo.toml`

Caused by:
  error inheriting `lints` from workspace root manifest's `workspace.lints`

Caused by:
  failed to find a workspace root

Any idea why failed to find a workspace root? i am not getting this error when i run

cargo clean
RUSTFLAGS="-C debuginfo=1" RUST_BACKTRACE=1 cargo +stable clippy -- -D warnings

locally

@findepi findepi marked this pull request as draft November 26, 2024 09:08
@tustvold
Copy link
Contributor

Any idea why failed to find a workspace root? i am not getting this error when i run

object_store is not a member of the crate workspace as it is released separately

@findepi
Copy link
Member Author

findepi commented Nov 26, 2024

Thanks, that explains why it fails on CI.
I missed the working-directory: object_store when replying CI steps locally.

@tustvold
Copy link
Contributor

tustvold commented Nov 29, 2024

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

@tustvold tustvold marked this pull request as draft November 29, 2024 17:08
@findepi findepi marked this pull request as ready for review December 2, 2024 09:46
@github-actions github-actions bot removed parquet Changes to the parquet crate object-store Object Store Interface parquet-derive labels Dec 2, 2024
@findepi
Copy link
Member Author

findepi commented Dec 2, 2024

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 (#allow or fake imports). Moreover, being very strict about dev/test dependencies isn't probably useful, as it doesn't benefit Arrow's users in any way.

@findepi
Copy link
Member Author

findepi commented Dec 10, 2024

cc @alamb

Copy link
Contributor

@alamb alamb left a 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
Copy link
Contributor

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:

Suggested change
- name: Clippy arrow-schema
- name: Clippy arrow-schema with all features

Copy link
Member Author

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"

Copy link
Member Author

Choose a reason for hiding this comment

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

done

.github/workflows/arrow.yml Show resolved Hide resolved
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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@alamb
Copy link
Contributor

alamb commented Dec 10, 2024

I'll plan to merge this tomorrow unless there are any other comments.

I hope to make the RC around Thursday

@alamb alamb changed the title Enable unused_crate_dependencies Rust lint Enable unused_crate_dependencies Rust lint, remove unused dependencies Dec 10, 2024
@alamb alamb merged commit 4acf4d3 into apache:main Dec 11, 2024
34 checks passed
@alamb
Copy link
Contributor

alamb commented Dec 11, 2024

Thanks again @findepi

@findepi findepi deleted the findepi/unu branch December 13, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a lint to automatically check for unused dependencies
3 participants