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
