This patch series implements features to facilitate running checkpatch on the
entire kernel as part of automatic testing.
This is done by by adding a few small features to checkpatch and put these
features to use to implement support for a new Makefile environment variable
P={1,2} following the pattern of sparse and the C={1,2} variable. The basic
functionality + docs are in patch #1-4.
It also fixes a minor issue with "checkpatch --fix-inplace" found during testing
(patch #5).
The most important checkpatch feature added is the --ignore-cfg feature, which
takes a file argument and parses that file according to this minimal language:
# comments
line_len <n>
except checkpatch_type [files ...]
pervasive checkpatch_type1 [checkpatch_type2 ...]
With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
same directory as the source file. If that file exists, checkpatch will be run
with an implicit --strict and with the @ignore list expanded with content from
the configuration file. If it does not exist, make will simply silently ignore
the file.
Patches #6-7 enhances this behaviour to also scan the directories above a file
until a match for the --file parameter is found.
The idea is that the community can work to add checkpatch.cfg files to
directories, serving both as documentation and as a way for subsystem
maintainers to enforce policies and individual tastes as well as TODOs and/or
priorities, to make it easier for newcomers to contribute in this area. By
ignoring directories without such files, automation can start right away as it
is trivially possible to run errorless with P=2 for the entire kernel.
The patches includes a documentation file with some more details.
This patch set has evolved from an earlier implementation I made that was just a
wrapper script around checkpatch. That version have been used for a number of
years on a driver project I worked on where we had automatic checkin regression
testing. I extended that to also run checkpatch to avoid having to clean up
frequent unintended whitespace changes and style violations from others...
I have also tested this version on some directories I am familiar with. The
result of that work is available in two patch sets of 10 and 11 patches, but we
agreed that it would be better to post them as separate patch sets later.
Those patch sets illustrates how I picture the "flow" from just "reining in" the
checkpatch detections to actually fixing classes of checkpatch issues one by
one, while updating the checkpatch.cfg file(s) to have 0 errors or warnings at
any commit boundary.
The combined set is available here:
git://github.com/knuto/linux.git branch checkpatch
Comments and suggestions appreciated!
Thanks,
Knut
Knut Omang (7):
checkpatch: Implement new --ignore-cfg parameter
kbuild: Add P= command line flag to run checkpatch
checkpatch: Add a few convenience options to disable/modify features
Documentation: Add documentation for the new P= Makefile option
checkpatch: Improve --fix-inplace for TABSTOP
checkpatch: Make --ignore-cfg look recursively for the file
Documentation: Update checkpatch --ignore-cfg description
Documentation/dev-tools/index.rst | 1 +-
Documentation/dev-tools/run-checkpatch.rst | 109 ++++++++++++++++++++++-
Makefile | 20 +++-
scripts/Makefile.build | 13 +++-
scripts/checkpatch.pl | 108 +++++++++++++++++++++-
5 files changed, 249 insertions(+), 2 deletions(-)
create mode 100644 Documentation/dev-tools/run-checkpatch.rst
base-commit: bebc6082da0a9f5d47a1ea2edc099bf671058bd4
--
git-series 0.9.1
From 1584991906081394327@xxx Fri Nov 24 23:27:52 +0000 2017
X-GM-THRID: 1584936292664143894
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
The initial version of the logic for --ignore-cfg supported looking
for the file in the current directory, and if not found, look
in the directory of the source file.
With this change, in the case of a file name with no directory specification,
and for an in-kernel file, checkpatch will iterate upwards in the directories
above, looking for the same file until either hitting the top of the
tree or finding a match.
This should make the P={1,2} make target more useful for maintainers,
as a start of functionality for checkpatch checking that
can be globally enabled for a subsystem.
Signed-off-by: Knut Omang <[email protected]>
---
scripts/checkpatch.pl | 44 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 38 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c17178e..a276eca 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2186,15 +2186,47 @@ sub pos_last_openparen {
return length(expand_tabs(substr($line, 0, $last_openparen))) + 1;
}
+
# Checkpatch suppression list configuration file support
#
# See Documentation/dev-tools/run-checkpatch.rst
#
-sub parse_ignore_cfg_file {
- defined($ignore_cfg_file) || return 1;
+
+# Usage: find_ignore_cfg_file(filename,ignorefilename)
+# If filename contains a directory, use it directly with no further attempts,
+# If no directory is specified, whether relative or absolute,
+# look for 'filename' in the following order until a match is found:
+# 1) current dir,
+# 2) the directory of the source file
+# 3) if an in-source source-file (same source tree as this script)
+# look in directories above for the first match.
+#
+sub find_ignore_cfg_file {
my $path = shift(@_);
+ my $ipath = shift(@_);
+ my $ifile = basename($ipath);
my $filename = basename($path);
my $dir = dirname($path);
+ my $root = dirname($D);
+ ( -f $ipath ) && return ($filename, $ipath);
+ ( $ipath =~ m/\// ) && return ($filename,"");
+
+ do {
+ $ipath = "$dir/$ifile";
+ #print "*** Trying $ipath ***\n";
+ ( -f $ipath ) && return ($filename, $ipath);
+ $dir = dirname($dir);
+ } while ( $dir =~ m/^$root/ && ! -f $ifile );
+ return ($filename,"");
+}
+
+
+sub parse_ignore_cfg_file {
+ my $path = shift(@_);
+ my $filename; # The file to check
+ my $ifile; # The ignore file name
+ my $ignfile; # The ignore file handle
+ defined($ignore_cfg_file) || return 1;
my %IgnoreCfgKeywords = (
'except' => sub { my $type = shift(@_);
grep( /^$filename$/, @_ ) && push(@ignore, $type);
@@ -2202,11 +2234,11 @@ sub parse_ignore_cfg_file {
'pervasive' => sub { push(@ignore, @_); },
'line_len' => sub { $max_line_length = shift(@_); }
);
- my $ignfile;
- ( -f $ignore_cfg_file ) || ( $ignore_cfg_file = "$dir/$ignore_cfg_file" );
- ( ! -f $ignore_cfg_file ) && return 0;
- open($ignfile, '<', "$ignore_cfg_file") || return 0;
+ ($filename, $ifile) = find_ignore_cfg_file($path, $ignore_cfg_file);
+ ( $ifile eq "" ) && return 0;
+ open($ignfile, '<', "$ifile") || return 0;
+ #print "*** Found $ifile ***\n";
($#_ >= 0) &&
die "$P: The --ignore-cfg option is only supported with one source file at a time!\n";
--
git-series 0.9.1
From 1584189704233396528@xxx Thu Nov 16 02:57:13 +0000 2017
X-GM-THRID: 1583929063651093838
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
When running checkpatch with --ignore-cfg it will now traverse the directories
above the source file until a match is found.
Reflect this enhanced behaviour in the documentation.
Signed-off-by: Knut Omang <[email protected]>
---
Documentation/dev-tools/run-checkpatch.rst | 48 ++++++++++++-----------
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/Documentation/dev-tools/run-checkpatch.rst b/Documentation/dev-tools/run-checkpatch.rst
index c72f818..566d8c6 100644
--- a/Documentation/dev-tools/run-checkpatch.rst
+++ b/Documentation/dev-tools/run-checkpatch.rst
@@ -26,17 +26,20 @@ number of challenges:
This is the purpose of supplying the option ``--ignore-cfg checkpatch.cfg`` to
``scripts/checkpatch.pl``. It will then look for a file named ``checkpatch.cfg``
in the current directory or alternatively in the directory of the source
-file. If that file exists, checkpatch parses a set of rules from it, and use
-them to determine how to invoke checkpatch for a particular file. The kernel
-Makefile system supports using this feature as an integrated part of compiling
-the code.
+file. If neither is found, and the file is within the kernel tree, checkpatch
+will recursively look for a file with the same name in the directories above
+until a file is found or the top of the tree is reached.
+
+If a match is found, checkpatch parses a set of rules from it, and use them to
+determine how to invoke checkpatch for a particular file. The kernel Makefile
+system supports using this feature as an integrated part of compiling the code.
The ignore configuration file
-----------------------------
The ignore configuration file can be used to set policies and "rein in"
-checkpatch errors piece by piece for a particular subsystem or driver.
-The the following syntax is supported::
+checkpatch errors piece by piece for a particular subsystem or driver. The the
+following syntax is supported::
# comments
line_len <n>
@@ -55,9 +58,9 @@ exceptions.
Running checkpatch from make
----------------------------
-You can run checkpatch subject to rules defined in ``checkpatch.cfg`` in the
-directory of the source file by using "make P=1" to run checkpatch on all files
-that gets recompiled, or "make P=2" to run checkpatch on all source files.
+You can run checkpatch subject to rules defined in the closest matching
+``checkpatch.cfg`` file in the tree by using "make P=1" to run checkpatch on all
+files that gets recompiled, or "make P=2" to run checkpatch on all source files.
A make variable ``PF`` allows passing additional parameters to
checkpatch.pl. You can for instance use::
@@ -70,15 +73,15 @@ selectively enabling of types of errors via changes to the local
``checkpatch.cfg``, and you can focus on fixing up errors subsystem or driver by
driver on a type by type basis.
-By default checkpatch will skip all files in directories without a
-checkpatch.cfg file when invoked with the --ignore-cfg parameter. This is to
-allow builds with P=2 to pass even for subsystems that has not yet done anything
-to rein in checkpatch errors. At some point when all subsystems and drivers
-either have fixed all checkpatch errors or added proper checkpatch.cfg files,
-this can be changed.
+When invoked with the --ignore-cfg parameter, by default checkpatch will skip
+all files in directories without a matching checkpatch.cfg according to the
+algorithm described above. This is to allow builds with P=2 to pass even for
+subsystems that has not yet done anything to rein in checkpatch errors. At some
+point when all subsystems and drivers either have fixed all checkpatch errors or
+added proper checkpatch.cfg files, this can be changed.
-To force checkpatch to run a full run in directories without a checkpatch.cfg
-file as well, use::
+To force checkpatch to run a full run in directories without a
+checkpatch.cfg file as well, use::
make P=2 PF="--req-ignore-cfg"
@@ -96,10 +99,11 @@ with the -k option to ``make`` to let it continue upon errors.
Ever tightening checkpatch rules
--------------------------------
-Commit the changes to checkpatch.cfg together with the code changes that fixes a
-particular type of issue, this will allow automatic checkpatch testing. This way
-we can ensure that new errors of that particular type do not inadvertently sneak
-in again! This can be done at any subsystem or module maintainer's discretion
-and at the right time without having to do it all at the same time.
+Commit the changes to the relevant checkpatch.cfg together with the code changes
+that fixes a particular type of issue, this will allow automatic checkpatch
+testing. This way we can ensure that new errors of that particular type do not
+inadvertently sneak in again! This can be done at any subsystem or module
+maintainer's discretion and at the right time without having to do it all at the
+same time.
Before submitting your changes, verify that "make P=2" passes with no errors.
--
git-series 0.9.1
From 1583398936866587686@xxx Tue Nov 07 09:28:18 +0000 2017
X-GM-THRID: 1583398936866587686
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Add interpretation of a new environment variable P={1,2} in spirit of the
C= option, but executing checkpatch instead of sparse.
Signed-off-by: Knut Omang <[email protected]>
Reviewed-by: Håkon Bugge <[email protected]>
Acked-by: Åsmund Østvold <[email protected]>
---
Makefile | 20 +++++++++++++++++++-
scripts/Makefile.build | 13 +++++++++++++
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index ccd9818..eb4bca9 100644
--- a/Makefile
+++ b/Makefile
@@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC
KBUILD_CHECKSRC = 0
endif
+# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg
+#
+# Use 'make P=1' to enable checking of only re-compiled files.
+# Use 'make P=2' to enable checking of *all* source files, regardless
+#
+# See the file "Documentation/dev-tools/run-checkpatch.rst" for more details,
+#
+ifeq ("$(origin P)", "command line")
+ KBUILD_CHECKPATCH = $(P)
+endif
+ifndef KBUILD_CHECKPATCH
+ KBUILD_CHECKPATCH = 0
+endif
+
# Use make M=dir to specify directory of external module to build
# Old syntax make ... SUBDIRS=$PWD is still supported
# Setting the environment variable KBUILD_EXTMOD take precedence
@@ -340,7 +354,7 @@ ifeq ($(MAKECMDGOALS),)
endif
export KBUILD_MODULES KBUILD_BUILTIN
-export KBUILD_CHECKSRC KBUILD_SRC KBUILD_EXTMOD
+export KBUILD_CHECKSRC KBUILD_CHECKPATCH KBUILD_SRC KBUILD_EXTMOD
# We need some generic definitions (do not try to remake the file).
scripts/Kbuild.include: ;
@@ -363,9 +377,12 @@ DEPMOD = /sbin/depmod
PERL = perl
PYTHON = python
CHECK = sparse
+CHECKP = scripts/checkpatch.pl
CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
-Wbitwise -Wno-return-void $(CF)
+CHECKPFLAGS := --quiet --show-types --emacs \
+ --ignore-cfg checkpatch.cfg --file $(PF)
NOSTDINC_FLAGS =
CFLAGS_MODULE =
AFLAGS_MODULE =
@@ -419,6 +436,7 @@ export ARCH SRCARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
export CPP AR NM STRIP OBJCOPY OBJDUMP HOSTLDFLAGS HOST_LOADLIBES
export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
+export CHECKP CHECKPFLAGS
export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bb831d4..cfc540a 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -109,6 +109,17 @@ ifneq ($(KBUILD_CHECKSRC),0)
endif
endif
+# Run per-directory/per-file specific checkpatch testing:
+ifneq ($(KBUILD_CHECKPATCH),0)
+ ifeq ($(KBUILD_CHECKPATCH),2)
+ quiet_cmd_force_checkpatch = CHECKP $<
+ cmd_force_checkpatch = $(srctree)/$(CHECKP) $(POPTS) $< $(CHECKPFLAGS) ;
+ else
+ quiet_cmd_checkpatch = CHECKP $<
+ cmd_checkpatch = $(srctree)/$(CHECKP) $(POPTS) $< $(CHECKPFLAGS) ;
+ endif
+endif
+
# Do section mismatch analysis for each module/built-in.o
ifdef CONFIG_DEBUG_SECTION_MISMATCH
cmd_secanalysis = ; scripts/mod/modpost $@
@@ -290,6 +301,7 @@ objtool_dep = $(objtool_obj) \
define rule_cc_o_c
$(call echo-cmd,checksrc) $(cmd_checksrc) \
+ $(call echo-cmd,checkpatch) $(cmd_checkpatch) \
$(call cmd_and_fixdep,cc_o_c) \
$(cmd_modversions_c) \
$(call echo-cmd,objtool) $(cmd_objtool) \
@@ -312,6 +324,7 @@ endif
# Built-in and composite module parts
$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
$(call cmd,force_checksrc)
+ $(call cmd,force_checkpatch)
$(call if_changed_rule,cc_o_c)
# Single-part modules are special since we need to mark them in $(MODVERDIR)
--
git-series 0.9.1
From 1585216797093825186@xxx Mon Nov 27 11:02:25 +0000 2017
X-GM-THRID: 1585216797093825186
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread