Re: [Fuego] [PATCH] Optimize the code of assert_has_program.

Hi Tim,

>> Usage:
>> before:
> >    assert_has_program AAA
> >    assert_has_program BBB
>> 
> >after:
> >    assert_has_program "AAA BBB"
> >    assert_has_program "AAA BBB" SEARCH_PATH
>> 
> >Note: SEARCH_PATH is optional.
>Can you explain when SEARCH_PATH is needed?

It is not necessary.
I see that the original is_on_target can search in the specified path, so I added SEARCH_PATH here.
If need to do the same thing with the original assert_has_program, this variable can be deleted.


>> +             if [ ! -s $tmpfile ]
>> +             then
>> +                 find / -name \"\$prg\" | head -n 1 >$tmpfile
>> +             fi
>This will detect the program anywhere in the filesystem, not just on the PATH.
>That changes the behavior of assert_has_program, which currently only looks on the target's PATH.

This code is still used to consider the user-specified path.
If need to do the same thing with the original assert_has_program, this code can be deleted.

>> +             if [ ! -s $tmpfile ]
>> +             then
> >+                 echo \"\$prg\" >$tmpfile ; break;
>This would put the program name into tmpfile, which means that even if the program was not detected, tmpfile will have a non-empty result.
>I don't understand this.
The program was not detected will be saved into tmpfile.
And assert_on_target will get the program and transform to assert_has_program to show  an error  message.


>> +             else
> >+                 > $tmpfile
>I also don't understand why this is needed.  The file is always touched above, so it exists as an empty file.  Why does it need to have the empty redirection into it?
Consider the case where assert_has_program specifies multiple programs at the same time. 

>I can't tell what happens if I provide the name of a file which isn't on the path, but is in the filesystem, or which isn't in the filesystem at all.
>Did you test for these cases?
I tested a variety of situations, including specifying multiple programs at the same time。
And existing or non-existent  program  is also tested.

When I designed assert_has_program, I considered the existing is_on_target and is_on_target_path and wanted to be as compatible as possible with them so that they could be replaced in the future.

best regards!



-----Original Message-----
From: [email protected] [mailto:[email protected]] 
Sent: Saturday, September 21, 2019 10:25 AM
To: Wang, Mingyu/王 鸣瑜 <[email protected]>; [email protected]
Subject: RE: [Fuego] [PATCH] Optimize the code of assert_has_program.

I'm not sure this gives the same results at the original code.
See my comments inline below.

Also, combining multiple programs into a single 'assert_has_program'
lines will make it a bit more difficult to implement a cache system for programs detected on the board.

I will have to think about this one.

> -----Original Message-----
> From: Wang Mingyu on Tuesday, September 03, 2019 3:09 PM
> 
> 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"
>     assert_has_program "AAA BBB" SEARCH_PATH
> 
> Note: SEARCH_PATH is optional.

Can you explain when SEARCH_PATH is needed?

> 
> Signed-off-by: Wang Mingyu <[email protected]>
> ---
>  scripts/functions.sh  | 65
> +++++++++++++++++++++++++++++++++++++++----
>  scripts/need_check.sh | 11 ++------
>  2 files changed, 61 insertions(+), 15 deletions(-)
> 
> diff --git a/scripts/functions.sh b/scripts/functions.sh index 
> 0fa80b8..3aea0c8 100755
> --- a/scripts/functions.sh
> +++ b/scripts/functions.sh
> @@ -1010,6 +1010,57 @@ function is_on_target_path {
>      is_on_target $1 $2 $TARGET_PATH
>  }
> 
> +# 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 is the path 
> +of target to search the program/file # $3 - tmpfile to save the 
> +detect result function set_cmd_str {
In general, I like this approach of executing a command on the target that does the search loop.

> +   tmpfile=$3
> +   cmd_str="touch $tmpfile
> +         # split $1 on whitespace, without file globbing
> +         for prg in \$(echo $1 | tr \" \" \"\\n\") ; do
> +             # split search path on colon
> +             for d in \$(echo $2 | tr \":\" \"\\n\") ; do
> +                 # execute a command on the target to detect \$d/\$prg
> +                 if [ -z \"\$(cat $tmpfile)\" -a -e \"\$d/\$prg\" ]
> +                 then
> +                     echo \"\$d/\$prg\" >$tmpfile ;break;
> +                 fi
> +             done
> +             if [ ! -s $tmpfile ]
> +             then
> +                 find / -name \"\$prg\" | head -n 1 >$tmpfile
> +             fi
This will detect the program anywhere in the filesystem, not just on the PATH.
That changes the behavior of assert_has_program, which currently only looks on the target's PATH.

> +             if [ ! -s $tmpfile ]
> +             then
> +                 echo \"\$prg\" >$tmpfile ; break;
This would put the program name into tmpfile, which means that even if the program was not detected, tmpfile will have a non-empty result.
I don't understand this.

> +             else
> +                 > $tmpfile
I also don't understand why this is needed.  The file is always touched above, so it exists as an empty file.  Why does it need to have the empty redirection into it?

> +             fi
> +         done"
> +}
> +
> +# 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 is the path of target to search the 
> +program/file (optional, default:
> $PATH)
> +function assert_on_target {
> +    local TARGET_PATH=${3}
> +    if [ -z "$TARGET_PATH" ]; then
> +       TARGET_PATH=$(cmd "echo \$PATH")
> +    fi
> +
> +    tmpfile=$(mktemp /tmp/found_loc.XXXXXX)
> +    set_cmd_str "$1" $TARGET_PATH $tmpfile
> +    cmd "$cmd_str"
> +    get $tmpfile $tmpfile
> +    if [ -s $tmpfile ] ; then
> +        export $2=$(cat $tmpfile)
> +    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  
> # $1 - the file (library) we want to search for in the SDK  # $2 - 
> variable to set the location (if the file is found) @@ -1036,13 
> +1087,15 @@ 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.
> +# $2 is the path of target to search the program/file (optional, default:
> $PATH)
>  function assert_has_program {
> -   upName=${1^^}
> -   progVar=PROGRAM_${upName//[-,.]/_}
> -   is_on_target_path $1 ${progVar}
> -   assert_define ${progVar} "Missing '$1' program on target board"
> +   assert_on_target "$1" prgName $2
> +   if [ -n "$prgName" ]
> +   then
> +       upName=${prgName^^}
> +       progVar=PROGRAM_${upName//[-,.]/_}
> +       assert_define ${progVar} "Missing '$prgName' 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..9bb0098 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
> +    assert_on_target "$1" prg
> +    if [ -n "$prg" ] ; then
>        echo -e "\n\nABORTED: Expected command \"$prg\" on the target, 
> 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 not sure this does the same thing as the original assert_has_program.

I can't tell what happens if I provide the name of a file which isn't on the path, but is in the filesystem, or which isn't in the filesystem at all.
Did you test for these cases?
 -- Tim