* [PATCH V2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf @ 2023-09-29 6:49 Athira Rajeev 2023-10-05 14:37 ` Athira Rajeev 0 siblings, 1 reply; 4+ messages in thread From: Athira Rajeev @ 2023-09-29 6:49 UTC (permalink / raw) To: acme, jolsa, adrian.hunter, irogers, namhyung Cc: linux-perf-users, linuxppc-dev, maddy, atrajeev, kjain, disgoel 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: 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 Signed-off-by: Athira Rajeev <atrajeev@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 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@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 ]]) + $(Q)$(call frecho-cmd,test)@touch $@ + $(Q)$(call frecho-cmd,test)@rm -rf $@.log +$(DIRS): + @mkdir -p $@ + +clean: + @rm -rf output -- 2.31.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH V2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf 2023-09-29 6:49 [PATCH V2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf Athira Rajeev @ 2023-10-05 14:37 ` Athira Rajeev 2023-10-09 5:08 ` Namhyung Kim 0 siblings, 1 reply; 4+ messages in thread From: Athira Rajeev @ 2023-10-05 14:37 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter, Ian Rogers, Namhyung Kim Cc: linux-perf-users, linuxppc-dev, Madhavan Srinivasan, Kajol Jain, disgoel > On 29-Sep-2023, at 12:19 PM, Athira Rajeev <atrajeev@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: > > 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 > > Signed-off-by: Athira Rajeev <atrajeev@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@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 ]]) > + $(Q)$(call frecho-cmd,test)@touch $@ > + $(Q)$(call frecho-cmd,test)@rm -rf $@.log > +$(DIRS): > + @mkdir -p $@ > + > +clean: > + @rm -rf output > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf 2023-10-05 14:37 ` Athira Rajeev @ 2023-10-09 5:08 ` Namhyung Kim 2023-10-12 9:53 ` Athira Rajeev 0 siblings, 1 reply; 4+ messages in thread From: Namhyung Kim @ 2023-10-09 5:08 UTC (permalink / raw) To: Athira Rajeev Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter, Ian Rogers, linux-perf-users, linuxppc-dev, Madhavan Srinivasan, Kajol Jain, disgoel Hello, Sorry for the late reply. On Thu, Oct 5, 2023 at 8:27 AM Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > > > > On 29-Sep-2023, at 12:19 PM, Athira Rajeev <atrajeev@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@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@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 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf 2023-10-09 5:08 ` Namhyung Kim @ 2023-10-12 9:53 ` Athira Rajeev 0 siblings, 0 replies; 4+ messages in thread From: Athira Rajeev @ 2023-10-12 9:53 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter, Ian Rogers, linux-perf-users, linuxppc-dev, Madhavan Srinivasan, Kajol Jain, disgoel > On 09-Oct-2023, at 10:38 AM, Namhyung Kim <namhyung@kernel.org> wrote: > > Hello, > > Sorry for the late reply. > > On Thu, Oct 5, 2023 at 8:27 AM Athira Rajeev > <atrajeev@linux.vnet.ibm.com> wrote: >> >> >> >>> On 29-Sep-2023, at 12:19 PM, Athira Rajeev <atrajeev@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@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@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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-10-12 9:54 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-29 6:49 [PATCH V2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf Athira Rajeev 2023-10-05 14:37 ` Athira Rajeev 2023-10-09 5:08 ` Namhyung Kim 2023-10-12 9:53 ` Athira Rajeev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).