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

[James Clark]

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