scripts: update checkpatch based on current linux kernel version #27469

pabigot · 2020-08-10T15:50:32Z

This squashes and applies the Linux diffs to scripts/checkpatch.pl between Linux commit 16fbf79b0f83bc75 ("Linux 5.6-rc7") and 9123e3a74ec7b93 ("Linux 5.9-rc1") except for commits identified below.

The last 1000 commits to Zephyr master were compared for checkpatch output differences between the previous Zephyr version and this version. One new diagnostic about function declarations with an empty parameter-list was introduced (FUNCTION_WITHOUT_ARGS). The text of an existing diagnostic was changed (DT_SPLIT_BINDING_PATCH). The text of LONG_LINE diagnostics was enhanced to provide the actual line length.

Linux commit dfa05c28ca7ffc0a ("checkpatch: remove email address comment from email address comparisons") was removed because differences in the scripts resulted in false signed-off-by check diagnostics when a full name included characters not in Basic Latin, due to changes in how the author name was extracted. Earlier upstream changes not integrated into Zephyr may be required.

Linux commit b95692f8b3000166 ("checkpatch: prefer fallthrough; over fallthrough comments") was removed because Zephyr doesn't support the upstream pseudo keyword.

Linux commit bdc48fa11e46f867 ("checkpatch/coding-style: deprecate 80-column warning") was edited to not actually change the 80-column maximum line limit as this change has not been mooted for Zephyr.

Linux commit ced69da1db0b57bb ("checkpatch: fix CONST_STRUCT when const_structs.checkpatch is missing") was edited to the CONST_STRUCT file as that's not supported in Zephyr.

Also, separately, the new REPEATED_WORD diagnostic was disabled.

nashif · 2020-08-20T19:30:23Z

.checkpatch.conf

@@ -1,7 +1,7 @@
 --emacs
 --summary-file
 --show-types
--max-line-length=80
+--max-line-length=100


this needs to be discussed, I am fine with that. I also think we should decide if we keep following the linux style with changes or if we should establish our own based on a list of rules (from Linux and others) and grow from there. Changes that happen to Linux should not be automatically adopted into Zephyr, thats why we have a few exceptions already.

I remember seeing the LKML discussion about this and as someone who uses a text editor with multiple columns I would not be thrilled with this change to the coding style

If accepted, we need to update .editorconfigto match.

I remember thinking about this, but apparently didn't do it. Done now: all line length checks updated in one combined commit.

henrikbrixandersen · 2020-08-20T19:34:28Z

scripts/checkpatch.pl

@@ -3105,7 +3159,7 @@ sub process {
 					$comment = '/*';
 				} elsif ($realfile =~ /\.(c|dts|dtsi)$/) {
 					$comment = '//';
-				} elsif (($checklicenseline == 2) || $realfile =~ /\.(sh|pl|py|awk|tc)$/) {
+				} elsif (($checklicenseline == 2) || $realfile =~ /\.(sh|pl|py|awk|tc|yaml)$/) {


This would introduce a license check against .yaml files in the Zephyr tree, right?

Presumably. AFAIK we were already requiring that, it just (maybe) wasn't being checked.

pabigot · 2020-08-20T19:38:33Z

Also see #27058 on clang-format and #21392 which is the motivating driver for a lot of this effort.

I will say I hate some of what clang-format does. But I'd rather have an objective standard against which style conformance can be measured than the "This way is right", "no this way is right" we have now.

nashif · 2020-08-20T19:39:53Z

I will say I hate some of what clang-format does. But I'd rather have an objective standard against which style conformance can be measured than the "This way is right", "no this way is right" we have now.

I would not say clang-format is a standard, you make your own standard and enable/disable based on your needs, clang-format is just a tool

pabigot · 2020-08-20T19:43:44Z

I would not say clang-format is a standard, you make your own standard and enable/disable based on your needs, clang-format is just a tool

yes, I was referring to what clang-format does using the .clang-format that comes from Linux that is updated in #27058.

andrewboie · 2020-08-20T20:49:11Z

I don't want to hold up this whole patch but can we separate out the line length policy change into its own PR?

nashif · 2020-08-20T20:53:45Z

I don't want to hold up this whole patch but can we separate out the line length policy change into its own PR?

agree, I would just keep the 80 in the dot file, because this one has nothing to do with the script changes and syncing with upstream, we will need to decide on moving to 100 in a different thread...

andrewboie

Adding -1 until column policy change is separated out

This squashes and applies the Linux diffs to scripts/checkpatch.pl between Linux commit 16fbf79b0f83bc75 ("Linux 5.6-rc7") and 9123e3a74ec7b93 ("Linux 5.9-rc1") except for commits identified below. The last 1000 commits to Zephyr master were compared for checkpatch output differences between the previous Zephyr version and this version. One new diagnostic about function declarations with an empty parameter-list was introduced (FUNCTION_WITHOUT_ARGS). The text of an existing diagnostic was changed (DT_SPLIT_BINDING_PATCH). The text of LONG_LINE diagnostics was enhanced to provide the actual line length. Linux commit dfa05c28ca7ffc0a ("checkpatch: remove email address comment from email address comparisons") was removed because differences in the scripts resulted in false signed-off-by check diagnostics when a full name included characters not in Basic Latin, due to changes in how the author name was extracted. Earlier upstream changes not integrated into Zephyr may be required. Linux commit b95692f8b3000166 ("checkpatch: prefer fallthrough; over fallthrough comments") was removed because Zephyr doesn't support the upstream pseudo keyword. Linux commit bdc48fa11e46f867 ("checkpatch/coding-style: deprecate 80-column warning") was edited to not actually change the 80-column maximum line limit as this change has not been mooted for Zephyr. Linux commit ced69da1db0b57bb ("checkpatch: fix CONST_STRUCT when const_structs.checkpatch is missing") was edited to the CONST_STRUCT file as that's not supported in Zephyr. Signed-off-by: Peter A. Bigot <[email protected]>

@param

A new check in Linux checks for repeated words, which spews false positives from text like: * @param param the parameter * @param device device instance Signed-off-by: Peter A. Bigot <[email protected]>

andrewboie

Thanks LGTM!

pabigot · 2020-08-20T21:57:07Z

The maximum line length changes have been separated out and are tracked in https://github.com/pabigot/zephyr/tree/pabigot/20200810a%2Bll so they're easy to apply later.

I've added six more changes that were added in Linux since I opened this 10 days ago. I've disabled one of those in a separate commit. The PR description has been updated to note the disposition of some of those additions, and remove reference to the line length change.

However, there's a problem with the logic of removing a specific policy change from this update. Everybody should be fully aware that as long as we're using Linux checkpatch we're adopting Linux policy, even if we don't see the effect in the test of the last 1000 commits. There are 26 distinct changes to checkpatch aggregated into this squashed commit. Nobody here can tell what they do because we're discarding the commit messages in the squash. I have some idea because I read them all, but I'm not editing them for non-technical reasons. One visible change I think is good while another is perhaps problematic, but it was already present in an equally problematic way anyway. They're mentioned in the updated description.

If the consensus is we don't want checkpatch-monitored policy changes from Linux to be adopted into Zephyr, then we should stop updating this script. There's a lot of activity happening up there related to this script, and maybe it's time to stop trying to follow Linux style and codify one for Zephyr.

henrikbrixandersen

LGTM

pabigot mentioned this pull request Aug 10, 2020

Zephyr Modules: document changes & improvements in the workflow #27185

Merged

pabigot added this to the v2.4.0 milestone Aug 12, 2020

pabigot requested a review from nashif August 20, 2020 19:20

nashif reviewed Aug 20, 2020

View reviewed changes

nashif requested review from MaureenHelm, andrewboie, carlescufi, galak, ioannisg and jfischer-no August 20, 2020 19:31

henrikbrixandersen reviewed Aug 20, 2020

View reviewed changes

pabigot force-pushed the pabigot/20200810a branch from 881ef58 to bd1310b Compare August 20, 2020 20:28

andrewboie suggested changes Aug 20, 2020

View reviewed changes

pabigot added 2 commits August 20, 2020 16:43

checkpatch: disable REPEATED_WORD

b2f44f2

A new check in Linux checks for repeated words, which spews false positives from text like: * @param param the parameter * @param device device instance Signed-off-by: Peter A. Bigot <[email protected]>

pabigot force-pushed the pabigot/20200810a branch from bd1310b to b2f44f2 Compare August 20, 2020 21:45

andrewboie approved these changes Aug 20, 2020

View reviewed changes

nashif approved these changes Aug 21, 2020

View reviewed changes

henrikbrixandersen approved these changes Aug 21, 2020

View reviewed changes

jfischer-no approved these changes Aug 21, 2020

View reviewed changes

nashif merged commit e32ecab into zephyrproject-rtos:master Aug 21, 2020

pabigot deleted the pabigot/20200810a branch August 24, 2020 13:15

pabigot mentioned this pull request Aug 31, 2020

checkpatch: Increase maximum line length to 100 nrfconnect/sdk-nrf#2696

Closed

pabigot mentioned this pull request Dec 4, 2020

Enforce all checkpatch warnings and move to 100 characters per line #30426

Closed

nashif mentioned this pull request Dec 4, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: update checkpatch based on current linux kernel version #27469

scripts: update checkpatch based on current linux kernel version #27469

pabigot commented Aug 10, 2020 •

edited

Loading

nashif Aug 20, 2020 •

edited

Loading

andrewboie Aug 20, 2020

henrikbrixandersen Aug 20, 2020

pabigot Aug 20, 2020

henrikbrixandersen Aug 20, 2020

pabigot Aug 20, 2020

pabigot commented Aug 20, 2020

nashif commented Aug 20, 2020

pabigot commented Aug 20, 2020 •

edited

Loading

andrewboie commented Aug 20, 2020

nashif commented Aug 20, 2020

andrewboie left a comment

andrewboie left a comment

pabigot commented Aug 20, 2020

henrikbrixandersen left a comment

scripts: update checkpatch based on current linux kernel version #27469

scripts: update checkpatch based on current linux kernel version #27469

Conversation

pabigot commented Aug 10, 2020 • edited Loading

nashif Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

andrewboie Aug 20, 2020

Choose a reason for hiding this comment

henrikbrixandersen Aug 20, 2020

Choose a reason for hiding this comment

pabigot Aug 20, 2020

Choose a reason for hiding this comment

henrikbrixandersen Aug 20, 2020

Choose a reason for hiding this comment

pabigot Aug 20, 2020

Choose a reason for hiding this comment

pabigot commented Aug 20, 2020

nashif commented Aug 20, 2020

pabigot commented Aug 20, 2020 • edited Loading

andrewboie commented Aug 20, 2020

nashif commented Aug 20, 2020

andrewboie left a comment

Choose a reason for hiding this comment

andrewboie left a comment

Choose a reason for hiding this comment

pabigot commented Aug 20, 2020

henrikbrixandersen left a comment

Choose a reason for hiding this comment

pabigot commented Aug 10, 2020 •

edited

Loading

nashif Aug 20, 2020 •

edited

Loading

pabigot commented Aug 20, 2020 •

edited

Loading