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

Thanks Tim, for fixing the problems, I will pull the latest changes and verify them.

>-----Original Message-----
>From: Bird, Tim <[email protected]>
>Sent: 09 February 2022 04:00
>To: pyla venkata(ＴＳＩＰ) <[email protected]>
>Cc: [email protected]; dinesh kumar(ＴＳＩＰ)
><[email protected]>; hayashi kazuhiro(林 和宏 □ＳＷＣ◯ＡＣＴ)
><[email protected]>
>Subject: RE: [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
>