Re: [PATCH 1/2] tools/perf: Add new CONFIG_SHELLCHECK for detecting shellcheck binary

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 27-Sep-2023, at 5:21 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> Hello,
> 
> On Thu, Sep 14, 2023 at 10:18 AM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> shellcheck tool can detect coding/formatting issues on
>> shell scripts. In perf directory "tests/shell", there are lot
>> of shell test scripts and this tool can detect coding/formatting
>> issues on these scripts.
>> 
>> Example to use shellcheck for severity level for
>> errors and warnings, below command is used:
>> 
>>   # for F in $(find tests/shell/ -perm -o=x -name '*.sh'); do shellcheck -S warning $F; done
>>   # echo $?
>>     0
>> 
>> This testing needs to be automated into the build so that it
>> can avoid regressions and also run the check for newly added
>> during build test itself. Add a new feature check to detect
>> presence of shellcheck. Add CONFIG_SHELLCHECK feature check in
>> the build to avoid not having shellcheck breaking the build.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> tools/build/Makefile.feature |  6 ++++--
>> tools/build/feature/Makefile |  8 +++++++-
>> tools/perf/Makefile.config   | 10 ++++++++++
>> 3 files changed, 21 insertions(+), 3 deletions(-)
>> 
>> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
>> index 934e2777a2db..23f56b95babf 100644
>> --- a/tools/build/Makefile.feature
>> +++ b/tools/build/Makefile.feature
>> @@ -72,7 +72,8 @@ FEATURE_TESTS_BASIC :=                  \
>>         libzstd                                \
>>         disassembler-four-args         \
>>         disassembler-init-styled       \
>> -        file-handle
>> +        file-handle                    \
>> +        shellcheck
>> 
>> # FEATURE_TESTS_BASIC + FEATURE_TESTS_EXTRA is the complete list
>> # of all feature tests
>> @@ -138,7 +139,8 @@ FEATURE_DISPLAY ?=              \
>>          get_cpuid              \
>>          bpf                   \
>>          libaio                        \
>> -         libzstd
>> +         libzstd               \
>> +         shellcheck
>> 
>> #
>> # Declare group members of a feature to display the logical OR of the detection
>> diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
>> index 3184f387990a..44ba6d0c98d0 100644
>> --- a/tools/build/feature/Makefile
>> +++ b/tools/build/feature/Makefile
>> @@ -76,7 +76,8 @@ FILES=                                          \
>>          test-libzstd.bin                      \
>>          test-clang-bpf-co-re.bin              \
>>          test-file-handle.bin                  \
>> -         test-libpfm4.bin
>> +         test-libpfm4.bin                      \
>> +         test-shellcheck.bin
>> 
>> FILES := $(addprefix $(OUTPUT),$(FILES))
>> 
>> @@ -92,6 +93,8 @@ __BUILD = $(CC) $(CFLAGS) -MD -Wall -Werror -o $@ $(patsubst %.bin,%.c,$(@F)) $(
>> __BUILDXX = $(CXX) $(CXXFLAGS) -MD -Wall -Werror -o $@ $(patsubst %.bin,%.cpp,$(@F)) $(LDFLAGS)
>>   BUILDXX = $(__BUILDXX) > $(@:.bin=.make.output) 2>&1
>> 
>> +  BUILD_BINARY = sh -c $1 > $(@:.bin=.make.output) 2>&1
>> +
>> ###############################
>> 
>> $(OUTPUT)test-all.bin:
>> @@ -207,6 +210,9 @@ $(OUTPUT)test-libslang-include-subdir.bin:
>> $(OUTPUT)test-libtraceevent.bin:
>>        $(BUILD) -ltraceevent
>> 
>> +$(OUTPUT)test-shellcheck.bin:
>> +       $(BUILD_BINARY) "shellcheck --version"
> 
> I don't think it'd generate the .bin file.
> 
> Anyway, it's a binary file already.  Can we check it with
> `command -v` and get rid of the feature test?
> 
> Thanks,
> Namhyung

Hi Namhyung,

Thanks for the review. Sure, I will check on this

Athira
> 
> 
>> +
>> $(OUTPUT)test-libtracefs.bin:
>>         $(BUILD) $(shell $(PKG_CONFIG) --cflags libtraceevent 2>/dev/null) -ltracefs
>> 
>> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
>> index d66b52407e19..e71fe95ad865 100644
>> --- a/tools/perf/Makefile.config
>> +++ b/tools/perf/Makefile.config
>> @@ -779,6 +779,16 @@ ifndef NO_SLANG
>>   endif
>> endif
>> 
>> +ifneq ($(NO_SHELLCHECK),1)
>> +  $(call feature_check,shellcheck)
>> +  ifneq ($(feature-shellcheck), 1)
>> +    msg := $(warning No shellcheck found. please install ShellCheck);
>> +  else
>> +    $(call detected,CONFIG_SHELLCHECK)
>> +    NO_SHELLCHECK := 0
>> +  endif
>> +endif
>> +
>> ifdef GTK2
>>   FLAGS_GTK2=$(CFLAGS) $(LDFLAGS) $(EXTLIBS) $(shell $(PKG_CONFIG) --libs --cflags gtk+-2.0 2>/dev/null)
>>   $(call feature_check,gtk2)
>> --
>> 2.31.1