| Message ID | 20210827095210.23602-2-rpalethorpe@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] Vendor checkpatch.pl from kernel next-20210826 | expand |
Hi Richie, > Add another check command type. CHECK_NOFLAGS just takes the source > file name as an argument. By default it is set to > scripts/checkpatch.pl which is probably the only thing we want to use > it for. OTOH you can set it to clang-tidy instead. The same we could do with checkbashisms for tests using new shell API. > It is run with '-' because of the large number of errors it presently > produces. Also of course, check errors are not actually fatal. If we > wish to stop on errors in the future (e.g. for CI) then a "strict" > option can be introduced. Thanks for doing this! Could it be possible to run it only for tests which use new API? Otherwise it takes long time before we can use it in CI due lots of output from tests using legacy API: tst_record_childstatus.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 => tst_record_childstatus.c uses test.h. Kind regards, Petr
Hi! > > Add another check command type. CHECK_NOFLAGS just takes the source > > file name as an argument. By default it is set to > > scripts/checkpatch.pl which is probably the only thing we want to use > > it for. OTOH you can set it to clang-tidy instead. > The same we could do with checkbashisms for tests using new shell API. This would be a good idea as well. > > It is run with '-' because of the large number of errors it presently > > produces. Also of course, check errors are not actually fatal. If we > > wish to stop on errors in the future (e.g. for CI) then a "strict" > > option can be introduced. > > Thanks for doing this! > > Could it be possible to run it only for tests which use new API? Otherwise it > takes long time before we can use it in CI due lots of output from tests using > legacy API: > > tst_record_childstatus.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 > => tst_record_childstatus.c uses test.h. Well I do see this to be mainly targetting development so that everyone can do 'make check' before submitting. So I would like to get this merged ASAP and we can try to make it work with CI later on.
Hi all, > Well I do see this to be mainly targetting development so that everyone > can do 'make check' before submitting. So I would like to get this > merged ASAP and we can try to make it work with CI later on. Sure! Running make check for particular test substitute the need to do anything with the patchset. Kind regards, Petr
Hi Cyril, Richie,
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Waiting for the others to have time to review it before merge.
Kind regards,
Petr
Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
I guess that we should update the Test Contribution Checklist in the
test-writing-guidelines.txt and remove the point 2 about checkpath and
move the point 6 about make check to the second place in the list.
Hi Cyril, Richie, Thanks both, merged! > I guess that we should update the Test Contribution Checklist in the > test-writing-guidelines.txt and remove the point 2 about checkpath and > move the point 6 about make check to the second place in the list. I plan to update docs in a patchset which adds checkbashism. Kind regards, Petr
diff --git a/include/mk/env_post.mk b/include/mk/env_post.mk index 4722da907..eb76b38a4 100644 --- a/include/mk/env_post.mk +++ b/include/mk/env_post.mk @@ -92,6 +92,7 @@ endif CHECK_TARGETS ?= $(addprefix check-,$(notdir $(patsubst %.c,%,$(sort $(wildcard $(abs_srcdir)/*.c))))) CHECK_TARGETS := $(filter-out $(addprefix check-, $(FILTER_OUT_MAKE_TARGETS)), $(CHECK_TARGETS)) CHECK ?= $(abs_top_srcdir)/tools/sparse/sparse-ltp +CHECK_NOFLAGS ?= $(abs_top_srcdir)/scripts/checkpatch.pl -f --no-tree --terse --no-summary --ignore CONST_STRUCT,VOLATILE,SPLIT_STRING ifeq ($(CHECK),$(abs_top_srcdir)/tools/sparse/sparse-ltp) CHECK_DEPS += $(CHECK) diff --git a/include/mk/rules.mk b/include/mk/rules.mk index 2a04b2b67..6bd184841 100644 --- a/include/mk/rules.mk +++ b/include/mk/rules.mk @@ -41,8 +41,10 @@ endif .PHONY: $(CHECK_TARGETS) $(CHECK_TARGETS): check-%: %.c ifdef VERBOSE - $(CHECK) $(CHECK_FLAGS) $(CPPFLAGS) $(CFLAGS) $< + -$(CHECK_NOFLAGS) $< + -$(CHECK) $(CHECK_FLAGS) $(CPPFLAGS) $(CFLAGS) $< else - @$(CHECK) $(CHECK_FLAGS) $(CPPFLAGS) $(CFLAGS) $< @echo CHECK $(target_rel_dir)$< + @-$(CHECK_NOFLAGS) $< + @-$(CHECK) $(CHECK_FLAGS) $(CPPFLAGS) $(CFLAGS) $< endif
Add another check command type. CHECK_NOFLAGS just takes the source file name as an argument. By default it is set to scripts/checkpatch.pl which is probably the only thing we want to use it for. OTOH you can set it to clang-tidy instead. It is run with '-' because of the large number of errors it presently produces. Also of course, check errors are not actually fatal. If we wish to stop on errors in the future (e.g. for CI) then a "strict" option can be introduced. Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> Suggested-by: Cyril Hrubis <chrubis@suse.cz> --- include/mk/env_post.mk | 1 + include/mk/rules.mk | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-)