[PATCH V3] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf

[Athira Rajeev]

> On 23-Oct-2023, at 4:14 PM, James Clark <james.clark at 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