* [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).