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

[Ian Rogers]

On Mon, Jun 23, 2025 at 1:10 AM James Clark <james.clark@xxxxxxxxxx> wrote:
>
>
>
> On 20/06/2025 6:40 pm, Collin Funk wrote:
> > Hi James,
> >
> > James Clark <james.clark@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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