Enforce all checkpatch warnings and move to 100 characters per line

[nashif]

Introduction

Enforce checkpatch warnings and change line length to 100 (from 80)

Problem description

Right now we do not enforce checkpatch warnings and do not fail CI on warnings. Some of the warnings expose various issues that are worth fixing, however, in most PRs, the majority of the warning are related to line length.

We used to just put the warnings in a comment but not enforce, this is not really helpful, because it does cause inconsistency in reviews and we end up delaying PR from going in when a reviewer asks to fix warning but CI actually passes without fixing them.
Not all reviewers for example would care if you have lines > 80, so this becomes inconsistent

Proposed change

• Enforce checkpatch warnings in CI, so CI will fail on warnings + errors
• Change default max line length and match what Linux has (100 instead of 80)

Proposed change (Detailed)

• change compliance check to not ignore warnings
• change checkpatch to allow 100 instead of 80 chars per line.

Dependencies

None

Concerns and Unresolved Questions

None

Alternatives

Keep it as is and miss on warnings.

[mbolivar-nordic]

I may be old-fashioned, but I still prefer the 80 column rule whenever practical for Python and C. More verbose languages like CMake (or Java) suffer unless you drop that requirement, but I like it for firmware.

80 characters per line allows 2 side-by-side text windows even on laptop screens, and 3 or more on larger monitors. This really helps me cross reference information in multiple files when I'm trying to figure things out.

Moving to 100 characters causes line wrapping in either of those cases, even on my 4K 28" desktop monitor, unless I make the font size too small to read comfortably.

So I hope we can keep the "preferred" line length 80, even if we allow up to 100.

[mbolivar-nordic]

This really helps me cross reference information in multiple files when I'm trying to figure things out.

This also makes it a lot easier to look at a diff at the same time as a source file affected by the diff, side by side.

[gmarull]

I think sticking to 80 but allowing up to 100 is a good compromise. In some cases lines in the 80-90 range make code more readable. Somewhat related: one of the most popular Python formatters, black, defaults to 88 columns, see the reasons here: https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length

[cfriedt]

I'm not entirely sure how well Doxygen responds to line breaks - maybe I'm being paranoid - but if it's not well behaved, some lines in C need to be long (e.g. a URL in Doxygen comments), so failing on that warning might not be a good idea.

[nashif]

Pr #30427 implements the changes

[erwango]

I think sticking to 80 but allowing up to 100 is a good compromise.

I would tend to agree as well. Maybe 90 would even be sufficient for 99% of the cases.

Anyway enforcing checkpatch is desired indeed.

[pabigot]

I have no problems with changing all checkpatch diagnostics to errors, as long as it's understood that not all checkpatch diagnostics are valid or can be reasonably fixed, so we will always have to merge PRs that fail to pass the checkpatch part of compliance.

I tried moving to 100 characters following Linux in #27469, and had to remove it because of strong objections from @andrewboie. What has changed since then?

Based on that history this issue should be separated into (a) enforce all checkpatch warnings as errors, and (b) what line length should Zephyr use. That would allow us to solve #29960 (a) and express opinions about line lengths and potential justifications thereof somewhere else.

[nashif]

I have no problems with changing all checkpatch diagnostics to errors, as long as it's understood that not all checkpatch diagnostics are valid or can be reasonably fixed, so we will always have to merge PRs that fail to pass the checkpatch part of compliance.

I would rather not change much in checkpatch itself, instead, all the unrelated warnings/errors should be disabled in .checkpatch.conf.

I tried moving to 100 characters following Linux in #27469, and had to remove it because of strong objections from @andrewboie. What has changed since then?

nothing changed, the reason was the fact it was done as part of updating checkpatch, i.e. changing policy and we wanted this to be discussed somewhere else (read: here).

Based on that history this issue should be separated into (a) enforce all checkpatch warnings as errors, and (b) what line length should Zephyr use. That would allow us to solve #29960 (a) and express opinions about line lengths and potential justifications thereof somewhere else.

Both issues are related, we cant enforce warnings easily if keep line length at 80, this has to be done in one shot.