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

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> 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 ?

> 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.