Stream: git-cranelift

Topic: cranelift / Issue #1170 Integrate clippy checking in CI b...

GitHub (Jan 10 2020 at 21:51):

abrown commented on Issue #1170:

Just looked at this a bit more and I think someone with rights should install https://github.com/marketplace/actions/rust-clippy-check. That GitHub action will use clippy to check PRs and post any issues as annotations (follow the previous link for an example). I think to use this we need to use a GitHub secret token, see https://github.com/marketplace/actions/rust-clippy-check#example-workflow.

GitHub (Jan 13 2020 at 10:07):

bnjbvr commented on Issue #1170:

Interesting. I'm only wondering what this will do to existing Clippy warnings: will they be reported as part of a PR that modifies code around, or will only new Clippy warnings be reported? In the former case, I think it would be nicer to first fix all the Clippy warnings before enabling such a check.

GitHub (Jan 13 2020 at 17:35):

abrown commented on Issue #1170:

I suspect we would have to fix all the clippy warnings first but that wasn't too difficult: #1340.

GitHub (Feb 26 2020 at 19:59):

abrown commented on Issue #1170:

@alexcrichton, what do you think about adding this to cranelift and wasmtime, perhaps when they merge?

GitHub (Feb 26 2020 at 22:27):

alexcrichton commented on Issue #1170:

I personally agree with @fitzgen's thoughts above where hard-gating on clippy on CI is often quite onerous. I personally disagree with a lot of clippy's lints as well which doesn't help my own opinion though... I would advocate for a "here's how to run it if you want" at most, but otherwise don't hard-gate it on CI.

GitHub (Feb 27 2020 at 03:48):

abrown commented on Issue #1170:

:+1: I agree with the "no hard-gating" point of view as well. I think the point of installing this clippy action would be for awareness more than anything else: instead of running clippy in CI, ignoring failures, and thus doing extra work that is ignored, this action would make PR submitters and reviewers aware of what clippy thinks is problematic. If clippy finds a real issue--good; if clippy finds irrelevant stuff then they can ignore the warning (locally or globally).

GitHub (Feb 27 2020 at 04:08):

philipc commented on Issue #1170:

The limitations on https://github.com/marketplace/actions/rust-clippy-check make it seem to be not worth the effort: "Due to token permissions, this Action WILL NOT be able to post clippy annotations for Pull Requests from the forked repositories."

GitHub (Feb 27 2020 at 17:43):

abrown commented on Issue #1170:

Hm, yeah, then there's not much point to doing this until that is fixed; seems like this can issue can go forward when/if https://github.com/actions-rs/clippy-check/issues/2 is fixed.

GitHub (Feb 28 2020 at 23:27):

alexcrichton transferred Issue #1170 (assigned to abrown):

Please try to describe precisely what you would like to do in Cranelift and/or
expect from it. You can answer the questions below if they're relevant and
delete this text before submitting. Thanks for opening an issue!

• What is the feature or code improvement you would like to do in Cranelift?
  After merging #1169 (and seeing #1168), it occurred to me that we should output clippy warnings as a part of test-all.sh. I'm not sure we should fail the build (though this could be an option) but this would make non-clippy-approved additions more visible.

• What is the value of adding this in Cranelift?
  It should result in cleaner, more standard code in the long run.

• Do you have an implementation plan, and/or ideas for data structures or
  algorithms to use?
  Add cargo clippy to test-all.sh and ensure clippy is installed for Azure pipelines.

• Have you considered alternative implementations? If so, how are they better
  or worse than your proposal?
  No.

Last updated: Nov 22 2024 at 17:03 UTC