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

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

> On 17-Jan-2023, at 3:30 AM, Ian Rogers <irogers@google.com> wrote:
> 
> 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

Thanks Ian.

Hi Arnaldo,

I have re-posted the patch 2 ( in this series ) separately in new patchset here :
https://lore.kernel.org/linux-perf-users/20230116050131.17221-2-atrajeev@linux.vnet.ibm.com/

Ack from Ian is for Patch 1. Shall I rebase to upstream and post this patch 1 separately ?

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