Re: [Fuego] [PATCH v3] lmbench: Add variables to specify scripts and results directory

Venkata,

Thanks for sending this again.  When I went back and looked through the mail list
archives, I saw this patch.  I thought I had applied it, but in reviewing the git logs
and source yesterday, I didn’t see it.  I'm sorry about this, but it looks like this
one fell through the cracks also.

I did some review, and it mostly looks OK, with one issue I'll point out inline below.


> -----Original Message-----
> From: [email protected] <[email protected]>
> 
> Hi Tim,
> 
> Could you please review this change as well, I think you might have missed this mail.
> 
> Please feel free to ask me if you need more details on this patch, as it been long time we discussed it.
> 
> Thanks,
> Venkata.
> >-----Original Message-----
> >From: [email protected] <[email protected]>
> >Sent: 10 November 2021 23:52
> >To: [email protected]
> >Cc: pyla venkata(ＴＳＩＰ) <[email protected]>; sangorrin
> >daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ) <[email protected]>;
> >[email protected]; dinesh kumar(ＴＳＩＰ)
> ><[email protected]>; hayashi kazuhiro(林 和宏 □ＳＷＣ◯ＡＣＴ)
> ><[email protected]>
> >Subject: [PATCH v3] lmbench: Add variables to specify scripts and results
> >directory
> >
> >From: venkata pyla <[email protected]>
> >
> >lmbench by default uses board test directory, to look over binaries and other
> >files when compiled locally.
> >
> >if the files are not present in board test directory, then it checks in generally
> >installed location by distributions  SCRIPTS_DIR=/usr/lib/lmbench/scripts
> > RESULTS_DIR=/var/lib/lmbench/results
> >
> >If still want to specifiy different location for the lmbench files, one can use the
> >following dynamic variables
> >
> >$ ftc run-test -b local -t lmbench2 -p ptrca \
> >      --dynamic-vars SCRIPTS_DIR=/usr/lib/lmbench/scripts \
> >      --dynamic-vars RESULTS_DIR=/usr/share/lmbench/results
> >
> >Signed-off-by: venkata pyla <[email protected]>
> >---
> > tests/Benchmark.lmbench2/fuego_test.sh | 33 +++++++++++++++++++++-----
> > 1 file changed, 27 insertions(+), 6 deletions(-)
> >
> >diff --git a/tests/Benchmark.lmbench2/fuego_test.sh
> >b/tests/Benchmark.lmbench2/fuego_test.sh
> >index e21fe0a..e012a80 100755
> >--- a/tests/Benchmark.lmbench2/fuego_test.sh
> >+++ b/tests/Benchmark.lmbench2/fuego_test.sh
> >@@ -23,16 +23,37 @@ function test_deploy {  }
> >
> > function test_run {
> >+   # Get the scripts and results directory paths of lmbench
> >+   if [ -z "$BENCHMARK_LMBENCH2_SCRIPTS_DIR" ]; then
> >+      SCRIPTS_DIR="$BOARD_TESTDIR/fuego.$TESTDIR/scripts"
> >+      RESULTS_DIR="$BOARD_TESTDIR/fuego.$TESTDIR/results"
> >+      if cmd "test ! -d $SCRIPTS_DIR" ; then
> >+         SCRIPTS_DIR=/usr/lib/lmbench/scripts
> >+         RESULTS_DIR=/var/lib/lmbench/results
> >+         if cmd "test ! -d $SCRIPTS_DIR" ; then
> >+            abort_job "Could not find lmbench scripts directory. Maybe specify
> >SCRIPTS_DIR dynamic variable?"
> >+         fi
> >+      fi
> >+   else
> >+      SCRIPTS_DIR="$BENCHMARK_LMBENCH2_SCRIPTS_DIR"
> >+      if [ -z "$BENCHMARK_LMBENCH2_RESULTS_DIR" ]; then
> >+         RESULTS_DIR="$BOARD_TESTDIR/fuego.$TESTDIR/results"
> >+      else
> >+         RESULTS_DIR="$BENCHMARK_LMBENCH2_SCRIPTS_DIR"
> >+      fi
> >+   fi
> >+
> >    # some trickery to get the directory right
> >    if [ -n "$PREFIX" ] ; then
> >-      LMBENCH_OS=`ls ./bin`
> >+      LMBENCH_OS=$(ls $SCRIPTS_DIR/../bin)
This was originally running in the package build directory, so the use of a local operation
was OK.  However, SCRIPTS_DIR is a reference to a directory on the device under test,
so this needs to be $(cmd ls $SCRIPTS_DIR/../bin).

However, having said that, in the case where we are using a pre-installed version of lmbench, I
think it's a bit weird to be using information from Fuego that is derived by looking at Fuego's PREFIX variable,
and from the cross-build directory name under bin (from the Fuego build directory on the host).  This could have little or nothing to
do with the strings needed by the pre-installed lmbench.  (I mean, they probably match, but they
might not.)

The old code made more sense.  Since we were always running the version of lmbench that we
created, it made sense to use items from the build directory for that version, as part of
its invocation.

I tried removing the 'if [ -n $PREFIX ] ; ' conditional, but it broke the code.

I changed the name of LMBENCH_OS to LMBENCH_OS_STR, because the old
name was confusing.  It could either mean the path to the 'os' program, or the
string produced by the program.  I think using the _STR suffix makes the code
easier to read.

> >    else
> >-      LMBENCH_OS=$(scripts/os)
> >+      get_program_path os $SCRIPTS_DIR
> >$BOARD_TESTDIR/fuego.$TESTDIR/scripts/os
> >+      LMBENCH_OS=$(cmd "$PROGRAM_OS")
The second argument for get_program_path should use colons as separators
if more than one directory is specified.  See 
http://fuegotest.org/wiki/function_get_program_path

> >    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"
> >+   cmd "rm -rf $RESULTS_DIR/*"
> >+   cmd "cd $SCRIPTS_DIR; OS=$LMBENCH_OS BINDIR=$SCRIPTS_DIR/.. ./config-
> >run"
> >+   cmd "cd $SCRIPTS_DIR; OS=$LMBENCH_OS RESULTSDIR=$RESULTS_DIR
> >./results"
> >+   report "cd $SCRIPTS_DIR; ./getsummary $RESULTS_DIR/$LMBENCH_OS/*.0"
> > }
> >
> > function test_cleanup {
> >--
> >2.20.1
> 

In any event, I applied your patch, and then did a fixup patch on top to address a few of
the issues above.

Please check it out and let me know if you have any problems with the updated code.
I'm still testing, but it seems to work OK for the boards in my lab.

 -- Tim