Re: [Fuego] [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