[Fuego] [PATCH] lmbench: remove unnecessary debugging information added

[Fuego] [PATCH] lmbench: remove unnecessary debugging information added

[venkata.pyla]

From: venkata pyla <venkata.pyla@toshiba-tsip.com>

Signed-off-by: venkata pyla <venkata.pyla@toshiba-tsip.com>
---
 tests/Benchmark.lmbench2/fuego_test.sh | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tests/Benchmark.lmbench2/fuego_test.sh b/tests/Benchmark.lmbench2/fuego_test.sh
index d07606c..50dcff1 100755
--- a/tests/Benchmark.lmbench2/fuego_test.sh
+++ b/tests/Benchmark.lmbench2/fuego_test.sh
@@ -26,11 +26,7 @@ function test_run {
    if [ -n "$PREFIX" ] ; then
       LMBENCH_OS=`ls ./bin`
    else
-      LMBENCH_OS=`ls ./bin`
-      echo "lmbench_os: $LMBENCH_OS"
-      echo "curr path: $(pwd)"
       LMBENCH_OS=$(scripts/os)
-      echo "lmbench_os: $LMBENCH_OS"
    fi
    safe_cmd "rm -rf $BOARD_TESTDIR/fuego.$TESTDIR/results"
    safe_cmd "cd $BOARD_TESTDIR/fuego.$TESTDIR/scripts; OS=$LMBENCH_OS ./config-run"
-- 
2.20.1

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

[venkata.pyla]

From: venkata pyla <venkata.pyla@toshiba-tsip.com>

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'}"

Signed-off-by: venkata pyla <venkata.pyla@toshiba-tsip.com>
---
 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
+      SCRIPTS_PATH="$BOARD_TESTDIR/fuego.$TESTDIR/scripts"
+   else
+      SCRIPTS_PATH="$BENCHMARK_LMBENCH2_SCRIPTS_PATH"
+   fi
+
    # some trickery to get the directory right
    if [ -n "$PREFIX" ] ; then
       LMBENCH_OS=`ls ./bin`
    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")
    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"
+   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"
 }
 
 function test_cleanup {
-- 
2.20.1

Re: [Fuego] [PATCH] lmbench: remove unnecessary debugging information added

[Tim.Bird]

Looks good. applied.
 -- Tim


> -----Original Message-----
> From: venkata pyla <venkata.pyla@toshiba-tsip.com>
> 
> Signed-off-by: venkata pyla <venkata.pyla@toshiba-tsip.com>
> ---
>  tests/Benchmark.lmbench2/fuego_test.sh | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/tests/Benchmark.lmbench2/fuego_test.sh b/tests/Benchmark.lmbench2/fuego_test.sh
> index d07606c..50dcff1 100755
> --- a/tests/Benchmark.lmbench2/fuego_test.sh
> +++ b/tests/Benchmark.lmbench2/fuego_test.sh
> @@ -26,11 +26,7 @@ function test_run {
>     if [ -n "$PREFIX" ] ; then
>        LMBENCH_OS=`ls ./bin`
>     else
> -      LMBENCH_OS=`ls ./bin`
> -      echo "lmbench_os: $LMBENCH_OS"
> -      echo "curr path: $(pwd)"
>        LMBENCH_OS=$(scripts/os)
> -      echo "lmbench_os: $LMBENCH_OS"
>     fi
>     safe_cmd "rm -rf $BOARD_TESTDIR/fuego.$TESTDIR/results"
>     safe_cmd "cd $BOARD_TESTDIR/fuego.$TESTDIR/scripts; OS=$LMBENCH_OS ./config-run"
> --
> 2.20.1
> 

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

[Tim.Bird]

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

See comments inline below.

> -----Original Message-----
> From: venkata.pyla@toshiba-tsip.com <venkata.pyla@toshiba-tsip.com>
> 
> From: venkata pyla <venkata.pyla@toshiba-tsip.com>
> 
> 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 <venkata.pyla@toshiba-tsip.com>
> ---
>  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

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

[Venkata.Pyla]

Hi Tim,

Thank you for your comments, please find my inline comments.

>-----Original Message-----
>From: Tim.Bird@sony.com <Tim.Bird@sony.com>
>Sent: 05 November 2021 23:38
>To: pyla venkata(ＴＳＩＰ) <Venkata.Pyla@toshiba-tsip.com>
>Cc: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ)
><daniel.sangorrin@toshiba.co.jp>; fuego@lists.linuxfoundation.org; dinesh
>kumar(ＴＳＩＰ) <dinesh.kumar@toshiba-tsip.com>
>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: venkata.pyla@toshiba-tsip.com <venkata.pyla@toshiba-tsip.com>
>>
>> From: venkata pyla <venkata.pyla@toshiba-tsip.com>
>>
>> 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 <venkata.pyla@toshiba-tsip.com>
>> ---
>>  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

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

[Tim.Bird]

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

> -----Original Message-----
> From: Venkata.Pyla@toshiba-tsip.com <Venkata.Pyla@toshiba-tsip.com>
> Sent: Tuesday, November 9, 2021 10:53 AM
> To: Bird, Tim <Tim.Bird@sony.com>
> Cc: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org; dinesh.kumar@toshiba-tsip.com; dinesh.kumar@toshiba-tsip.com
> 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: Tim.Bird@sony.com <Tim.Bird@sony.com>
> >Sent: 05 November 2021 23:38
> >To: pyla venkata(ＴＳＩＰ) <Venkata.Pyla@toshiba-tsip.com>
> >Cc: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ)
> ><daniel.sangorrin@toshiba.co.jp>; fuego@lists.linuxfoundation.org; dinesh
> >kumar(ＴＳＩＰ) <dinesh.kumar@toshiba-tsip.com>
> >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: venkata.pyla@toshiba-tsip.com <venkata.pyla@toshiba-tsip.com>
> >>
> >> From: venkata pyla <venkata.pyla@toshiba-tsip.com>
> >>
> >> 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 <venkata.pyla@toshiba-tsip.com>
> >> ---
> >>  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

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

[venkata.pyla]

From: venkata pyla <venkata.pyla@toshiba-tsip.com>

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 <venkata.pyla@toshiba-tsip.com>
---
 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)
    else
-      LMBENCH_OS=$(scripts/os)
+      get_program_path os $SCRIPTS_DIR $BOARD_TESTDIR/fuego.$TESTDIR/scripts/os
+      LMBENCH_OS=$(cmd "$PROGRAM_OS")
    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

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

[Venkata.Pyla]

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: venkata.pyla@toshiba-tsip.com <venkata.pyla@toshiba-tsip.com>
>Sent: 10 November 2021 23:52
>To: tim.bird@sony.com
>Cc: pyla venkata(ＴＳＩＰ) <Venkata.Pyla@toshiba-tsip.com>; sangorrin
>daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ) <daniel.sangorrin@toshiba.co.jp>;
>fuego@lists.linuxfoundation.org; dinesh kumar(ＴＳＩＰ)
><dinesh.kumar@toshiba-tsip.com>; hayashi kazuhiro(林 和宏 □ＳＷＣ◯ＡＣＴ)
><kazuhiro3.hayashi@toshiba.co.jp>
>Subject: [PATCH v3] lmbench: Add variables to specify scripts and results
>directory
>
>From: venkata pyla <venkata.pyla@toshiba-tsip.com>
>
>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 <venkata.pyla@toshiba-tsip.com>
>---
> 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)
>    else
>-      LMBENCH_OS=$(scripts/os)
>+      get_program_path os $SCRIPTS_DIR
>$BOARD_TESTDIR/fuego.$TESTDIR/scripts/os
>+      LMBENCH_OS=$(cmd "$PROGRAM_OS")
>    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

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

[Bird, Tim]

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: Venkata.Pyla@toshiba-tsip.com <Venkata.Pyla@toshiba-tsip.com>
> 
> 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: venkata.pyla@toshiba-tsip.com <venkata.pyla@toshiba-tsip.com>
> >Sent: 10 November 2021 23:52
> >To: tim.bird@sony.com
> >Cc: pyla venkata(ＴＳＩＰ) <Venkata.Pyla@toshiba-tsip.com>; sangorrin
> >daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ) <daniel.sangorrin@toshiba.co.jp>;
> >fuego@lists.linuxfoundation.org; dinesh kumar(ＴＳＩＰ)
> ><dinesh.kumar@toshiba-tsip.com>; hayashi kazuhiro(林 和宏 □ＳＷＣ◯ＡＣＴ)
> ><kazuhiro3.hayashi@toshiba.co.jp>
> >Subject: [PATCH v3] lmbench: Add variables to specify scripts and results
> >directory
> >
> >From: venkata pyla <venkata.pyla@toshiba-tsip.com>
> >
> >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 <venkata.pyla@toshiba-tsip.com>
> >---
> > 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

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

[Venkata.Pyla]

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

>-----Original Message-----
>From: Bird, Tim <Tim.Bird@sony.com>
>Sent: 09 February 2022 04:00
>To: pyla venkata(ＴＳＩＰ) <Venkata.Pyla@toshiba-tsip.com>
>Cc: fuego@lists.linuxfoundation.org; dinesh kumar(ＴＳＩＰ)
><dinesh.kumar@toshiba-tsip.com>; hayashi kazuhiro(林 和宏 □ＳＷＣ◯ＡＣＴ)
><kazuhiro3.hayashi@toshiba.co.jp>
>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: Venkata.Pyla@toshiba-tsip.com <Venkata.Pyla@toshiba-tsip.com>
>>
>> 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: venkata.pyla@toshiba-tsip.com <venkata.pyla@toshiba-tsip.com>
>> >Sent: 10 November 2021 23:52
>> >To: tim.bird@sony.com
>> >Cc: pyla venkata(ＴＳＩＰ) <Venkata.Pyla@toshiba-tsip.com>; sangorrin
>> >daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ)
><daniel.sangorrin@toshiba.co.jp>;
>> >fuego@lists.linuxfoundation.org; dinesh kumar(ＴＳＩＰ)
>> ><dinesh.kumar@toshiba-tsip.com>; hayashi kazuhiro(林 和宏 □ＳＷＣ◯ＡＣ
>Ｔ)
>> ><kazuhiro3.hayashi@toshiba.co.jp>
>> >Subject: [PATCH v3] lmbench: Add variables to specify scripts and
>> >results directory
>> >
>> >From: venkata pyla <venkata.pyla@toshiba-tsip.com>
>> >
>> >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 <venkata.pyla@toshiba-tsip.com>
>> >---
>> > 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
>