Re: [Fuego] [PATCH v2] lmbench: Use 'SCRIPTS_PATH' to specify distribution installed binaries path

OK - this one I have some suggestions for...

See comments inline below.

> -----Original Message-----
> From: [email protected] <[email protected]>
> 
> From: venkata pyla <[email protected]>
> 
> Use 'SCRIPTS_PATH' variable to specify lmbench scripts directory if the
> lmbench is installed by distribution, and then use get_program_path to
> search the binaries in the 'SCRIPTS_PATH'.
> 
> default 'SCRIPTS_PATH' directory is $BOARD_TESTDIR/fuego.$TESTDIR/scripts
> and can be specify using --dynamic-vars
> 
> $ ftc run-test -b local -t Benchmark.lmbench2 -p ptrca \
>                --dynamic-vars "{'SCRIPTS_PATH':'/usr/lib/lmbench/scripts'}"

I'd prefer it if this was not needed to run the test.  As an optional aid
for a truly bizarre setup, I think this is OK.  But if /usr/lib/lmbench is the
default location for lmbench assets (that is, the default install location),
then I'd rather just auto-detect that the assets are there, rather than requiring
the user to need to specify that path.

On my Ubuntu 20.04 system, the lmbench package puts things all over
the place:
/usr/lib/lmbench/scripts/*
/usr/lib/lmbench/bin/x86_64-linux-gnu/*
/var/lib/lmbench/config
/var/lib/lmbench/results

And, a helper wrapper seems to be at /usr/bin/lmbench-run

Can you tell me where the scripts, bin, config and results directories are
for the lmbench package in your distribution?
In particular, I'm curious if your config or results are under /usr/lib/lmbench
or /var/lib/lmbench.  It appears from this patch, that they are under
/usr/lib/lmbench, as you use paths relative to SCRIPTS_DIR to access them.


Also, just as a note, the following is equivalent syntax for specifying
a dynamic variable (and is a bit easier to read, IMHO):
$ ftc run-test -b local -t Benchmark.lmbench2 -p ptrca \
               --dynamic-vars SCRIPTS_PATH=/usr/lib/lmbench/scripts

(Also, you don't need the 'Benchmark.' prefix on the test name anymore)

> Signed-off-by: venkata pyla <[email protected]>
> ---
>  tests/Benchmark.lmbench2/fuego_test.sh | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/Benchmark.lmbench2/fuego_test.sh b/tests/Benchmark.lmbench2/fuego_test.sh
> index d07606c..02a3b8e 100755
> --- a/tests/Benchmark.lmbench2/fuego_test.sh
> +++ b/tests/Benchmark.lmbench2/fuego_test.sh
> @@ -22,20 +22,23 @@ function test_deploy {
>  }
> 
>  function test_run {
> +   if [ -z "$BENCHMARK_LMBENCH2_SCRIPTS_PATH" ]; then
This conditional is OK, but I'd like to add some more autodetection here, when
BENCHMARK_LMBENCH2_SCRIPTS_PATH is not set.

> +      SCRIPTS_PATH="$BOARD_TESTDIR/fuego.$TESTDIR/scripts"
Can we add something here like this:

            if cmd "test ! -d $SCRIPTS_PATH" ; then
                   SCRIPTS_PATH=/usr/lib/lmbench/scripts
                    if cmd "test ! -d $SCRIPTS_PATH" ; then
                           abort_job "Could not find lmbench scripts directory. Maybe specify SCRIPTS_PATH dynamic variable?"
                    fi
            fi

> +   else
> +      SCRIPTS_PATH="$BENCHMARK_LMBENCH2_SCRIPTS_PATH"
> +   fi
> +
>     # some trickery to get the directory right
>     if [ -n "$PREFIX" ] ; then
>        LMBENCH_OS=`ls ./bin`
You should probably use this instead:
          LMBENCH_OS=$(cmd ls $SCRIPTS_PATH/bin)
The previous command is run in the build directory, but I'm not sure that the
build directory will be correctly populated in your use case.

Just as a side note, I prefer the $() command syntax, over using
backticks, to set shell variables from command output.

>     else
> -      LMBENCH_OS=`ls ./bin`
> -      echo "lmbench_os: $LMBENCH_OS"
> -      echo "curr path: $(pwd)"
> -      LMBENCH_OS=$(scripts/os)
> -      echo "lmbench_os: $LMBENCH_OS"
> +      get_program_path os $SCRIPTS_PATH $BOARD_TESTDIR/fuego.$TESTDIR/scripts/os
> +      LMBENCH_OS=$(safe_cmd "$PROGRAM_OS")
This can be 'cmd' instead of 'safe_cmd'.

safe_cmd should only be used if there's a significant chance of the command exhausting
memory on the target board.  You were perhaps misled by the other uses of 'safe_cmd'
here, some of which should just be 'cmd'.

>     fi
> -   safe_cmd "rm -rf $BOARD_TESTDIR/fuego.$TESTDIR/results"
> -   safe_cmd "cd $BOARD_TESTDIR/fuego.$TESTDIR/scripts; OS=$LMBENCH_OS ./config-run"
> -   safe_cmd "cd $BOARD_TESTDIR/fuego.$TESTDIR/scripts; OS=$LMBENCH_OS ./results"
> -   report "cd $BOARD_TESTDIR/fuego.$TESTDIR/scripts; ./getsummary ../results/$LMBENCH_OS/*.0"
> +   safe_cmd "rm -rf $SCRIPTS_PATH/../results"
This should just be 'cmd'.  (This incorrect usage was there before, but can you fix it in 
a second revision of your patch?)

Also, this assumes the 'results' directory is relative to SCRIPTS_PATH, which
is not true for my Ubuntu (/Debian) package for lmbench.
Please consider using $RESULTS_DIR (see below)

> +   safe_cmd "cd $SCRIPTS_PATH; OS=$LMBENCH_OS ./config-run"
> +   safe_cmd "cd $SCRIPTS_PATH; OS=$LMBENCH_OS ./results"
> +   report "cd $SCRIPTS_PATH; ./getsummary ../results/$LMBENCH_OS/*.0"

This structure assumes that the 'results' directory is underneath the SCRIPTS_PATH.
Can you confirm that this is indeed the case for your test system?

I'm thinking about possibly autodetecting the 'results' directory also, if it's
not found in the fuego  test directory.  This is just to handle a case like the
Ubuntu package, where it puts results under /var/ instead of /usr.

Maybe using something like this:
RESULTS_DIR="$SCRIPTS_PATH/../results"
if cmd "test ! -d $RESULTS_DIR" ; then
    RESULTS_RESULTS=/var/lib/lmbench/results
     if cmd "test ! -d $RESULTS_DIR" ; then
             abort_job "Could not find lmbench results directory. Maybe specify SCRIPTS_PATH dynamic variable?"
     fi
fi

>  }
> 
>  function test_cleanup {
> --
> 2.20.1
> 

I haven't tested any of my suggestions.  Can you please respond to my above questions, and submit
another version of this patch based on my suggestions (or let me know if you have alternative
suggestions or feedback?)

Thanks,
 -- Tim