Re: [Fuego] [PATCH] Optimiz the assert_has_program code.

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: [email protected] [mailto:[email protected]] 
Sent: Wednesday, August 14, 2019 3:27 AM
To: Wang, Mingyu/王 鸣瑜 <[email protected]>; [email protected]
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 <[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