Thank you for this submission. I appreciate the effort to address the
efficiency of the test_pre_check phase (which needs improvement).
However, this patch is too intrusive to introduce before the 1.5 release.
I will provide some feedback below, but please hold off on this patch
for a few weeks while I try to get the 1.5 release out.
-- Tim
> -----Original Message-----
> From: Wang Mingyu
>
> It is faster to check the prgoram or file on the target.
prgoram -> program
>
> Usage:
> before:
> assert_has_program AAA
> assert_has_program BBB
>
> after:
> assert_has_program "AAA BBB"
>
> Signed-off-by: Wang Mingyu <[email protected]>
> ---
> scripts/functions.sh | 74 ++++++++++++++++++++++++-------------------
> scripts/need_check.sh | 13 ++------
> 2 files changed, 45 insertions(+), 42 deletions(-)
>
> diff --git a/scripts/functions.sh b/scripts/functions.sh
> index 0fa80b8..d100db3 100755
> --- a/scripts/functions.sh
> +++ b/scripts/functions.sh
> @@ -977,37 +977,48 @@ function hd_test_clean_umount() {
>
> # check for a program or file on the target, and set a variable if it's present
> # $1 - file, dir or program on target
> -# $2 - variable to set during the build
> -# $3 - (optional) set of paths to look for on target
> -# if $3 is not specified, a find is done from the root
> -# this requires the 'find' command on the target
> -function is_on_target {
This removes the function is_on_target() (unless I'm missing somewhere
it gets added back). However, is_on_target is used
in several tests:
./Functional.neon/fuego_test.sh: is_on_target libneon.so.27 LIB_NEON /lib:/usr/lib:/usr/local/lib
./Functional.neon/fuego_test.sh: is_on_target libproxy.so.1 LIB_PROXY /lib:/usr/lib:/usr/local/lib
./Functional.fuego_test_phases/fuego_test.sh: is_on_target nohup PROGRAM_NOHUP /usr/bin
./Benchmark.blobsallad/fuego_test.sh: is_on_target xrandr XRANDR_PROGRAM /usr/bin
./Benchmark.backfire/fuego_test.sh: is_on_target insmod PROGRAM_INSMOD /sbin:/usr/sbin:/usr/local/sbin
./Benchmark.backfire/fuego_test.sh: is_on_target rmmod PROGRAM_RMMOD /sbin:/usr/sbin:/usr/local/sbin
./Functional.boost/fuego_test.sh: is_on_target $lib $LIB_VAR_NAME /lib:/usr/lib:/usr/local/lib:/usr/lib/$ARCH-linux-*/:/usr/lib/$TOOLCHAIN-linux-*/
./Functional.LTP/fuego_test.sh: is_on_target ${PROGRAM} FOUND /bin:/usr/bin:/usr/sbin:/usr/local/bin
./Functional.LTP/fuego_test.sh: is_on_target runltp PROGRAM_RUNLTP $FUNCTIONAL_LTP_HOMEDIR
./Functional.year2038/fuego_test.sh: is_on_target perl PROGRAM_PERL /usr/bin
./Benchmark.dd/fuego_test.sh:# * can't use is_on_target to detect it
./Benchmark.vuls/fuego_test.sh: is_on_target reboot-notifier SERVICE_REBOOT_NOTIFIER /etc/cron.daily/:/etc/default
./Benchmark.vuls/fuego_test.sh: is_on_target aptitude-curses PROGRAM_APTITUDE /usr/bin
./Benchmark.vuls/fuego_test.sh: is_on_target go PROGRAM_GO $PATH
./Functional.lwip/fuego_test.sh: is_on_target liblwip.so LIB_LWIP /lib:/usr/lib:/usr/local/lib
./Functional.autopkgtest/fuego_test.sh: is_on_target autopkgtest PROGRAM_AUTOPKGTEST /usr/bin
./Functional.fuse/fuego_test.sh: is_on_target libfuse.so.2 LIB_FUSE /usr/lib
./Functional.fuego_abort/fuego_test.sh: is_on_target nohup PROGRAM_NOHUP /usr/bin
Unless we also fixup those uses of is_on_target(), we can't remove this function.
I would recommend leaving the current is_on_target, and migrating those callers
of the routine over time (if that seems appropriate).
> - # FIXTHIS: race condition
> - tmpfile=$(mktemp /tmp/found_loc.XXXXXX)
> - cmd "touch $tmpfile"
> - if [ -z "$3" ] ; then
> - safe_cmd "find / -name \"$1\" | head -n 1 >$tmpfile"
> - else
> - # split search path on colon
> - for d in $(echo "$3" | tr ":" "\n") ; do
> - # execute a command on the target to detect $d/$1
> - cmd "if [ -z \"\$(cat $tmpfile)\" -a -e \"$d/$1\" ] ; then echo \"$d/$1\"
> >$tmpfile ; fi"
> - done
> - fi
> - get $tmpfile $tmpfile
> - LOCATION=$(cat $tmpfile)
> - export $2=$LOCATION
> - cmd "rm $tmpfile"
> - rm -f $tmpfile # -f for tests running on the host
> +# $2 - tmpfile to save the detect result
> +function set_cmd_str {
> + tmpfile=$2
> + cmd_str="touch $tmpfile
> + for prg in \$(echo $1 | tr \" \" \"\\n\")
Please add a trailing ';' on this line. this serves to separate
the set of strings from the following lines.
I'm not sure what the 'tr' is doing here. This is a bit tricky.
Please add a comment describing what it's doing.
> + do
> + # execute command type on the target to detect \$prg
> + if type -P \$prg
is 'type -P' supported by all shells? I don't think it's supported by 'dash',
which is commonly used on Ubuntu systems.
$ /bin/dash
$ type -P ls
-P: not found
ls is /bin/ls
> + then
> + echo \"\$d/\$prg\" >$tmpfile
> + fi
> + if [ ! -s $tmpfile ]
> + then
> + find / -name \"\$prg\" | head -n 1 >$tmpfile
> + fi
> + if [ ! -s $tmpfile ]
> + then
> + echo \"\$prg\" >$tmpfile
> + break;
> + else
> + > $tmpfile
> + fi
> + done"
> }
Although it's tricky, I like the approach of creating a command that
executes on target.
>
> -# check for a program or file on a directory listed on the PATH on the target,
> -# and set a variable if it's present
> +# check for a program or file on the target, and set a variable if it's present
> # $1 - file, dir or program on target
> # $2 - variable to set during the build
> +# $3 - name of program that is not found on the target
> function is_on_target_path {
> - TARGET_PATH=$(cmd "echo \$PATH")
> - is_on_target $1 $2 $TARGET_PATH
> + tmpfile=$(mktemp /tmp/found_loc.XXXXXX)
> + set_cmd_str "$1" $tmpfile
> + cmd "$cmd_str"
> + get $tmpfile $tmpfile
> + if [ -s $tmpfile ] ; then
> + prg=$(cat $tmpfile)
> + upName=${prg^^}
> + export $2=PROGRAM_${upName//[-,.]/_}
> + export $3=$prg
> + fi
> + cmd "rm $tmpfile"
> + rm -f $tmpfile # -f for tests running on the host
> }
>
> # check for a library on the SDK, and set a variable if it's present
> @@ -1036,13 +1047,12 @@ function is_on_sdk {
>
> # check for a program or file on the target, and send message if the program
> or file is missing
> # $1 has the program that is required on the target board
> -# this has the side effect of defining PROGRAM_$1 (uppercased)
> -# with the value as the directory where $1 is found on the board.
> function assert_has_program {
> - upName=${1^^}
> - progVar=PROGRAM_${upName//[-,.]/_}
You removed the setting of a PROGRAM_<var> variable
with the path of a program, when a program is found.
I had planned to use this side effect in future code.
(But maybe that needs a re-think.)
> - is_on_target_path $1 ${progVar}
> - assert_define ${progVar} "Missing '$1' program on target board"
> + is_on_target_path "$1" prog_path prg
> + if [ ! -z "$prog_path" ]
This should be '-n' instead of '! -z'
> + then
> + assert_define ${prog_path} "Missing '$prg' program on target board"
> + fi
> }
>
> # check for a module on the target, and abort if it is missing
> diff --git a/scripts/need_check.sh b/scripts/need_check.sh
> index c9bbb75..5912ee1 100755
> --- a/scripts/need_check.sh
> +++ b/scripts/need_check.sh
> @@ -319,18 +319,11 @@ function check_root {
> # check if those specified commands exist on board
> # $1 has single string with a list of command entries to check for
> function check_program {
> - # split $1 on whitespace, without file globbing
> - set -f
> - arg_array=($1)
> - set +f
> -
> - for prg in "${arg_array[@]}" ; do
> - is_on_target_path $prg prg_path
> - if [ -z "$prg_path" ] ; then
> - echo -e "\n\nABORTED: Expected command \"$prg\" on the target, but
> it's not there!"
> + is_on_target_path "$1" prg_path prg
> + if [ ! -z "$prg_path" ] ; then
Should use '-n' instead of '! -z'
> + echo -e "\n\nABORTED: Expected command \"$prg\" is on the target,
should remove 'is' here.
> but it's not there!"
> return 1
> fi
> - done
>
> # return OK if all necessary commands exist on the target
> return 0
> --
> 2.17.1
I'm going to hold off applying this until the 1.5 release, because it touches
core code (and has some unresolved issues).
-- Tim
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.