chore: add hach clippy with each feature #3166
Conversation
a6aa382 to
be3a255
Compare
There was a problem hiding this comment.
Thanks for creating this PR @lsytj0413. While your past fixes have shown that we need tooling support for catching compilation problems with using features, I am wondering what the additional costs of cargo hack clippy --each-feature are (added compilation time). If it is negligible then it is ok. If it slows the feedback cycle down, then I am not 100% whether the added safety is worth the slowdown. Maybe a compromise could be to have a clippy-each-feature command which we only run on CI?
| @@ -77,7 +77,7 @@ check-fmt: | |||
| cargo fmt --all -- --check | |||
|
|
|||
| clippy: (_target-installed target) | |||
| cargo clippy {{ _target-option }} --all-targets --workspace -- -D warnings | |||
There was a problem hiding this comment.
What's the additional compilation time due to using hack --each-feature on CI and locally?
There was a problem hiding this comment.
hack clippy takes 18 minutes and clippy takes 4 minutes on my computer. If it's too long, maybe we can run hack --each-feature periodically ( per day or per week)?
There was a problem hiding this comment.
Running it on a daily basis on CI could be a worthwhile compromise indeed.
There was a problem hiding this comment.
I have add an daily workflow, ptal
e211383 to
a869b1b
Compare
There was a problem hiding this comment.
Thanks a lot for updating this PR @lsytj0413. The changes look good to me :-) This will help us to catch changes that break features we have defined in our crates. Thanks a lot 🙏
When running just hack-clippy there are a few errors. I think before we can merge this PR, we need to address them. Do you want to take a stab at them?
.github/workflows/hack-clippy.yml
Outdated
| LOCAL_CLUSTER_RUNNER_FORWARD_LOGS: "true" | ||
| LOCAL_CLUSTER_RUNNER_RETAIN_TEMPDIR: "true" |
There was a problem hiding this comment.
Are we running tests as part of the hack-clippy command? If not, then these env variables might not be needed.
a869b1b to
13c158e
Compare
|
Sorry for not getting back earlier @lsytj0413. The changes look good. I am currently trying to fix the violations when running |
8349c8d to
f07e4c0
Compare
This is the outcome of running just hack-clippy and making the build pass.
f07e4c0 to
6dff0f6
Compare
@tillrohrmann propose to add clippy for each features, this will help us catch some errors which will cause compile failed when enable different feature or disable some feature. If you prefer this i will go on, I'm looking forward to receiving your feedback.