[Fuego] [PATCH] Optimiz the assert_has_program code.
Wang, Mingyu
wangmy at cn.fujitsu.com
Wed Aug 14 01:19:10 UTC 2019
Hi Tim,
Thank you for your quick reply.
I'll make some modifies according to these comments and resubmit the patch after 1.5 release.
Best Regards
-----Original Message-----
From: Tim.Bird at sony.com [mailto:Tim.Bird at sony.com]
Sent: Wednesday, August 14, 2019 3:27 AM
To: Wang, Mingyu/王 鸣瑜 <wangmy at cn.fujitsu.com>; fuego at lists.linuxfoundation.org
Subject: RE: [Fuego] [PATCH] Optimiz the assert_has_program code.
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 <wangmy at cn.fujitsu.com>
> ---
> 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
More information about the Fuego
mailing list