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

Thanks.  I look forward to seeing the updated patch.
 -- Tim

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Tuesday, November 9, 2021 10:53 AM
> To: Bird, Tim <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; [email protected]
> Subject: RE: [PATCH v2] lmbench: Use 'SCRIPTS_PATH' to specify distribution installed binaries path
> 
> Hi Tim,
> 
> Thank you for your comments, please find my inline comments.
> 
> >-----Original Message-----
> >From: [email protected] <[email protected]>
> >Sent: 05 November 2021 23:38
> >To: pyla venkata(ＴＳＩＰ) <[email protected]>
> >Cc: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ)
> ><[email protected]>; [email protected]; dinesh
> >kumar(ＴＳＩＰ) <[email protected]>
> >Subject: RE: [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.
> 
> I think adding default location is also good idea, as many of the distributions I have checked has the scripts located at '/usr/lib/lmbench'
> 
> >
> >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.
> 
> We use debian based distribution and in which the scripts, bin, config, directories are present under /usr/lib/lmbench/,
> and results folder is under /usr/share/lmbench
> 
> >
> >
> >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)
> >
> 
> Got it, thank you for letting me know.
> 
> >> 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.
> >
> 
> Thank you for the suggestion, I will correct this behaviour.
> 
> >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?)
> 
> Sure I will change this in my next 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?
> 
> For the results directory I was using the same way how the source code compiled binaries were using, with reference to SCRIPTS_PATH.
> If I introduce another variable RESILTS_DIR, then I think I can use the results directory from the distribution specified path only when the
> distribution binaries are used, else use the path with reference to SCRIPTS_PATH.
> 
> >
> >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
> >
> 
> I will verify this change and submit in my next version patch
> 
> >>  }
> >>
> >>  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?)
> 
> Sure, thank you for the review comments, I will update my patch and resend.
> 
> >
> >Thanks,
> > -- Tim