[PATCH V2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf

[Athira Rajeev]

> On 09-Oct-2023, at 10:38 AM, Namhyung Kim <namhyung at kernel.org> wrote:
> 
> Hello,
> 
> Sorry for the late reply.
> 
> On Thu, Oct 5, 2023 at 8:27 AM Athira Rajeev
> <atrajeev at linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>>> On 29-Sep-2023, at 12:19 PM, Athira Rajeev <atrajeev at linux.vnet.ibm.com> wrote:
>>> 
>>> Add rule in new Makefile "tests/Makefile.tests" for running
>>> shellcheck on shell test scripts. This automates below shellcheck
>>> into the build.
>>> 
>>> $ for F in $(find tests/shell/ -perm -o=x -name '*.sh'); do shellcheck -S warning $F; done
>>> 
>>> Condition for shellcheck is added in Makefile.perf to avoid build
>>> breakage in the absence of shellcheck binary. Update Makefile.perf
>>> to contain new rule for "SHELLCHECK_TEST" which is for making
>>> shellcheck test as a dependency on perf binary. Added
>>> "tests/Makefile.tests" to run shellcheck on shellscripts in
>>> tests/shell. The make rule "SHLLCHECK_RUN" ensures that, every
>>> time during make, shellcheck will be run only on modified files
>>> during subsequent invocations. By this, if any newly added shell
>>> scripts or fixes in existing scripts breaks coding/formatting
>>> style, it will get captured during the perf build.
>>> 
>>> Example build failure with present scripts in tests/shell:
>>> 
>>> INSTALL libsubcmd_headers
>>> INSTALL libperf_headers
>>> INSTALL libapi_headers
>>> INSTALL libsymbol_headers
>>> INSTALL libbpf_headers
>>> make[3]: *** [/root/athira/namhyung/perf-tools-next/tools/perf/tests/Makefile.tests:17: output/tests/shell/record_sideband.dep] Error 1
>>> make[3]: *** Waiting for unfinished jobs....
>>> make[3]: *** [/root/athira/namhyung/perf-tools-next/tools/perf/tests/Makefile.tests:17: output/tests/shell/test_arm_coresight.dep] Error 1
>>> make[3]: *** [/root/athira/namhyung/perf-tools-next/tools/perf/tests/Makefile.tests:17: output/tests/shell/lock_contention.dep] Error 1
>>> make[2]: *** [Makefile.perf:675: SHELLCHECK_TEST] Error 2
>>> make[1]: *** [Makefile.perf:238: sub-make] Error 2
>>> make: *** [Makefile:70: all] Error 2
>>> 
>>> After this, for testing, changed "tests/shell/record.sh" to
>>> break shellcheck format. In the next make run, it is
>>> also captured:
> 
> Where can I see the actual failure messages?
Hi Namhyung,
Thanks for the review comments.

Example, with current tree, we have these format issues:

 GENSKEL /root/athira/linux/tools/perf/util/bpf_skel/kwork_trace.skel.h
CC bench/uprobe.o
CC util/header.o
LD bench/perf-in.o
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/stat_all_metricgroups.dep] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/record_sideband.dep] Error 1
CC util/bpf_counter.o
CC util/bpf_counter_cgroup.o
CC util/bpf_ftrace.o
CC util/bpf_off_cpu.o
CC util/bpf-filter.o
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:15: output/tests/shell/test_arm_coresight.dep] Error 1
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:15: output/tests/shell/lock_contention.dep] Error 1
make[2]: *** [Makefile.perf:679: SHELLCHECK_TEST] Error 2
make[2]: *** Waiting for unfinished jobs....
LD util/perf-in.o
LD perf-in.o
make[1]: *** [Makefile.perf:242: sub-make] Error 2
make: *** [Makefile:70: all] Error 2

The actual fail can be seen here:

# cat output/tests/shell/stat_all_metricgroups.dep.log 

In ./tests/shell/stat_all_metricgroups.sh line 7:
function ParanoidAndNotRoot()
^-- SC2112: 'function' keyword is non-standard. Delete it.

For more information:
https://www.shellcheck.net/wiki/SC2112 -- 'function' keyword is non-standar...
> 


>>> 
>>> make[3]: *** [/root/athira/namhyung/perf-tools-next/tools/perf/tests/Makefile.tests:17: output/tests/shell/record_sideband.dep] Error 1
>>> make[3]: *** Waiting for unfinished jobs....
>>> make[3]: *** [/root/athira/namhyung/perf-tools-next/tools/perf/tests/Makefile.tests:17: output/tests/shell/record.dep] Error 1
>>> make[3]: *** [/root/athira/namhyung/perf-tools-next/tools/perf/tests/Makefile.tests:17: output/tests/shell/test_arm_coresight.dep] Error 1
>>> make[3]: *** [/root/athira/namhyung/perf-tools-next/tools/perf/tests/Makefile.tests:17: output/tests/shell/lock_contention.dep] Error 1
>>> make[2]: *** [Makefile.perf:675: SHELLCHECK_TEST] Error 2
>>> make[1]: *** [Makefile.perf:238: sub-make] Error 2
>>> make: *** [Makefile:70: all] Error 2
> 
> So is this reported at build time before I run the test command?
> I'm ok with that but maybe I need to build it even though I know
> some test is broken.  Can we have an option to do that like with
> `make NO_SHELLCHECK=1` ?

Yes, agree Namhyung. Valid point.
I will add that in V3

> 
>>> 
>>> Signed-off-by: Athira Rajeev <atrajeev at linux.vnet.ibm.com>
>>> ---
>>> Changelog:
>>> v1 -> v2:
>>> Version1 had shellcheck in feature check which is not
>>> required since shellcheck is already a binary. Presence
>>> of binary can be checked using:
>>> $(shell command -v shellcheck)
>>> Addressed these changes as suggested by Namhyung in V2
>>> Feature test logic is removed in V2. Also added example
>>> for build breakage when shellcheck fails in commit message
>> 
>> Hi All,
>> 
>> Looking for feedback on this patch
>> 
>> Thanks
>> Athira
>>> 
>>> tools/perf/Makefile.perf        | 14 +++++++++++++-
>>> tools/perf/tests/Makefile.tests | 24 ++++++++++++++++++++++++
>>> 2 files changed, 37 insertions(+), 1 deletion(-)
>>> create mode 100644 tools/perf/tests/Makefile.tests
>>> 
>>> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
>>> index 98604e396ac3..56a66ca253ab 100644
>>> --- a/tools/perf/Makefile.perf
>>> +++ b/tools/perf/Makefile.perf
>>> @@ -667,7 +667,18 @@ $(PERF_IN): prepare FORCE
>>> $(PMU_EVENTS_IN): FORCE prepare
>>> $(Q)$(MAKE) -f $(srctree)/tools/build/Makefile.build dir=pmu-events obj=pmu-events
>>> 
>>> -$(OUTPUT)perf: $(PERFLIBS) $(PERF_IN) $(PMU_EVENTS_IN)
>>> +# Runs shellcheck on perf test shell scripts
>>> +
>>> +SHELLCHECK := $(shell command -v shellcheck)
>>> +ifneq ($(SHELLCHECK),)
>>> +SHELLCHECK_TEST: FORCE prepare
>>> + $(Q)$(MAKE) -f $(srctree)/tools/perf/tests/Makefile.tests
>>> +else
>>> +SHELLCHECK_TEST:
>>> + @:
>>> +endif
>>> +
>>> +$(OUTPUT)perf: $(PERFLIBS) $(PERF_IN) $(PMU_EVENTS_IN) SHELLCHECK_TEST
>>> $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) \
>>> $(PERF_IN) $(PMU_EVENTS_IN) $(LIBS) -o $@
>>> 
>>> @@ -1130,6 +1141,7 @@ bpf-skel-clean:
>>> $(call QUIET_CLEAN, bpf-skel) $(RM) -r $(SKEL_TMP_OUT) $(SKELETONS)
>>> 
>>> clean:: $(LIBAPI)-clean $(LIBBPF)-clean $(LIBSUBCMD)-clean $(LIBSYMBOL)-clean $(LIBPERF)-clean fixdep-clean python-clean bpf-skel-clean tests-coresight-targets-clean
>>> + $(Q)$(MAKE) -f $(srctree)/tools/perf/tests/Makefile.tests clean
>>> $(call QUIET_CLEAN, core-objs)  $(RM) $(LIBPERF_A) $(OUTPUT)perf-archive $(OUTPUT)perf-iostat $(LANG_BINDINGS)
>>> $(Q)find $(or $(OUTPUT),.) -name '*.o' -delete -o -name '\.*.cmd' -delete -o -name '\.*.d' -delete
>>> $(Q)$(RM) $(OUTPUT).config-detected
>>> diff --git a/tools/perf/tests/Makefile.tests b/tools/perf/tests/Makefile.tests
>>> new file mode 100644
>>> index 000000000000..8011e99768a3
>>> --- /dev/null
>>> +++ b/tools/perf/tests/Makefile.tests
>>> @@ -0,0 +1,24 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +# Athira Rajeev <atrajeev at linux.vnet.ibm.com>, 2023
>>> +
>>> +PROGS = $(subst ./,,$(shell find tests/shell -perm -o=x -type f -name '*.sh'))
>>> +DEPS = $(addprefix output/,$(addsuffix .dep,$(basename $(PROGS))))
>>> +DIRS = $(shell echo $(dir $(DEPS)) | xargs -n1 | sort -u | xargs)
>>> +
>>> +.PHONY: all
>>> +all: SHELLCHECK_RUN
>>> + @:
>>> +
>>> +SHELLCHECK_RUN: $(DEPS) $(DIRS)
>>> +
>>> +output/%.dep: %.sh | $(DIRS)
>>> + $(call rule_mkdir)
>>> + $(eval input_file := $(subst output/,./,$(patsubst %.dep, %.sh, $@)))
>>> + $(Q)$(call frecho-cmd,test)@shellcheck -S warning ${input_file} 1> $@.log && ([[ ! -s $@.log ]])
> 
> What is the last part?  I guess it checks if shellcheck found no errors.
> Can we just check the exit code of the shellcheck?  Is there a case
> it didn't work?

It checks to see if the redirected file has any warnings ( if it is not empty ).
I had tried exit code, but it reported false fails with using exit code

Diff I used:

diff --git a/tools/perf/tests/Makefile.tests b/tools/perf/tests/Makefile.tests
index 8011e99768a3..b3089f46ba6f 100644
--- a/tools/perf/tests/Makefile.tests
+++ b/tools/perf/tests/Makefile.tests
@@ -14,7 +14,7 @@ SHELLCHECK_RUN: $(DEPS) $(DIRS)
output/%.dep: %.sh | $(DIRS)
$(call rule_mkdir)
$(eval input_file := $(subst output/,./,$(patsubst %.dep, %.sh, $@)))
- $(Q)$(call frecho-cmd,test)@shellcheck -S warning ${input_file} 1> $@.log && ([[ ! -s $@.log ]])
+ $(Q)$(call frecho-cmd,test)@shellcheck -S warning ${input_file} 1> $@.log && ([[ ! $? ]])
$(Q)$(call frecho-cmd,test)@touch $@
$(Q)$(call frecho-cmd,test)@rm -rf $@.log
$(DIRS):


  INSTALL libsubcmd_headers
INSTALL libsymbol_headers
INSTALL libperf_headers
INSTALL libapi_headers
INSTALL libbpf_headers
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/stat_all_metricgroups.dep] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/trace+probe_vfs_getname.dep] Error 1
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/record_sideband.dep] Error 1
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/test_java_symbol.dep] Error 1
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/test_brstack.dep] Error 1
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/test_task_analyzer.dep] Error 1
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/test_arm_coresight.dep] Error 1
make[3]: *** [/root/athira/linux/tools/perf/tests/Makefile.tests:17: output/tests/shell/lock_contention.dep] Error 1
make[2]: *** [Makefile.perf:679: SHELLCHECK_TEST] Error 2
make[1]: *** [Makefile.perf:242: sub-make] Error 2
make: *** [Makefile:70: all] Error 2

I think it happens since we run the checks parallely.
Hence checking the corresponding log for the particular script will give us accuracy with the results.

Thanks
Athira
> 
> Thanks,
> Namhyung
> 
> 
>>> + $(Q)$(call frecho-cmd,test)@touch $@
>>> + $(Q)$(call frecho-cmd,test)@rm -rf $@.log
>>> +$(DIRS):
>>> + @mkdir -p $@
>>> +
>>> +clean:
>>> + @rm -rf output
>>> --
>>> 2.31.1