ockam icon indicating copy to clipboard operation
ockam copied to clipboard

CI - Github action that turns any compiler warnings into Github issues.

Open mrinalwadhwa opened this issue 3 years ago • 5 comments

mrinalwadhwa avatar Oct 05 '21 14:10 mrinalwadhwa

@mrinalwadhwa I'd like to work on this issue, can you please assign it to me? Also can you suggest how I can get started on this, I'm aware of github actions, but need some help regarding how warnings can be handled as events

deepto98 avatar Oct 05 '21 20:10 deepto98

@deepto98 thank you for picking this up 🥳

Context:

The over all goal is if we intentionally turn off warning lints in our Rust code somewhere then this action should create an issue, so we remember to turn it back on.

For example this create has it enabled but this other carte has it disabled.

  1. One simple approach maybe to simple grep for the #![deny( blocks in each crate and create an issue if they are disabled.
  2. Another more complicated approach may be to dig into rustc flags. I think RUSTFLAGS=-Dwarnings cargo check will fail if there are warning.

@thomcc @SanjoDeundiak @jared-s @metaclips thoughts?

mrinalwadhwa avatar Oct 05 '21 20:10 mrinalwadhwa

@mrinalwadhwa @deepto98 we could run RUSTFLAGS="-D warnings" cargo check only on develop branch and then if the command fails, we create an issue with the logs. What do you all think?. Why I suggest the CI should run only on develop is that originally, our CI fails on #![deny()] errors so if warnings is omitted, we get to catch the errors after merge. Also, we don't want to be flooded with issues due to omitted warnings.

metaclips avatar Oct 06 '21 14:10 metaclips

So, I'm not sure what this means really. I kind of don't think we should plan on merging code into develop before cargo check/cargo clippy are clean and I also don't think we should assume #[allow(...)] is always a TODO — it's often because the lint doesn't apply for whatever reason.

In fact, for clippy lints, they're developed under the assumption that you will use allow (and that that's good — it serves as "documentation" that "yes, I know this looks odd, it's intentional", and so it's not really a bug if the lints have false positives — anything that might cause issues should be pointed out.

OTOH for cargo check lints those have very few false positives, and I'd be okay with filing issues on any commented-out item in #![deny] at the crate root.

(Also, Note that at the moment, there are issues with our detection of warnings, which are fixed in #1816)

thomcc avatar Oct 06 '21 22:10 thomcc

Thanks @thomcc

I think this is a good place to start.

filing issues on any commented-out item in #![deny] at the crate root.

mrinalwadhwa avatar Oct 06 '21 23:10 mrinalwadhwa

Closing, its too old, we can revisit if we feel the need again.

mrinalwadhwa avatar Jun 09 '23 10:06 mrinalwadhwa