ci: handle checpatch warnings as errors and increase line limit to 100

[nashif]

Fail CI if we have both errors and warnings.

Fixes #29960
Closes #30426

Signed-off-by: Anas Nashif [email protected]

[pabigot]

Based on previous history in #27469 (comment) I think the line length change should be pulled out and handled separately. But I won't block this for that reason alone.

I don't object to making certain warnings errors if that causes the checkpatch part of compliance to go red, as long as it's understood that we're still going to have PRs that need to be merged with checkpatch errors where the diagnostic is simply irrelevant, and the reviewers and mergers will have to be careful to handle those exceptions consistently.

[nashif]

Based on previous history in #27469 (comment) I think the line length change should be pulled out and handled separately. But I won't block this for that reason alone.

This is all related and should be done the same time IMO, the main reason warnings are not failing CI right now is because of max line length.

[mbolivar-nordic]

Does this checkpatch configuration file treat complex macros without a do { ... } while (0) as warnings?

If so, it will do the wrong thing in some cases and error out when it should be ignored. E.g. soc/riscv/openisa_rv32m1/soc_offsets.h has a GEN_SOC_OFFSET_SYMS definition that can't be wrapped with a do/while.

[nashif]

Does this checkpatch configuration file treat complex macros without a do { ... } while (0) as warnings?

If so, it will do the wrong thing in some cases and error out when it should be ignored. E.g. soc/riscv/openisa_rv32m1/soc_offsets.h has a GEN_SOC_OFFSET_SYMS definition that can't be wrapped with a do/while.

we probably should just consider disabling the COMPLEX_MACRO warning in the configuration file and stop the many false positives we get. Right now they are being ignored anyways.

[pabigot]

we probably should just consider disabling the COMPLEX_MACRO warning in the configuration file and stop the many false positives we get. Right now they are being ignored anyways.

Also MULTISTATEMENT_MACRO_USE_DO_WHILE since that's wrong when the macros produce declarations, and TRAILING_SEMICOLON in similar circumstances.

LINE_SPACING generates diagnostics for things like static K_FOO_DEFINE() obj in a function when another declaration immediately precedes it, and SPACING when using declaring int useful(FILE* stream, ...) because it sees a multiplication expression.

This is a slippery slope. Maybe we should just throw out checkpatch.

[nashif]

Also MULTISTATEMENT_MACRO_USE_DO_WHILE since that's wrong when the macros produce declarations, and TRAILING_SEMICOLON in similar circumstances.

LINE_SPACING generates diagnostics for things like static K_FOO_DEFINE() obj in a function when another declaration immediately precedes it, and SPACING when using declaring int useful(FILE* stream, ...) because it sees a multiplication expression.

This is a slippery slope. Maybe we should just throw out checkpatch.

quick grep shows that checkpatch has 140 warnings and 47 errors, we right now ignore the 140 warnings and just enforce the errors.

It is not a slippery slope if we are decisive about certain rules and either remove them completely because they do not apply or fix them if that make sense. However, having a rule that we always have to override and that would confuse contributors and delay processing of PRs is the slippery slope we have to deal with.

checkpatch is not some type of holy book we have to follow and adhere to every rule, we have to be able to make "smart" decisions based on the nature of our code base and not how this works in Linux.

If we are to enforce warnings in CI then the most frequent and "annoying" warning going to be the line length, also because we have not enforce it for years, so all of the sudden EVERY change will fail.

again, there is nothing wrong with disabling some rules that are inconsistent and cause confusing and almost always require us to override.

[cvinayak]

My approval is not required for the changes in this PR, as I am not an expert in these checks/scripts.

[carlescufi]

Let's get this in. @nashif I'd appreciate if you'd send an email to the mailing list informing of the policy change after this is merged.

[nashif]

looking into removing some very common and irrelevant warnings before we merge.