* [PATCH V3] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
@ 2023-10-13 7:36 Athira Rajeev
2023-10-22 7:12 ` Athira Rajeev
2023-10-23 10:44 ` James Clark
0 siblings, 2 replies; 4+ messages in thread
From: Athira Rajeev @ 2023-10-13 7:36 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
The exact failure log can be found in:
output/tests/shell/record.dep.log file
This is reported at build time. To be able to go ahead with
the build or disable shellcheck even though it is known that
some test is broken, add a "NO_SHELLCHECK" option. Example:
make NO_LIBTRACEEVENT=1 NO_SHELLCHECK=1
INSTALL libsubcmd_headers
INSTALL libsymbol_headers
INSTALL libperf_headers
INSTALL libapi_headers
INSTALL libbpf_headers
PERF_VERSION = 6.6.rc1.g7108a40e02ae
GEN perf-iostat
GEN perf-archive
CC util/header.o
LD util/perf-in.o
LD perf-in.o
LINK perf
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
changelog:
v2 -> v3:
Add option "NO_SHELLCHECK". This will allow to go ahead
with the build or disable shellcheck even though it is
known that some test is broken
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 | 20 +++++++++++++++++++-
tools/perf/tests/Makefile.tests | 24 ++++++++++++++++++++++++
2 files changed, 43 insertions(+), 1 deletion(-)
create mode 100644 tools/perf/tests/Makefile.tests
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 456872ac410d..bb49eb8b0d43 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -134,6 +134,8 @@ include ../scripts/utilities.mak
# x86 instruction decoder - new instructions test
#
# Define GEN_VMLINUX_H to generate vmlinux.h from the BTF.
+#
+# Define NO_SHELLCHECK if you do not want to run shellcheck during build
# As per kernel Makefile, avoid funny character set dependencies
unexport LC_ALL
@@ -671,7 +673,22 @@ $(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)
+ifeq ($(NO_SHELLCHECK),1)
+SHELLCHECK :=
+endif
+
+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 $@
@@ -1134,6 +1151,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 V3] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
2023-10-13 7:36 [PATCH V3] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf Athira Rajeev
@ 2023-10-22 7:12 ` Athira Rajeev
2023-10-23 10:44 ` James Clark
1 sibling, 0 replies; 4+ messages in thread
From: Athira Rajeev @ 2023-10-22 7:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter, Ian Rogers,
Namhyung Kim
Cc: Kajol Jain, linux-perf-users, Madhavan Srinivasan, Disha Goel,
linuxppc-dev
> On 13-Oct-2023, at 1:06 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
>
> The exact failure log can be found in:
> output/tests/shell/record.dep.log file
>
> This is reported at build time. To be able to go ahead with
> the build or disable shellcheck even though it is known that
> some test is broken, add a "NO_SHELLCHECK" option. Example:
>
> make NO_LIBTRACEEVENT=1 NO_SHELLCHECK=1
>
> INSTALL libsubcmd_headers
> INSTALL libsymbol_headers
> INSTALL libperf_headers
> INSTALL libapi_headers
> INSTALL libbpf_headers
> PERF_VERSION = 6.6.rc1.g7108a40e02ae
> GEN perf-iostat
> GEN perf-archive
> CC util/header.o
> LD util/perf-in.o
> LD perf-in.o
> LINK perf
>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
> changelog:
> v2 -> v3:
> Add option "NO_SHELLCHECK". This will allow to go ahead
> with the build or disable shellcheck even though it is
> known that some test is broken
>
> 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 review comments on this patch
Thanks
Athira
>
> tools/perf/Makefile.perf | 20 +++++++++++++++++++-
> tools/perf/tests/Makefile.tests | 24 ++++++++++++++++++++++++
> 2 files changed, 43 insertions(+), 1 deletion(-)
> create mode 100644 tools/perf/tests/Makefile.tests
>
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index 456872ac410d..bb49eb8b0d43 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -134,6 +134,8 @@ include ../scripts/utilities.mak
> # x86 instruction decoder - new instructions test
> #
> # Define GEN_VMLINUX_H to generate vmlinux.h from the BTF.
> +#
> +# Define NO_SHELLCHECK if you do not want to run shellcheck during build
>
> # As per kernel Makefile, avoid funny character set dependencies
> unexport LC_ALL
> @@ -671,7 +673,22 @@ $(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)
> +ifeq ($(NO_SHELLCHECK),1)
> +SHELLCHECK :=
> +endif
> +
> +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 $@
>
> @@ -1134,6 +1151,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 V3] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
2023-10-13 7:36 [PATCH V3] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf Athira Rajeev
2023-10-22 7:12 ` Athira Rajeev
@ 2023-10-23 10:44 ` James Clark
2023-11-07 6:40 ` Athira Rajeev
1 sibling, 1 reply; 4+ messages in thread
From: James Clark @ 2023-10-23 10:44 UTC (permalink / raw)
To: Athira Rajeev
Cc: linux-perf-users, linuxppc-dev, maddy, kjain, disgoel, acme,
jolsa, adrian.hunter, irogers, namhyung
On 13/10/2023 08:36, Athira Rajeev 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
>
> The exact failure log can be found in:
> output/tests/shell/record.dep.log file
>
Hi Athira,
Having the reason for a hard failure put into a log file rather than the
console output is very non standard. I'm not sure what the reason for
this is.
The log filename isn't even listed in the output so how would anyone
know what went wrong?
Can we just have it so that the failure is printed in the make output
like any other build error.
[...]
> +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 ]])
[[ ]] is a bash extension, but the build system seems to use /bin/sh so
you get this error depending on your distro:
tools/perf/tests/Makefile.tests:17: output/tests/shell
/record+probe_libc_inet_pton.dep] Error 127
/bin/sh: 1: [[: not found
Changing it to [ ] fixes it
> + $(Q)$(call frecho-cmd,test)@touch $@
Touching the source file in the build system doesn't feel right, surely
this could be open to all kinds of parallel build race conditions or
version controll issues.
Isn't the output of the rule the .log file, so just a normal make rule
based on those two files work? Then if the .log file is older than the
source file, the shellcheck is re-run, otherwise not? It feels like the
.dep file would then also be unecessary.
The .dep lines in the make output are a bit confusing because they're
not in the source tree so it's not clear to an outsider what that make
output is for.
Other than that, it does seem to work ok for me.
> + $(Q)$(call frecho-cmd,test)@rm -rf $@.log
> +$(DIRS):
> + @mkdir -p $@
> +
> +clean:
> + @rm -rf output
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V3] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
2023-10-23 10:44 ` James Clark
@ 2023-11-07 6:40 ` Athira Rajeev
0 siblings, 0 replies; 4+ messages in thread
From: Athira Rajeev @ 2023-11-07 6:40 UTC (permalink / raw)
To: James Clark
Cc: linux-perf-users, linuxppc-dev, Madhavan Srinivasan, kjain,
disgoel, acme, jolsa, adrian.hunter, irogers, namhyung
> On 23-Oct-2023, at 4:14 PM, James Clark <james.clark@arm.com> wrote:
>
>
>
> On 13/10/2023 08:36, Athira Rajeev 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
>>
>> The exact failure log can be found in:
>> output/tests/shell/record.dep.log file
>>
>
> Hi Athira,
>
> Having the reason for a hard failure put into a log file rather than the
> console output is very non standard. I'm not sure what the reason for
> this is.
>
> The log filename isn't even listed in the output so how would anyone
> know what went wrong?
>
> Can we just have it so that the failure is printed in the make output
> like any other build error.
Sure James, Thanks for looking into and sharing the review comment.
I will address the change in V4
>
> [...]
>
>> +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 ]])
>
> [[ ]] is a bash extension, but the build system seems to use /bin/sh so
> you get this error depending on your distro:
>
> tools/perf/tests/Makefile.tests:17: output/tests/shell
> /record+probe_libc_inet_pton.dep] Error 127
> /bin/sh: 1: [[: not found
>
> Changing it to [ ] fixes it
Ok, will make the change in next version
>
>> + $(Q)$(call frecho-cmd,test)@touch $@
>
> Touching the source file in the build system doesn't feel right, surely
> this could be open to all kinds of parallel build race conditions or
> version controll issues.
>
> Isn't the output of the rule the .log file, so just a normal make rule
> based on those two files work? Then if the .log file is older than the
> source file, the shellcheck is re-run, otherwise not? It feels like the
> .dep file would then also be unecessary.
Ok, I will fix this.
>
> The .dep lines in the make output are a bit confusing because they're
> not in the source tree so it's not clear to an outsider what that make
> output is for.
>
> Other than that, it does seem to work ok for me.
Thanks for the review. I will post V4 with all the changes
Athira
>
>> + $(Q)$(call frecho-cmd,test)@rm -rf $@.log
>> +$(DIRS):
>> + @mkdir -p $@
>> +
>> +clean:
>> + @rm -rf output
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-07 6:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-13 7:36 [PATCH V3] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf Athira Rajeev
2023-10-22 7:12 ` Athira Rajeev
2023-10-23 10:44 ` James Clark
2023-11-07 6:40 ` 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).