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

[Namhyung Kim]

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?

> >
> > 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` ?

> >
> > 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?

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
> >
>