* [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test
@ 2022-09-21 17:08 Athira Rajeev
2022-09-21 17:08 ` [PATCH 2/2] tools/perf/tests: Fix build id test check for PE file Athira Rajeev
2022-09-22 19:15 ` [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test Arnaldo Carvalho de Melo
0 siblings, 2 replies; 11+ messages in thread
From: Athira Rajeev @ 2022-09-21 17:08 UTC (permalink / raw)
To: acme, jolsa, mpe
Cc: maddy, rnsastry, kjain, linux-perf-users, disgoel, linuxppc-dev
The perf test named “build id cache operations” skips with below
error on some distros:
<<>>
78: build id cache operations :
test child forked, pid 111101
WARNING: wine not found. PE binaries will not be run.
test binaries: /tmp/perf.ex.SHA1.PKz /tmp/perf.ex.MD5.Gt3 ./tests/shell/../pe-file.exe
DEBUGINFOD_URLS=
Adding 4abd406f041feb4f10ecde3fc30fd0639e1a91cb /tmp/perf.ex.SHA1.PKz: Ok
build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb
./tests/shell/buildid.sh: 69: ./tests/shell/buildid.sh: Bad substitution
test child finished with -2
build id cache operations: Skip
<<>>
The test script "tests/shell/buildid.sh" uses some of the
string substitution ways which are supported in bash, but not in
"sh" or other shells. Above error on line number 69 that reports
"Bad substitution" is:
<<>>
link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
<<>>
Here the way of getting first two characters from id ie,
${id:0:2} and similarly expressions like ${id:2} is not
recognised in "sh". So the line errors and instead of
hitting failure, the test gets skipped as shown in logs.
So the syntax issue causes test not to be executed in
such cases. Similarly usage : "${@: -1}" [ to pick last
argument passed to a function] in “test_record” doesn’t
work in all distros.
Fix this by using alternative way with "cut" command
to pick "n" characters from the string. Also fix the usage
of “${@: -1}” to work in all cases.
Another usage in “test_record” is:
<<>>
${perf} record --buildid-all -o ${data} $@ &> ${log}
<<>>
This causes the perf record to start in background and
Results in the data file not being created by the time
"check" function is invoked. Below log shows perf record
result getting displayed after the call to "check" function.
<<>>
running: perf record /tmp/perf.ex.SHA1.EAU
build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb
link: /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb
failed: link /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb does not exist
test child finished with -1
build id cache operations: FAILED!
root@machine:~/athira/linux/tools/perf# Couldn't synthesize bpf events.
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.010 MB /tmp/perf.data.bFF ]
<<>>
Fix this by redirecting output instead of using “&” which
starts the command in background.
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
tools/perf/tests/shell/buildid.sh | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh
index f05670d1e39e..3512c4423d48 100755
--- a/tools/perf/tests/shell/buildid.sh
+++ b/tools/perf/tests/shell/buildid.sh
@@ -66,7 +66,7 @@ check()
esac
echo "build id: ${id}"
- link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
+ link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-)
echo "link: ${link}"
if [ ! -h $link ]; then
@@ -74,7 +74,7 @@ check()
exit 1
fi
- file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
+ file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf
echo "file: ${file}"
if [ ! -x $file ]; then
@@ -117,20 +117,22 @@ test_record()
{
data=$(mktemp /tmp/perf.data.XXX)
build_id_dir=$(mktemp -d /tmp/perf.debug.XXX)
- log=$(mktemp /tmp/perf.log.XXX)
+ log_out=$(mktemp /tmp/perf.log.out.XXX)
+ log_err=$(mktemp /tmp/perf.log.err.XXX)
perf="perf --buildid-dir ${build_id_dir}"
+ eval last=\${$#}
echo "running: perf record $@"
- ${perf} record --buildid-all -o ${data} $@ &> ${log}
+ ${perf} record --buildid-all -o ${data} $@ 1>${log_out} 2>${log_err}
if [ $? -ne 0 ]; then
echo "failed: record $@"
- echo "see log: ${log}"
+ echo "see log: ${log_err}"
exit 1
fi
- check ${@: -1}
+ check $last
- rm -f ${log}
+ rm -f ${log_out} ${log_err}
rm -rf ${build_id_dir}
rm -rf ${data}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/2] tools/perf/tests: Fix build id test check for PE file 2022-09-21 17:08 [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test Athira Rajeev @ 2022-09-21 17:08 ` Athira Rajeev 2022-09-22 19:15 ` [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test Arnaldo Carvalho de Melo 1 sibling, 0 replies; 11+ messages in thread From: Athira Rajeev @ 2022-09-21 17:08 UTC (permalink / raw) To: acme, jolsa, mpe Cc: maddy, rnsastry, kjain, linux-perf-users, disgoel, linuxppc-dev Perf test "build id cache operations" fails for PE executable. Logs below from powerpc system. Same is observed on x86 as well. <<>> Adding 5a0fd882b53084224ba47b624c55a469 ./tests/shell/../pe-file.exe: Ok build id: 5a0fd882b53084224ba47b624c55a469 link: /tmp/perf.debug.w0V/.build-id/5a/0fd882b53084224ba47b624c55a469 file: /tmp/perf.debug.w0V/.build-id/5a/../../root/athira/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf failed: file /tmp/perf.debug.w0V/.build-id/5a/../../root/athira/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf does not exist test child finished with -1 ---- end ---- build id cache operations: FAILED! <<>> The test tries to do: <<>> mkdir /tmp/perf.debug.TeY1 perf --buildid-dir /tmp/perf.debug.TeY1 buildid-cache -v -a ./tests/shell/../pe-file.exe <<>> The option "--buildid-dir" sets the build id cache directory as /tmp/perf.debug.TeY1. The option given to buildid-cahe, ie "-a ./tests/shell/../pe-file.exe", is to add the pe-file.exe to the cache. The testcase, sets buildid-dir and adds the file: pe-file.exe to build id cache. To check if the command is run successfully, "check" function looks for presence of the file in buildid cache directory. But the check here expects the added file to be executable. Snippet below: <<>> if [ ! -x $file ]; then echo "failed: file ${file} does not exist" exit 1 fi <<>> Buildid test is done for sha1 binary, md5 binary and also for PE file. The firt two binaries are created at runtime by compiling with "--build-id" option and hence the check for sha1/md5 test should use [ ! -x ]. But in case of PE file, the permission for this input file is rw-r--r-- Hence the file added to build id cache has same permissoin Original file: ls tests/pe-file.exe | xargs stat --printf "%n %A \n" tests/pe-file.exe -rw-r--r-- buildid cache file: ls /tmp/perf.debug.w0V/.build-id/5a/../../root/athira/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf | xargs stat --printf "%n %A \n" /tmp/perf.debug.w0V/.build-id/5a/../../root/athira/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf -rw-r--r-- Fix the test to match with the permission of original file in case of FE file. ie if the "tests/pe-file.exe" file is not having exec permission, just check for existence of the buildid file using [ ! -e <file> ] Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> --- tools/perf/tests/shell/buildid.sh | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh index 3512c4423d48..75f5117c3417 100755 --- a/tools/perf/tests/shell/buildid.sh +++ b/tools/perf/tests/shell/buildid.sh @@ -77,7 +77,20 @@ check() file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf echo "file: ${file}" - if [ ! -x $file ]; then + # Check for file permission of orginal file + # in case of pe-file.exe file + echo $1 | grep ".exe" + if [ $? -eq 0 ]; then + if [ -x $1 -a ! -x $file ]; then + echo "failed: file ${file} executable does not exist" + exit 1 + fi + + if [ ! -x $file -a ! -e $file ]; then + echo "failed: file ${file} does not exist" + exit 1 + fi + elif [ ! -x $file ]; then echo "failed: file ${file} does not exist" exit 1 fi -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2022-09-21 17:08 [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test Athira Rajeev 2022-09-21 17:08 ` [PATCH 2/2] tools/perf/tests: Fix build id test check for PE file Athira Rajeev @ 2022-09-22 19:15 ` Arnaldo Carvalho de Melo 2022-09-22 23:44 ` Ian Rogers 2022-09-23 6:24 ` Adrian Hunter 1 sibling, 2 replies; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-09-22 19:15 UTC (permalink / raw) To: Athira Rajeev Cc: Ian Rogers, maddy, rnsastry, Adrian Hunter, linux-perf-users, Jiri Olsa, kjain, disgoel, linuxppc-dev 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? - Arnaldo > <<>> > 78: build id cache operations : > test child forked, pid 111101 > WARNING: wine not found. PE binaries will not be run. > test binaries: /tmp/perf.ex.SHA1.PKz /tmp/perf.ex.MD5.Gt3 ./tests/shell/../pe-file.exe > DEBUGINFOD_URLS= > Adding 4abd406f041feb4f10ecde3fc30fd0639e1a91cb /tmp/perf.ex.SHA1.PKz: Ok > build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb > ./tests/shell/buildid.sh: 69: ./tests/shell/buildid.sh: Bad substitution > test child finished with -2 > build id cache operations: Skip > <<>> > > The test script "tests/shell/buildid.sh" uses some of the > string substitution ways which are supported in bash, but not in > "sh" or other shells. Above error on line number 69 that reports > "Bad substitution" is: > > <<>> > link=${build_id_dir}/.build-id/${id:0:2}/${id:2} > <<>> > > Here the way of getting first two characters from id ie, > ${id:0:2} and similarly expressions like ${id:2} is not > recognised in "sh". So the line errors and instead of > hitting failure, the test gets skipped as shown in logs. > So the syntax issue causes test not to be executed in > such cases. Similarly usage : "${@: -1}" [ to pick last > argument passed to a function] in “test_record” doesn’t > work in all distros. > > Fix this by using alternative way with "cut" command > to pick "n" characters from the string. Also fix the usage > of “${@: -1}” to work in all cases. > > Another usage in “test_record” is: > <<>> > ${perf} record --buildid-all -o ${data} $@ &> ${log} > <<>> > > This causes the perf record to start in background and > Results in the data file not being created by the time > "check" function is invoked. Below log shows perf record > result getting displayed after the call to "check" function. > > <<>> > running: perf record /tmp/perf.ex.SHA1.EAU > build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb > link: /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb > failed: link /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb does not exist > test child finished with -1 > build id cache operations: FAILED! > root@machine:~/athira/linux/tools/perf# Couldn't synthesize bpf events. > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.010 MB /tmp/perf.data.bFF ] > <<>> > > Fix this by redirecting output instead of using “&” which > starts the command in background. > > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > --- > tools/perf/tests/shell/buildid.sh | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh > index f05670d1e39e..3512c4423d48 100755 > --- a/tools/perf/tests/shell/buildid.sh > +++ b/tools/perf/tests/shell/buildid.sh > @@ -66,7 +66,7 @@ check() > esac > echo "build id: ${id}" > > - link=${build_id_dir}/.build-id/${id:0:2}/${id:2} > + link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-) > echo "link: ${link}" > > if [ ! -h $link ]; then > @@ -74,7 +74,7 @@ check() > exit 1 > fi > > - file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf > + file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf > echo "file: ${file}" > > if [ ! -x $file ]; then > @@ -117,20 +117,22 @@ test_record() > { > data=$(mktemp /tmp/perf.data.XXX) > build_id_dir=$(mktemp -d /tmp/perf.debug.XXX) > - log=$(mktemp /tmp/perf.log.XXX) > + log_out=$(mktemp /tmp/perf.log.out.XXX) > + log_err=$(mktemp /tmp/perf.log.err.XXX) > perf="perf --buildid-dir ${build_id_dir}" > + eval last=\${$#} > > echo "running: perf record $@" > - ${perf} record --buildid-all -o ${data} $@ &> ${log} > + ${perf} record --buildid-all -o ${data} $@ 1>${log_out} 2>${log_err} > if [ $? -ne 0 ]; then > echo "failed: record $@" > - echo "see log: ${log}" > + echo "see log: ${log_err}" > exit 1 > fi > > - check ${@: -1} > + check $last > > - rm -f ${log} > + rm -f ${log_out} ${log_err} > rm -rf ${build_id_dir} > rm -rf ${data} > } > -- > 2.17.1 -- - Arnaldo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2022-09-22 19:15 ` [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test Arnaldo Carvalho de Melo @ 2022-09-22 23:44 ` Ian Rogers 2022-09-23 6:24 ` Adrian Hunter 1 sibling, 0 replies; 11+ messages in thread From: Ian Rogers @ 2022-09-22 23:44 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Athira Rajeev, rnsastry, Adrian Hunter, linux-perf-users, maddy, Jiri Olsa, kjain, disgoel, linuxppc-dev On Thu, Sep 22, 2022 at 12:15 PM Arnaldo Carvalho de Melo <acme@kernel.org> 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? +1 to bash. Perhaps python longer term? The XML test output generated by things like kunit is possible to generate from either bash or python, but in my experience the python stuff feels better built. Thanks, Ian > - Arnaldo > > > <<>> > > 78: build id cache operations : > > test child forked, pid 111101 > > WARNING: wine not found. PE binaries will not be run. > > test binaries: /tmp/perf.ex.SHA1.PKz /tmp/perf.ex.MD5.Gt3 ./tests/shell/../pe-file.exe > > DEBUGINFOD_URLS= > > Adding 4abd406f041feb4f10ecde3fc30fd0639e1a91cb /tmp/perf.ex.SHA1.PKz: Ok > > build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb > > ./tests/shell/buildid.sh: 69: ./tests/shell/buildid.sh: Bad substitution > > test child finished with -2 > > build id cache operations: Skip > > <<>> > > > > The test script "tests/shell/buildid.sh" uses some of the > > string substitution ways which are supported in bash, but not in > > "sh" or other shells. Above error on line number 69 that reports > > "Bad substitution" is: > > > > <<>> > > link=${build_id_dir}/.build-id/${id:0:2}/${id:2} > > <<>> > > > > Here the way of getting first two characters from id ie, > > ${id:0:2} and similarly expressions like ${id:2} is not > > recognised in "sh". So the line errors and instead of > > hitting failure, the test gets skipped as shown in logs. > > So the syntax issue causes test not to be executed in > > such cases. Similarly usage : "${@: -1}" [ to pick last > > argument passed to a function] in “test_record” doesn’t > > work in all distros. > > > > Fix this by using alternative way with "cut" command > > to pick "n" characters from the string. Also fix the usage > > of “${@: -1}” to work in all cases. > > > > Another usage in “test_record” is: > > <<>> > > ${perf} record --buildid-all -o ${data} $@ &> ${log} > > <<>> > > > > This causes the perf record to start in background and > > Results in the data file not being created by the time > > "check" function is invoked. Below log shows perf record > > result getting displayed after the call to "check" function. > > > > <<>> > > running: perf record /tmp/perf.ex.SHA1.EAU > > build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb > > link: /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb > > failed: link /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb does not exist > > test child finished with -1 > > build id cache operations: FAILED! > > root@machine:~/athira/linux/tools/perf# Couldn't synthesize bpf events. > > [ perf record: Woken up 1 times to write data ] > > [ perf record: Captured and wrote 0.010 MB /tmp/perf.data.bFF ] > > <<>> > > > > Fix this by redirecting output instead of using “&” which > > starts the command in background. > > > > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > > --- > > tools/perf/tests/shell/buildid.sh | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh > > index f05670d1e39e..3512c4423d48 100755 > > --- a/tools/perf/tests/shell/buildid.sh > > +++ b/tools/perf/tests/shell/buildid.sh > > @@ -66,7 +66,7 @@ check() > > esac > > echo "build id: ${id}" > > > > - link=${build_id_dir}/.build-id/${id:0:2}/${id:2} > > + link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-) > > echo "link: ${link}" > > > > if [ ! -h $link ]; then > > @@ -74,7 +74,7 @@ check() > > exit 1 > > fi > > > > - file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf > > + file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf > > echo "file: ${file}" > > > > if [ ! -x $file ]; then > > @@ -117,20 +117,22 @@ test_record() > > { > > data=$(mktemp /tmp/perf.data.XXX) > > build_id_dir=$(mktemp -d /tmp/perf.debug.XXX) > > - log=$(mktemp /tmp/perf.log.XXX) > > + log_out=$(mktemp /tmp/perf.log.out.XXX) > > + log_err=$(mktemp /tmp/perf.log.err.XXX) > > perf="perf --buildid-dir ${build_id_dir}" > > + eval last=\${$#} > > > > echo "running: perf record $@" > > - ${perf} record --buildid-all -o ${data} $@ &> ${log} > > + ${perf} record --buildid-all -o ${data} $@ 1>${log_out} 2>${log_err} > > if [ $? -ne 0 ]; then > > echo "failed: record $@" > > - echo "see log: ${log}" > > + echo "see log: ${log_err}" > > exit 1 > > fi > > > > - check ${@: -1} > > + check $last > > > > - rm -f ${log} > > + rm -f ${log_out} ${log_err} > > rm -rf ${build_id_dir} > > rm -rf ${data} > > } > > -- > > 2.17.1 > > -- > > - Arnaldo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2022-09-22 19:15 ` [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test Arnaldo Carvalho de Melo 2022-09-22 23:44 ` Ian Rogers @ 2022-09-23 6:24 ` Adrian Hunter 2022-09-28 4:54 ` Athira Rajeev 1 sibling, 1 reply; 11+ messages in thread From: Adrian Hunter @ 2022-09-23 6:24 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Athira Rajeev Cc: Ian Rogers, maddy, rnsastry, linux-perf-users, Jiri Olsa, kjain, disgoel, linuxppc-dev 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2022-09-23 6:24 ` Adrian Hunter @ 2022-09-28 4:54 ` Athira Rajeev 2023-01-16 5:02 ` Athira Rajeev 0 siblings, 1 reply; 11+ messages in thread From: Athira Rajeev @ 2022-09-28 4:54 UTC (permalink / raw) To: Adrian Hunter Cc: Ian Rogers, maddy, Nageswara Sastry, Kajol Jain, Arnaldo Carvalho de Melo, linux-perf-users, Jiri Olsa, Disha Goel, linuxppc-dev 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 > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2022-09-28 4:54 ` Athira Rajeev @ 2023-01-16 5:02 ` Athira Rajeev 2023-01-16 6:17 ` Ian Rogers 0 siblings, 1 reply; 11+ messages in thread From: Athira Rajeev @ 2023-01-16 5:02 UTC (permalink / raw) To: Adrian Hunter, Arnaldo Carvalho de Melo, Ian Rogers Cc: maddy, Nageswara Sastry, Kajol Jain, linux-perf-users, Jiri Olsa, Disha Goel, linuxppc-dev > 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 >> 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. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2023-01-16 5:02 ` Athira Rajeev @ 2023-01-16 6:17 ` Ian Rogers 2023-01-16 11:59 ` Athira Rajeev 0 siblings, 1 reply; 11+ messages in thread From: Ian Rogers @ 2023-01-16 6:17 UTC (permalink / raw) To: Athira Rajeev Cc: maddy, Nageswara Sastry, Kajol Jain, Adrian Hunter, Arnaldo Carvalho de Melo, linux-perf-users, Jiri Olsa, Disha Goel, linuxppc-dev 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. > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2023-01-16 6:17 ` Ian Rogers @ 2023-01-16 11:59 ` Athira Rajeev 2023-01-16 22:00 ` Ian Rogers 0 siblings, 1 reply; 11+ messages in thread From: Athira Rajeev @ 2023-01-16 11:59 UTC (permalink / raw) To: Ian Rogers, Arnaldo Carvalho de Melo Cc: maddy, Nageswara Sastry, Kajol Jain, Adrian Hunter, linux-perf-users, Jiri Olsa, Disha Goel, linuxppc-dev > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2023-01-16 11:59 ` Athira Rajeev @ 2023-01-16 22:00 ` Ian Rogers 2023-01-17 13:14 ` Athira Rajeev 0 siblings, 1 reply; 11+ messages in thread From: Ian Rogers @ 2023-01-16 22:00 UTC (permalink / raw) To: Athira Rajeev Cc: maddy, Nageswara Sastry, Kajol Jain, Adrian Hunter, Arnaldo Carvalho de Melo, linux-perf-users, Jiri Olsa, Disha Goel, linuxppc-dev 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. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test 2023-01-16 22:00 ` Ian Rogers @ 2023-01-17 13:14 ` Athira Rajeev 0 siblings, 0 replies; 11+ messages in thread From: Athira Rajeev @ 2023-01-17 13:14 UTC (permalink / raw) To: Ian Rogers, Arnaldo Carvalho de Melo Cc: maddy, Nageswara Sastry, Kajol Jain, Adrian Hunter, linux-perf-users, Jiri Olsa, Disha Goel, linuxppc-dev > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-01-17 13:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-21 17:08 [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test Athira Rajeev 2022-09-21 17:08 ` [PATCH 2/2] tools/perf/tests: Fix build id test check for PE file Athira Rajeev 2022-09-22 19:15 ` [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test Arnaldo Carvalho de Melo 2022-09-22 23:44 ` Ian Rogers 2022-09-23 6:24 ` Adrian Hunter 2022-09-28 4:54 ` Athira Rajeev 2023-01-16 5:02 ` Athira Rajeev 2023-01-16 6:17 ` Ian Rogers 2023-01-16 11:59 ` Athira Rajeev 2023-01-16 22:00 ` Ian Rogers 2023-01-17 13:14 ` Athira Rajeev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).