Re: [PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
Em Wed, Dec 06, 2023 at 10:15:06AM +0530, Athira Rajeev escreveu: > > > > On 06-Dec-2023, at 3:20 AM, Arnaldo Carvalho de Melo > > wrote: > > > > Em Mon, Nov 27, 2023 at 11:12:57AM +, James Clark escreveu: > >> On 23/11/2023 16:02, Athira Rajeev wrote: > >>> --- a/tools/perf/Makefile.perf > >>> @@ -1134,6 +1152,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 > > > > While merging perf-tools-next with torvalds/master I noticed that maybe > > we better have the above added line as: > > > > + $(call QUIET_CLEAN, tests) $(Q)$(MAKE) -f > > $(srctree)/tools/perf/tests/Makefile.tests clean > > > > No? > > > > Anyway I'm merging as-is, but it just hit my eye while merging, > > > > - Arnaldo > Hi Arnaldo > > As Ian pointed we removed Makefile.tests as part of : > https://lore.kernel.org/lkml/20231129213428.2227448-1-irog...@google.com/ Right, thanks for checking, see my reply to Ian for further details. - Arnaldo
Re: [PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
Em Tue, Dec 05, 2023 at 02:09:01PM -0800, Ian Rogers escreveu: > On Tue, Dec 5, 2023 at 1:50 PM Arnaldo Carvalho de Melo > wrote: > > > > Em Mon, Nov 27, 2023 at 11:12:57AM +, James Clark escreveu: > > > On 23/11/2023 16:02, Athira Rajeev wrote: > > > > --- a/tools/perf/Makefile.perf > > > > @@ -1134,6 +1152,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 > > > > While merging perf-tools-next with torvalds/master I noticed that maybe > > we better have the above added line as: > > > > + $(call QUIET_CLEAN, tests) $(Q)$(MAKE) -f > > $(srctree)/tools/perf/tests/Makefile.tests clean > > > > No? > > > > Anyway I'm merging as-is, but it just hit my eye while merging, > > > > - Arnaldo > > Makefile.tests was removed in these recent patches adding support for > the OUTPUT directory: > https://lore.kernel.org/lkml/9c33887f-8a88-4973-8593-7936e36af...@linux.vnet.ibm.com/ Right, I made a mistake and was doing the merge on a different branch, now that I tried it on my latest perf-tools-next local branch all is well and the other merge conflict gets auto-resolved (arm64-sysreg-defs-clean stuff in tools/perf/Makefile.perf "clean" target). Thanks for checking, - Arnaldo
Re: [PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
> On 06-Dec-2023, at 3:20 AM, Arnaldo Carvalho de Melo wrote: > > Em Mon, Nov 27, 2023 at 11:12:57AM +, James Clark escreveu: >> On 23/11/2023 16:02, Athira Rajeev wrote: >>> --- a/tools/perf/Makefile.perf >>> @@ -1134,6 +1152,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 > > While merging perf-tools-next with torvalds/master I noticed that maybe > we better have the above added line as: > > + $(call QUIET_CLEAN, tests) $(Q)$(MAKE) -f > $(srctree)/tools/perf/tests/Makefile.tests clean > > No? > > Anyway I'm merging as-is, but it just hit my eye while merging, > > - Arnaldo Hi Arnaldo As Ian pointed we removed Makefile.tests as part of : https://lore.kernel.org/lkml/20231129213428.2227448-1-irog...@google.com/ Thanks Athira
Re: [PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
On Tue, Dec 5, 2023 at 1:50 PM Arnaldo Carvalho de Melo wrote: > > Em Mon, Nov 27, 2023 at 11:12:57AM +, James Clark escreveu: > > On 23/11/2023 16:02, Athira Rajeev wrote: > > > --- a/tools/perf/Makefile.perf > > > @@ -1134,6 +1152,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 > > While merging perf-tools-next with torvalds/master I noticed that maybe > we better have the above added line as: > > + $(call QUIET_CLEAN, tests) $(Q)$(MAKE) -f > $(srctree)/tools/perf/tests/Makefile.tests clean > > No? > > Anyway I'm merging as-is, but it just hit my eye while merging, > > - Arnaldo Makefile.tests was removed in these recent patches adding support for the OUTPUT directory: https://lore.kernel.org/lkml/9c33887f-8a88-4973-8593-7936e36af...@linux.vnet.ibm.com/ Thanks, Ian
Re: [PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
Em Mon, Nov 27, 2023 at 11:12:57AM +, James Clark escreveu: > On 23/11/2023 16:02, Athira Rajeev wrote: > > --- a/tools/perf/Makefile.perf > > @@ -1134,6 +1152,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 While merging perf-tools-next with torvalds/master I noticed that maybe we better have the above added line as: + $(call QUIET_CLEAN, tests) $(Q)$(MAKE) -f $(srctree)/tools/perf/tests/Makefile.tests clean No? Anyway I'm merging as-is, but it just hit my eye while merging, - Arnaldo
Re: [PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
> On 27-Nov-2023, at 8:21 PM, Arnaldo Carvalho de Melo wrote: > > Em Mon, Nov 27, 2023 at 11:12:57AM +, James Clark escreveu: >> On 23/11/2023 16:02, 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. > >> Seems to work really well. I also tested it on Ubuntu, and checked >> NO_SHELLCHECK, cleaning and with and without shellcheck installed etc. > >> Reviewed-by: James Clark > > Tested on Fedora 38, works as advertised, applied. > > - Arnaldo Hi James, Arnaldo Thanks for testing the patch and comments. Athira Rajeev
Re: [PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
Em Mon, Nov 27, 2023 at 11:12:57AM +, James Clark escreveu: > On 23/11/2023 16:02, 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. > Seems to work really well. I also tested it on Ubuntu, and checked > NO_SHELLCHECK, cleaning and with and without shellcheck installed etc. > Reviewed-by: James Clark Tested on Fedora 38, works as advertised, applied. - Arnaldo
Re: [PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
On 23/11/2023 16:02, 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. > Seems to work really well. I also tested it on Ubuntu, and checked NO_SHELLCHECK, cleaning and with and without shellcheck installed etc. Reviewed-by: James Clark > $ 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 by modifying probe_vfs_getname.sh in tests/shell: > > In tests/shell/probe_vfs_getname.sh line 8: > . $(dirname $0)/lib/probe.sh > ^---^ SC2046 (warning): Quote this to prevent word splitting. > > For more information: > https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word > splitt... > make[3]: *** > [/root/athira/perf-tools-next/tools/perf/tests/Makefile.tests:18: > tests/shell/.probe_vfs_getname.sh.shellcheck_log] Error 1 > make[2]: *** [Makefile.perf:686: SHELLCHECK_TEST] Error 2 > make[2]: *** Waiting for unfinished jobs > make[1]: *** [Makefile.perf:244: sub-make] Error 2 > make: *** [Makefile:70: all] Error 2 > > Here, like other files which gets created during > compilation (ex: .builtin-bench.o.cmd or .perf.o.cmd ), > create .shellcheck_log also as a hidden file. > Example: tests/shell/.probe_vfs_getname.sh.shellcheck_log > shellcheck is re-run if any of the script gets modified > based on its dependency of this log file. > > After this, for testing, changed "tests/shell/trace+probe_vfs_getname.sh" to > break shellcheck format. In the next make run, it is > also captured: > > In tests/shell/probe_vfs_getname.sh line 8: > . $(dirname $0)/lib/probe.sh > ^---^ SC2046 (warning): Quote this to prevent word splitting. > > For more information: > https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word > splitt... > make[3]: *** > [/root/athira/perf-tools-next/tools/perf/tests/Makefile.tests:18: > tests/shell/.probe_vfs_getname.sh.shellcheck_log] Error 1 > make[3]: *** Waiting for unfinished jobs > > In tests/shell/trace+probe_vfs_getname.sh line 14: > . $(dirname $0)/lib/probe.sh > ^---^ SC2046 (warning): Quote this to prevent word splitting. > > For more information: > https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word > splitt... > make[3]: *** > [/root/athira/perf-tools-next/tools/perf/tests/Makefile.tests:18: > tests/shell/.trace+probe_vfs_getname.sh.shellcheck_log] Error 1 > make[2]: *** [Makefile.perf:686: SHELLCHECK_TEST] Error 2 > make[2]: *** Waiting for unfinished jobs > make[1]: *** [Makefile.perf:244: sub-make] Error 2 > make: *** [Makefile:70: all] Error 2 > > Failure log can be found in the stdout of make itself. > > 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_SHELLCHECK=1 > > INSTALL libsubcmd_headers > INSTALL libsymbol_headers > INSTALL libapi_headers > INSTALL libperf_headers > INSTALL libbpf_headers > LINKperf > > Note: > This is tested on RHEL and also SLES. Use below check: > "$(shell which shellcheck 2> /dev/null)" to look for presence > of shellcheck binary. The approach "shell command -v" is not > used here. In some of the distros(RHEL), command is available > as executable file (/usr/bin/command). But in some distros(SLES), > it is a shell builtin and not available as executable file. > > Signed-off-by: Athira Rajeev > --- > changelog: > v3 -> v4: > Addressed review comments from James Clark. > - Made the shellcheck errors to be reported in make output > itself during make like any other build error. > - Removed creating .dep files. Instead use the log file > to determine whether shellcheck has to be re-run when > there is a change in source file. > - Change log file to have suffix as shellcheck_log so > as to differentiate it from test execution log. > - Also like other files which gets created during > compilation, example, .builtin-bench.o.cmd or .perf.o.cmd, > create .shellcheck_log
[PATCH V4] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
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 by modifying probe_vfs_getname.sh in tests/shell: In tests/shell/probe_vfs_getname.sh line 8: . $(dirname $0)/lib/probe.sh ^---^ SC2046 (warning): Quote this to prevent word splitting. For more information: https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt... make[3]: *** [/root/athira/perf-tools-next/tools/perf/tests/Makefile.tests:18: tests/shell/.probe_vfs_getname.sh.shellcheck_log] Error 1 make[2]: *** [Makefile.perf:686: SHELLCHECK_TEST] Error 2 make[2]: *** Waiting for unfinished jobs make[1]: *** [Makefile.perf:244: sub-make] Error 2 make: *** [Makefile:70: all] Error 2 Here, like other files which gets created during compilation (ex: .builtin-bench.o.cmd or .perf.o.cmd ), create .shellcheck_log also as a hidden file. Example: tests/shell/.probe_vfs_getname.sh.shellcheck_log shellcheck is re-run if any of the script gets modified based on its dependency of this log file. After this, for testing, changed "tests/shell/trace+probe_vfs_getname.sh" to break shellcheck format. In the next make run, it is also captured: In tests/shell/probe_vfs_getname.sh line 8: . $(dirname $0)/lib/probe.sh ^---^ SC2046 (warning): Quote this to prevent word splitting. For more information: https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt... make[3]: *** [/root/athira/perf-tools-next/tools/perf/tests/Makefile.tests:18: tests/shell/.probe_vfs_getname.sh.shellcheck_log] Error 1 make[3]: *** Waiting for unfinished jobs In tests/shell/trace+probe_vfs_getname.sh line 14: . $(dirname $0)/lib/probe.sh ^---^ SC2046 (warning): Quote this to prevent word splitting. For more information: https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt... make[3]: *** [/root/athira/perf-tools-next/tools/perf/tests/Makefile.tests:18: tests/shell/.trace+probe_vfs_getname.sh.shellcheck_log] Error 1 make[2]: *** [Makefile.perf:686: SHELLCHECK_TEST] Error 2 make[2]: *** Waiting for unfinished jobs make[1]: *** [Makefile.perf:244: sub-make] Error 2 make: *** [Makefile:70: all] Error 2 Failure log can be found in the stdout of make itself. 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_SHELLCHECK=1 INSTALL libsubcmd_headers INSTALL libsymbol_headers INSTALL libapi_headers INSTALL libperf_headers INSTALL libbpf_headers LINKperf Note: This is tested on RHEL and also SLES. Use below check: "$(shell which shellcheck 2> /dev/null)" to look for presence of shellcheck binary. The approach "shell command -v" is not used here. In some of the distros(RHEL), command is available as executable file (/usr/bin/command). But in some distros(SLES), it is a shell builtin and not available as executable file. Signed-off-by: Athira Rajeev --- changelog: v3 -> v4: Addressed review comments from James Clark. - Made the shellcheck errors to be reported in make output itself during make like any other build error. - Removed creating .dep files. Instead use the log file to determine whether shellcheck has to be re-run when there is a change in source file. - Change log file to have suffix as shellcheck_log so as to differentiate it from test execution log. - Also like other files which gets created during compilation, example, .builtin-bench.o.cmd or .perf.o.cmd, create .shellcheck_log as hidden file. Example: tests/shell/.buildid.sh.shellcheck_log - Initial version used "command -v shellcheck" to check presence of shellcheck. But while testing SLES, hit an issue with using "command". In RHEL, /usr/bin/command is available as part of bash rpm # rpm -qf /usr/bin/command bash-5.1.8-6.el9_1.ppc64le But in SLES ( tested in SLES 15 SP4 ), this is not
