perf build: Specify that spellcheck should use the bash dialect.

[PATCH] perf build: Specify that spellcheck should use the bash dialect.

Posted by Collin Funk 3 months, 4 weeks ago

When someone has a global shellcheckrc file, for example at
~/.config/shellcheckrc, with the directive 'shell=sh', building perf
will fail with many shellcheck errors like:

    In tests/shell/base_probe/test_adding_kernel.sh line 294:
    (( TEST_RESULT += $? ))
    ^---------------------^ SC3006 (warning): In POSIX sh, standalone ((..)) is undefined.

    For more information:
      https://www.shellcheck.net/wiki/SC3006 -- In POSIX sh, standalone ((..)) is...
    make[5]: *** [tests/Build:91: tests/shell/base_probe/test_adding_kernel.sh.shellcheck_log] Error 1

Passing the '-s bash' option ensures that it runs correctly regardless
of a developers global configuration.

Signed-off-by: Collin Funk <collin.funk1@gmail.com>
---
 tools/perf/tests/Build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 2181f5a92148..26efc5d20f6c 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -89,7 +89,7 @@ endif
 
 $(OUTPUT)%.shellcheck_log: %
 	$(call rule_mkdir)
-	$(Q)$(call echo-cmd,test)shellcheck -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
+	$(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
 
 perf-test-y += $(SHELL_TEST_LOGS)
 
-- 
2.49.0

Re: [PATCH] perf build: Specify that spellcheck should use the bash dialect.

Posted by James Clark 3 months, 3 weeks ago


On 13/06/2025 4:36 am, Collin Funk wrote:
> When someone has a global shellcheckrc file, for example at
> ~/.config/shellcheckrc, with the directive 'shell=sh', building perf
> will fail with many shellcheck errors like:
> 
>      In tests/shell/base_probe/test_adding_kernel.sh line 294:
>      (( TEST_RESULT += $? ))
>      ^---------------------^ SC3006 (warning): In POSIX sh, standalone ((..)) is undefined.
> 
>      For more information:
>        https://www.shellcheck.net/wiki/SC3006 -- In POSIX sh, standalone ((..)) is...
>      make[5]: *** [tests/Build:91: tests/shell/base_probe/test_adding_kernel.sh.shellcheck_log] Error 1
> 
> Passing the '-s bash' option ensures that it runs correctly regardless
> of a developers global configuration.
> 
> Signed-off-by: Collin Funk <collin.funk1@gmail.com>
> ---
>   tools/perf/tests/Build | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 2181f5a92148..26efc5d20f6c 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -89,7 +89,7 @@ endif
>   
>   $(OUTPUT)%.shellcheck_log: %
>   	$(call rule_mkdir)
> -	$(Q)$(call echo-cmd,test)shellcheck -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
> +	$(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
>   
>   perf-test-y += $(SHELL_TEST_LOGS)
>   

If we're enforcing bash style with static analysis shouldn't we also 
change all the hashbangs to bash? Recently there have been changes to 
change sh to bash in some of the tests so presumably the hard rule for 
sh is no more?

In the past I've had to replace bashisms that didn't work in sh but it 
would be nice to have only one language to write tests in. I doubt 
anyone running the tests today is running somewhere without bash, or 
that changing it will break anything. If anything it will fix more 
bashisms that have already been written.

Just for reference there are 34 #!/bin/bash and 42 #!/bin/sh in 
tools/perf/tests

Thanks
James

Re: [PATCH] perf build: Specify that spellcheck should use the bash dialect.

Posted by Namhyung Kim 3 months, 3 weeks ago

Hello,

On Thu, Jun 19, 2025 at 11:28:46AM +0100, James Clark wrote:
> 
> 
> On 13/06/2025 4:36 am, Collin Funk wrote:
> > When someone has a global shellcheckrc file, for example at
> > ~/.config/shellcheckrc, with the directive 'shell=sh', building perf
> > will fail with many shellcheck errors like:
> > 
> >      In tests/shell/base_probe/test_adding_kernel.sh line 294:
> >      (( TEST_RESULT += $? ))
> >      ^---------------------^ SC3006 (warning): In POSIX sh, standalone ((..)) is undefined.
> > 
> >      For more information:
> >        https://www.shellcheck.net/wiki/SC3006 -- In POSIX sh, standalone ((..)) is...
> >      make[5]: *** [tests/Build:91: tests/shell/base_probe/test_adding_kernel.sh.shellcheck_log] Error 1
> > 
> > Passing the '-s bash' option ensures that it runs correctly regardless
> > of a developers global configuration.
> > 
> > Signed-off-by: Collin Funk <collin.funk1@gmail.com>
> > ---
> >   tools/perf/tests/Build | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> > index 2181f5a92148..26efc5d20f6c 100644
> > --- a/tools/perf/tests/Build
> > +++ b/tools/perf/tests/Build
> > @@ -89,7 +89,7 @@ endif
> >   $(OUTPUT)%.shellcheck_log: %
> >   	$(call rule_mkdir)
> > -	$(Q)$(call echo-cmd,test)shellcheck -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
> > +	$(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
> >   perf-test-y += $(SHELL_TEST_LOGS)
> 
> If we're enforcing bash style with static analysis shouldn't we also change
> all the hashbangs to bash? Recently there have been changes to change sh to
> bash in some of the tests so presumably the hard rule for sh is no more?
> 
> In the past I've had to replace bashisms that didn't work in sh but it would
> be nice to have only one language to write tests in. I doubt anyone running
> the tests today is running somewhere without bash, or that changing it will
> break anything. If anything it will fix more bashisms that have already been
> written.
> 
> Just for reference there are 34 #!/bin/bash and 42 #!/bin/sh in
> tools/perf/tests

Thanks for raising the concern.  I agree that having one standard is a
way to go but I really don't have preference between those shells.

Thanks,
Namhyung

Re: [PATCH] perf build: Specify that spellcheck should use the bash dialect.

Posted by James Clark 3 months, 2 weeks ago


On 20/06/2025 8:49 pm, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Jun 19, 2025 at 11:28:46AM +0100, James Clark wrote:
>>
>>
>> On 13/06/2025 4:36 am, Collin Funk wrote:
>>> When someone has a global shellcheckrc file, for example at
>>> ~/.config/shellcheckrc, with the directive 'shell=sh', building perf
>>> will fail with many shellcheck errors like:
>>>
>>>       In tests/shell/base_probe/test_adding_kernel.sh line 294:
>>>       (( TEST_RESULT += $? ))
>>>       ^---------------------^ SC3006 (warning): In POSIX sh, standalone ((..)) is undefined.
>>>
>>>       For more information:
>>>         https://www.shellcheck.net/wiki/SC3006 -- In POSIX sh, standalone ((..)) is...
>>>       make[5]: *** [tests/Build:91: tests/shell/base_probe/test_adding_kernel.sh.shellcheck_log] Error 1
>>>
>>> Passing the '-s bash' option ensures that it runs correctly regardless
>>> of a developers global configuration.
>>>
>>> Signed-off-by: Collin Funk <collin.funk1@gmail.com>
>>> ---
>>>    tools/perf/tests/Build | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
>>> index 2181f5a92148..26efc5d20f6c 100644
>>> --- a/tools/perf/tests/Build
>>> +++ b/tools/perf/tests/Build
>>> @@ -89,7 +89,7 @@ endif
>>>    $(OUTPUT)%.shellcheck_log: %
>>>    	$(call rule_mkdir)
>>> -	$(Q)$(call echo-cmd,test)shellcheck -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
>>> +	$(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
>>>    perf-test-y += $(SHELL_TEST_LOGS)
>>
>> If we're enforcing bash style with static analysis shouldn't we also change
>> all the hashbangs to bash? Recently there have been changes to change sh to
>> bash in some of the tests so presumably the hard rule for sh is no more?
>>
>> In the past I've had to replace bashisms that didn't work in sh but it would
>> be nice to have only one language to write tests in. I doubt anyone running
>> the tests today is running somewhere without bash, or that changing it will
>> break anything. If anything it will fix more bashisms that have already been
>> written.
>>
>> Just for reference there are 34 #!/bin/bash and 42 #!/bin/sh in
>> tools/perf/tests
> 
> Thanks for raising the concern.  I agree that having one standard is a
> way to go but I really don't have preference between those shells.
> 
> Thanks,
> Namhyung
> 

I would vote for bash then, just because it has a few extra builtins 
that are sometimes useful. I can send a patch that does the change to 
see if anyone objects.

Thanks
James

Re: [PATCH] perf build: Specify that spellcheck should use the bash dialect.

Posted by Collin Funk 3 months, 3 weeks ago

Hi James,

James Clark <james.clark@linaro.org> writes:

> If we're enforcing bash style with static analysis shouldn't we also
> change all the hashbangs to bash? Recently there have been changes to
> change sh to bash in some of the tests so presumably the hard rule for
> sh is no more?
>
> In the past I've had to replace bashisms that didn't work in sh but it
> would be nice to have only one language to write tests in. I doubt
> anyone running the tests today is running somewhere without bash, or
> that changing it will break anything. If anything it will fix more
> bashisms that have already been written.
>
> Just for reference there are 34 #!/bin/bash and 42 #!/bin/sh in
> tools/perf/tests

That sounds reasonable to me. Writing portable shell is a hassle and if
we already assume a working /bin/bash in some places, I don't see a
reason not to use it for the others.

Regarding this patch, shellcheck will use the file extension or shebang
only if it does not find a 'shell' directive in a .shellcheckrc. So that
change will still require this patch.

I saw it was used in other places, so I assumed this patch was fine:

$ find tools/perf -name Build | xargs grep bash
tools/perf/Build:	$(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
tools/perf/trace/beauty/Build:	$(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)

Collin

Re: [PATCH] perf build: Specify that spellcheck should use the bash dialect.

Posted by James Clark 3 months, 2 weeks ago


On 20/06/2025 6:40 pm, Collin Funk wrote:
> Hi James,
> 
> James Clark <james.clark@linaro.org> writes:
> 
>> If we're enforcing bash style with static analysis shouldn't we also
>> change all the hashbangs to bash? Recently there have been changes to
>> change sh to bash in some of the tests so presumably the hard rule for
>> sh is no more?
>>
>> In the past I've had to replace bashisms that didn't work in sh but it
>> would be nice to have only one language to write tests in. I doubt
>> anyone running the tests today is running somewhere without bash, or
>> that changing it will break anything. If anything it will fix more
>> bashisms that have already been written.
>>
>> Just for reference there are 34 #!/bin/bash and 42 #!/bin/sh in
>> tools/perf/tests
> 
> That sounds reasonable to me. Writing portable shell is a hassle and if
> we already assume a working /bin/bash in some places, I don't see a
> reason not to use it for the others.
> 
> Regarding this patch, shellcheck will use the file extension or shebang
> only if it does not find a 'shell' directive in a .shellcheckrc. So that
> change will still require this patch.
> 
> I saw it was used in other places, so I assumed this patch was fine:
> 
> $ find tools/perf -name Build | xargs grep bash
> tools/perf/Build:	$(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
> tools/perf/trace/beauty/Build:	$(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
> 
> Collin

In that case:

Reviewed-by: James Clark <james.clark@linaro.org>

And I'll send the bulk hashbang change separately.

Re: [PATCH] perf build: Specify that spellcheck should use the bash dialect.

Posted by Ian Rogers 3 months, 2 weeks ago

On Mon, Jun 23, 2025 at 1:10 AM James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 20/06/2025 6:40 pm, Collin Funk wrote:
> > Hi James,
> >
> > James Clark <james.clark@linaro.org> writes:
> >
> >> If we're enforcing bash style with static analysis shouldn't we also
> >> change all the hashbangs to bash? Recently there have been changes to
> >> change sh to bash in some of the tests so presumably the hard rule for
> >> sh is no more?
> >>
> >> In the past I've had to replace bashisms that didn't work in sh but it
> >> would be nice to have only one language to write tests in. I doubt
> >> anyone running the tests today is running somewhere without bash, or
> >> that changing it will break anything. If anything it will fix more
> >> bashisms that have already been written.
> >>
> >> Just for reference there are 34 #!/bin/bash and 42 #!/bin/sh in
> >> tools/perf/tests
> >
> > That sounds reasonable to me. Writing portable shell is a hassle and if
> > we already assume a working /bin/bash in some places, I don't see a
> > reason not to use it for the others.
> >
> > Regarding this patch, shellcheck will use the file extension or shebang
> > only if it does not find a 'shell' directive in a .shellcheckrc. So that
> > change will still require this patch.
> >
> > I saw it was used in other places, so I assumed this patch was fine:
> >
> > $ find tools/perf -name Build | xargs grep bash
> > tools/perf/Build:     $(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
> > tools/perf/trace/beauty/Build:        $(Q)$(call echo-cmd,test)shellcheck -s bash -a -S warning "$<" > $@ || (cat $@ && rm $@ && false)
> >
> > Collin
>
> In that case:
>
> Reviewed-by: James Clark <james.clark@linaro.org>
>
> And I'll send the bulk hashbang change separately.

I've no objection to switching to using bash globally. It seems
sub-optimal that we've copy-pasted the shellcheck command across many
different Build files and that this patch will cause the
tools/perf/tests/Build one to differ. My preference would be to have a
global definition probably in Makefile.perf, then use it consistently.
Alternative all shellcheck invocations can pass "-s bash" for the sake
of consistency.  Fwiw, I think the 'tools/arch/x86/tools/gen-insn-*'
which is to some extent taken from the kernel's 'arch/x86/tools' is
okay with the change too.

Thanks,
Ian

Re: [PATCH] perf build: Specify that spellcheck should use the bash dialect.