Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Ian Rogers <irogers@google.com>]

On Sun, Jan 15, 2023 at 9:03 PM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
>
>
> > On 28-Sep-2022, at 10:24 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> >
> > Hi All,
> >
> > Looking for what direction we can take here.
> > Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
> > the approach in the patch ? Please share your comments
> >
> > Thanks
> > Athira
> >
>
> Hi All,
>
> Looking for what direction we can take here.
> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
> the approach in the patch ? Please share your comments
>
> Thanks
> Athira

I think some of what the patch is doing is good, some of it the
readability becomes a little harder by not being bash. I'm agnostic as
to which approach to take for the fix.

An aside, I noticed that we do run some tests at build time:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/lib/Makefile?h=perf/core#n390
So perhaps we can have a shellcheck build option, defaulted off but
enabled as part of Arnaldo's regular test scripts. The shellcheck
build option would run shellcheck to make sure that there weren't
errors in the shell code, which it is far too easy to introduce.

Thanks,
Ian

> >> On 23-Sep-2022, at 11:54 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 22/09/22 22:15, Arnaldo Carvalho de Melo wrote:
> >>> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
> >>>> The perf test named “build id cache operations” skips with below
> >>>> error on some distros:
> >>>
> >>> I wonder if we shouldn't instead state that bash is needed?
> >>>
> >>> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
> >>> #!/bin/sh
> >>> #!/bin/bash
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/bash
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/bash
> >>> #!/bin/sh
> >>> #!/bin/bash
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> ⬢[acme@toolbox perf-urgent]$
> >>>
> >>> Opinions?
> >>>
> >>
> >> Please don't change tools/perf/tests/shell/test_intel_pt.sh
> >>
> >> I started using shellcheck on that with the "perf test:
> >> test_intel_pt.sh: Add per-thread test" patch set that I sent.
> >>
> >> FYI, if you use shellcheck on buildid.sh, it shows up issues even
> >> after changing to bash:
> >>
> >> *** Before ***
> >>
> >> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
> >>
> >> In tools/perf/tests/shell/buildid.sh line 69:
> >>       link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
> >>                                      ^-------^ SC2039: In POSIX sh, string indexing is undefined.
> >>                                                ^-----^ SC2039: In POSIX sh, string indexing is undefined.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 77:
> >>       file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
> >>                                      ^-------^ SC2039: In POSIX sh, string indexing is undefined.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 123:
> >>       echo "running: perf record $@"
> >>                                  ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 124:
> >>       ${perf} record --buildid-all -o ${data} $@ &> ${log}
> >>                                               ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
> >>                                                  ^-------^ SC2039: In POSIX sh, &> is undefined.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 126:
> >>               echo "failed: record $@"
> >>                                    ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 131:
> >>       check ${@: -1}
> >>             ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
> >>             ^------^ SC2039: In POSIX sh, string indexing is undefined.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 158:
> >> exit ${err}
> >>    ^----^ SC2154: err is referenced but not assigned.
> >>
> >> For more information:
> >> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
> >> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
> >> https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, &> is undefined.
> >>
> >> *** After ***
> >>
> >> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
> >>
> >> In tools/perf/tests/shell/buildid.sh line 123:
> >>       echo "running: perf record $@"
> >>                                  ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 124:
> >>       ${perf} record --buildid-all -o ${data} $@ &> ${log}
> >>                                               ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 126:
> >>               echo "failed: record $@"
> >>                                    ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 131:
> >>       check ${@: -1}
> >>             ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 158:
> >> exit ${err}
> >>    ^----^ SC2154: err is referenced but not assigned.
> >>
> >> For more information:
> >> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
> >> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
> >> https://www.shellcheck.net/wiki/SC2154 -- err is referenced but not assigned.
> >
>