On Mon, Jan 16, 2023 at 4:00 AM Athira Rajeev <atraj...@linux.vnet.ibm.com> wrote: > > > > > On 16-Jan-2023, at 11:47 AM, Ian Rogers <irog...@google.com> wrote: > > > > On Sun, Jan 15, 2023 at 9:03 PM Athira Rajeev > > <atraj...@linux.vnet.ibm.com> wrote: > >> > >> > >> > >>> On 28-Sep-2022, at 10:24 AM, Athira Rajeev <atraj...@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 <irog...@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.hun...@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. >
