Re: [PATCH 2/2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
> On 27-Sep-2023, at 9:55 AM, Athira Rajeev wrote:
>
>
>
>> On 27-Sep-2023, at 5:25 AM, Namhyung Kim wrote:
>>
>> On Thu, Sep 14, 2023 at 10:18 AM 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
>>
>> I think you can do it if $(shell command -v shellcheck) returns
>> non-empty string (the path to the shellcheck). Then the feature
>> test logic can be gone.
>
> Ok, I will try this.
>>
>>>
>>> CONFIG_SHELLCHECK check is added 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.
>>
>> Can you show me the example output?
>
> Sure, I will add it.
>>
>>>
>>> Signed-off-by: Athira Rajeev
>>> ---
>>> tools/perf/Makefile.perf| 12 +++-
>>> tools/perf/tests/Makefile.tests | 24
>>> 2 files changed, 35 insertions(+), 1 deletion(-)
>>> create mode 100644 tools/perf/tests/Makefile.tests
>>>
>>> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
>>> index f6fdc2d5a92f..c27f54771e90 100644
>>> --- a/tools/perf/Makefile.perf
>>> +++ b/tools/perf/Makefile.perf
>>> @@ -667,7 +667,16 @@ $(PERF_IN): prepare FORCE
>>> $(PMU_EVENTS_IN): FORCE prepare
>>> $(Q)$(MAKE) -f $(srctree)/tools/build/Makefile.build dir=pmu-events
>>> obj=pmu-events
>>>
>>> -$(OUTPUT)perf: $(PERFLIBS) $(PERF_IN) $(PMU_EVENTS_IN)
>>> +# Runs shellcheck on perf test shell scripts
>>> +ifeq ($(CONFIG_SHELLCHECK),y)
>>> +SHELLCHECK_TEST: FORCE prepare
>>> + $(Q)$(MAKE) -f $(srctree)/tools/perf/tests/Makefile.tests
>>> +else
>>> +SHELLCHECK_TEST:
>>> + @:
>>> +endif
>>> +
>>> +$(OUTPUT)perf: $(PERFLIBS) $(PERF_IN) $(PMU_EVENTS_IN) SHELLCHECK_TEST
>>> $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) \
>>> $(PERF_IN) $(PMU_EVENTS_IN) $(LIBS) -o $@
>>>
>>> @@ -1129,6 +1138,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
>>> diff --git a/tools/perf/tests/Makefile.tests
>>> b/tools/perf/tests/Makefile.tests
>>> new file mode 100644
>>> index ..e74575559e83
>>> --- /dev/null
>>> +++ b/tools/perf/tests/Makefile.tests
>>> @@ -0,0 +1,24 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +# Athira Rajeev , 2023
>>> +-include $(OUTPUT).config-detected
>>> +
>>> +log_file = $(OUTPUT)shellcheck_test.log
>>> +PROGS = $(subst ./,,$(shell find tests/shell -perm -o=x -type f -name
>>> '*.sh'))
>>> +DEPS = $(addprefix output/,$(addsuffix .dep,$(basename $(PROGS
>>> +DIRS = $(shell echo $(dir $(DEPS)) | xargs -n1 | sort -u | xargs)
>>> +
>>> +.PHONY: all
>>> +all: SHELLCHECK_RUN
>>> + @:
>>> +
>>> +SHELLCHECK_RUN: $(DEPS) $(DIRS)
>>> +
>>> +output/%.dep: %.sh | $(DIRS)
>>> + $(call rule_mkdir)
>>> + $(Q)$(call frecho-cmd,test)@touch $@
>>> + $(Q)$(call frecho-cmd,test)@shellcheck -S warning $(subst
>>> output/,./,$(patsubst %.dep, %.sh, $@)) 1> ${log_file} && ([[ ! -s
>>> ${log_file} ]])
>>
>> This line is too long, please wrap it with some backslashes.
> Ok
>
> I will address all the comments in next version
Hi Namhyung,
While working on V2 for the Makefile changes and testing, came across three
issues with latest scripts in perf-tools-next.
I have addressed those in below patchset:
https://lore.kernel.org/linux-perf-users/20230929041133.95355-1-atraj...@linux.vnet.ibm.com/T/#m7b3dc8a96467058e1b392183190baed47ae0eb75
[PATCH 0/3] Fix for shellcheck issues with latest scripts in tests/shell
For the Makefile.perf changes, I will send V2 separately addressing review
comments
Thanks
Athira
>
> Thanks
> Athira
>>
>> Thanks,
>> Namhyung
>>
>>
>>> +$(DIRS):
>>> + @mkdir -p $@
>>> +
>>> +clean:
>>> + @rm -rf $(log_file) output
>>> --
>>> 2.31.1
Re: [PATCH 2/2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
> On 27-Sep-2023, at 5:25 AM, Namhyung Kim wrote:
>
> On Thu, Sep 14, 2023 at 10:18 AM 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
>
> I think you can do it if $(shell command -v shellcheck) returns
> non-empty string (the path to the shellcheck). Then the feature
> test logic can be gone.
Ok, I will try this.
>
>>
>> CONFIG_SHELLCHECK check is added 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.
>
> Can you show me the example output?
Sure, I will add it.
>
>>
>> Signed-off-by: Athira Rajeev
>> ---
>> tools/perf/Makefile.perf| 12 +++-
>> tools/perf/tests/Makefile.tests | 24
>> 2 files changed, 35 insertions(+), 1 deletion(-)
>> create mode 100644 tools/perf/tests/Makefile.tests
>>
>> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
>> index f6fdc2d5a92f..c27f54771e90 100644
>> --- a/tools/perf/Makefile.perf
>> +++ b/tools/perf/Makefile.perf
>> @@ -667,7 +667,16 @@ $(PERF_IN): prepare FORCE
>> $(PMU_EVENTS_IN): FORCE prepare
>>$(Q)$(MAKE) -f $(srctree)/tools/build/Makefile.build dir=pmu-events
>> obj=pmu-events
>>
>> -$(OUTPUT)perf: $(PERFLIBS) $(PERF_IN) $(PMU_EVENTS_IN)
>> +# Runs shellcheck on perf test shell scripts
>> +ifeq ($(CONFIG_SHELLCHECK),y)
>> +SHELLCHECK_TEST: FORCE prepare
>> + $(Q)$(MAKE) -f $(srctree)/tools/perf/tests/Makefile.tests
>> +else
>> +SHELLCHECK_TEST:
>> + @:
>> +endif
>> +
>> +$(OUTPUT)perf: $(PERFLIBS) $(PERF_IN) $(PMU_EVENTS_IN) SHELLCHECK_TEST
>>$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) \
>>$(PERF_IN) $(PMU_EVENTS_IN) $(LIBS) -o $@
>>
>> @@ -1129,6 +1138,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
>> diff --git a/tools/perf/tests/Makefile.tests
>> b/tools/perf/tests/Makefile.tests
>> new file mode 100644
>> index ..e74575559e83
>> --- /dev/null
>> +++ b/tools/perf/tests/Makefile.tests
>> @@ -0,0 +1,24 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Athira Rajeev , 2023
>> +-include $(OUTPUT).config-detected
>> +
>> +log_file = $(OUTPUT)shellcheck_test.log
>> +PROGS = $(subst ./,,$(shell find tests/shell -perm -o=x -type f -name
>> '*.sh'))
>> +DEPS = $(addprefix output/,$(addsuffix .dep,$(basename $(PROGS
>> +DIRS = $(shell echo $(dir $(DEPS)) | xargs -n1 | sort -u | xargs)
>> +
>> +.PHONY: all
>> +all: SHELLCHECK_RUN
>> + @:
>> +
>> +SHELLCHECK_RUN: $(DEPS) $(DIRS)
>> +
>> +output/%.dep: %.sh | $(DIRS)
>> + $(call rule_mkdir)
>> + $(Q)$(call frecho-cmd,test)@touch $@
>> + $(Q)$(call frecho-cmd,test)@shellcheck -S warning $(subst
>> output/,./,$(patsubst %.dep, %.sh, $@)) 1> ${log_file} && ([[ ! -s
>> ${log_file} ]])
>
> This line is too long, please wrap it with some backslashes.
Ok
I will address all the comments in next version
Thanks
Athira
>
> Thanks,
> Namhyung
>
>
>> +$(DIRS):
>> + @mkdir -p $@
>> +
>> +clean:
>> + @rm -rf $(log_file) output
>> --
>> 2.31.1
Re: [PATCH 2/2] tools/perf: Add perf binary dependent rule for shellcheck log in Makefile.perf
On Thu, Sep 14, 2023 at 10:18 AM 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
I think you can do it if $(shell command -v shellcheck) returns
non-empty string (the path to the shellcheck). Then the feature
test logic can be gone.
>
> CONFIG_SHELLCHECK check is added 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.
Can you show me the example output?
>
> Signed-off-by: Athira Rajeev
> ---
> tools/perf/Makefile.perf| 12 +++-
> tools/perf/tests/Makefile.tests | 24
> 2 files changed, 35 insertions(+), 1 deletion(-)
> create mode 100644 tools/perf/tests/Makefile.tests
>
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index f6fdc2d5a92f..c27f54771e90 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -667,7 +667,16 @@ $(PERF_IN): prepare FORCE
> $(PMU_EVENTS_IN): FORCE prepare
> $(Q)$(MAKE) -f $(srctree)/tools/build/Makefile.build dir=pmu-events
> obj=pmu-events
>
> -$(OUTPUT)perf: $(PERFLIBS) $(PERF_IN) $(PMU_EVENTS_IN)
> +# Runs shellcheck on perf test shell scripts
> +ifeq ($(CONFIG_SHELLCHECK),y)
> +SHELLCHECK_TEST: FORCE prepare
> + $(Q)$(MAKE) -f $(srctree)/tools/perf/tests/Makefile.tests
> +else
> +SHELLCHECK_TEST:
> + @:
> +endif
> +
> +$(OUTPUT)perf: $(PERFLIBS) $(PERF_IN) $(PMU_EVENTS_IN) SHELLCHECK_TEST
> $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) \
> $(PERF_IN) $(PMU_EVENTS_IN) $(LIBS) -o $@
>
> @@ -1129,6 +1138,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
> diff --git a/tools/perf/tests/Makefile.tests b/tools/perf/tests/Makefile.tests
> new file mode 100644
> index ..e74575559e83
> --- /dev/null
> +++ b/tools/perf/tests/Makefile.tests
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Athira Rajeev , 2023
> +-include $(OUTPUT).config-detected
> +
> +log_file = $(OUTPUT)shellcheck_test.log
> +PROGS = $(subst ./,,$(shell find tests/shell -perm -o=x -type f -name
> '*.sh'))
> +DEPS = $(addprefix output/,$(addsuffix .dep,$(basename $(PROGS
> +DIRS = $(shell echo $(dir $(DEPS)) | xargs -n1 | sort -u | xargs)
> +
> +.PHONY: all
> +all: SHELLCHECK_RUN
> + @:
> +
> +SHELLCHECK_RUN: $(DEPS) $(DIRS)
> +
> +output/%.dep: %.sh | $(DIRS)
> + $(call rule_mkdir)
> + $(Q)$(call frecho-cmd,test)@touch $@
> + $(Q)$(call frecho-cmd,test)@shellcheck -S warning $(subst
> output/,./,$(patsubst %.dep, %.sh, $@)) 1> ${log_file} && ([[ ! -s
> ${log_file} ]])
This line is too long, please wrap it with some backslashes.
Thanks,
Namhyung
> +$(DIRS):
> + @mkdir -p $@
> +
> +clean:
> + @rm -rf $(log_file) output
> --
> 2.31.1
>
