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

[Ian Rogers <irogers@google.com>]

On Mon, Jan 16, 2023 at 4:00 AM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
>
>
> > On 16-Jan-2023, at 11:47 AM, Ian Rogers <irogers@google.com> wrote:
> >
> > 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
> >
>
> Thanks Ian for the response.
>
> > I think some of what the patch is doing is good, some of it the
>
> Ian, can I take this as an ack for the patch so that Arnaldo can pick this in acme git ?

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> > readability becomes a little harder by not being bash. I'm agnostic as
> > to which approach to take for the fix.
>
> May be we can take this as separate mail thread to get everyone’s opinion on changing all tests in "tools/perf/tests/shell" except test_intel_pt.sh to use “bash" ?
>
> >
> > 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.
>
> Sure, that is a good option to have. I will check on having “shellcheck" as a build option.
>
> Thanks
> Athira
>
> >
> > 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.
>