Re: [Fuego] [PATCH 4/4] rt: search for the binary if the build phase is skipped

[<Tim.Bird@sony.com>]

> -----Original Message-----
> From: daniel.sangorrin@toshiba.co.jp <daniel.sangorrin@toshiba.co.jp>
> 
> Hi Tim,
> 
> Thanks again for your review.
> We will fix the patch and re-send.
> 
> See my comments inline:
> 
> > -----Original Message-----
> > From: Tim.Bird@sony.com <Tim.Bird@sony.com>
> > Sent: Saturday, August 21, 2021 5:16 AM
> > To: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ) <daniel.sangorrin@toshiba.co.jp>
> > Cc: fuego@lists.linuxfoundation.org; nguyen dat tho(ＴＳＤＶ Ｅｎｇ　１) <tho1.nguyendat@toshiba.co.jp>
> > Subject: RE: [PATCH 4/4] rt: search for the binary if the build phase is skipped
> >
> > > -----Original Message-----
> > > From: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > >
> > > From: Nguyen Dat Tho <tho1.nguyendat@toshiba.co.jp>
> > >
> > > ftc has the ability to skip the build phase to use a previously built
> > > binary or a binary installed on the board's file system (e.g. apt-get
> > > install rt-tests).
> > >
> > > Signed-off-by: Nguyen Dat Tho <tho1.nguyendat@toshiba.co.jp>
> > > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > > ---
> > >  tests/Benchmark.cyclictest/fuego_test.sh  | 7 ++++++-
> > >  tests/Benchmark.hackbench/fuego_test.sh   | 7 ++++++-
> > >  tests/Benchmark.migratetest/fuego_test.sh | 7 ++++++-
> > >  tests/Benchmark.pmqtest/fuego_test.sh     | 8 +++++++-
> > >  tests/Benchmark.ptsematest/fuego_test.sh  | 8 +++++++-
> > > tests/Benchmark.signaltest/fuego_test.sh  | 7 ++++++-
> > > tests/Benchmark.sigwaittest/fuego_test.sh | 7 ++++++-
> > > tests/Benchmark.svsematest/fuego_test.sh  | 7 ++++++-
> > >  tests/Functional.pi_tests/fuego_test.sh   | 7 ++++++-
> > >  9 files changed, 56 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/tests/Benchmark.cyclictest/fuego_test.sh
> > > b/tests/Benchmark.cyclictest/fuego_test.sh
> > > index 74d9d24..e7070dd 100755
> > > --- a/tests/Benchmark.cyclictest/fuego_test.sh
> > > +++ b/tests/Benchmark.cyclictest/fuego_test.sh
> > > @@ -24,5 +24,10 @@ function test_deploy {  }
> > >
> > >  function test_run {
> > > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./cyclictest $BENCHMARK_CYCLICTEST_PARAMS"
> > > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/cyclictest ]; then
> > This test ([ -f) won't work on a remote board.  This is running on the host machine, which has a different filesystem than the device
> > under test, unless you are running using the 'local' TRANSPORT.
> >
> > There should be a cmd() in here somewhere to perform this test on the board's filesystem.
> 
> Yes, you are totally right. Sorry for not catching that problem during my review.
> 
> > I'm not sure I follow this.  What does the test_deploy() function look like?
> > Under what circumstances would cyclictest not be present in the boards $BOARD_TESTDIR/fuego.$TESTDIR directory?
> 
> We want to run fuego natively because it is easier to run it on certain scenarios such as LAVA board farms and custom cloud images.
> We also want to use packaged versions of the tests and do not store any test tarball in our repositories. So the OS image will already have
> the tests there and therefore we will skip the build/deploy phases.
> 
> > It seems like this code should be coupled with code that detects if the program is already present, and if so 1) avoids deploying it and
> 
> We are skipping the build and deploy phase on purpose from ftc.
> 
> > 2) then uses it for the test.
> >
> > Something like this:
> > test_deploy {
> >   is_on_target_path cyclictest PROGRAM_CYCLICTEST
> >   if [ -z "$PROGRAM_CYCLICTEST" ] ; then
> >      put cyclictest $BOARD_TESTDIR/fuego.$TESTDIR
> >      PROGRAM_CYCLICTEST=$BOARD_TESTDIR/fuego.$TESTDIR/cyclictest
> >  fi
> >   export PROGRAM_CYCLICTEST
> > }
> >
> > and then in test_run:
> >   if [ -n "$PROGRAM_CYCLICTEST" ] ; then
> >     report "$PROGRAM_CYCLICTEST $BENCHMARK_CYCLICTEST_PARAMS"
> >   else
> >     abort_job "cyclictest is not found on the target"
> >   fi
> >
> > Maybe I'm not understanding the use case here.  Is this for running a pre-existing program on the board, or a program that was already
> > placed in the BOARD_TESTDIR directory (e.g. from a previous run of the test)?
> 
> A pre-existing program on the board (e.g. previously installed).
> 
> > If we have this pattern a lot, then maybe it would make sense to make the optional deploy code  a core function, like:
> >    put_if_program_not_present cyclictest
> 
> We are skipping the build/deploy phase using ftc. What do you think?

OK, based on that, I think I'd like to structure this as follows:

I'd like to add a new core function called 'get_program_path'

The intent is that code in a test_run function can use that to find the correct
path to execute the test program (possibly based on per-lab policy).

The sequence in test_run function would look like this:

  get_program_path cyclictest
  report "$PROGRAM_CYCLICTEST $ARGS"

There would be no conditionals in the code for individual tests - just this
additional call to get_program_path.

The 'get_program_path' function would have the following functionality:
(in pseudo-code, with 'cyclictest' used as a placeholder name for an 
arbitrary program name):

if is_on_target_path cyclictest ; then
   PROGRAM_CYCLICTEST=(the target path found)
elsif cmd "test -f $BOARD_TESTDIR/fuego.$TESTDIR/cyclictest" ; then
   PROGRAM_CYCLICTEST=$BOARD_TESTDIR/fuego.$TESTDIR/cyclictest
else
   abortjob "cyclictest was not found"

If the function succeeds, then it sets the value of PROGRAM_CYCLICTEST to
the path on the board to be used to execute cyclictest.

This should handle the following cases:
 - cyclictest is on the board (on the PATH) from external pre-installation (outside of Fuego)
 - cyclictest is on the board in the test directory (from Fuego's deploy, or from
  a previous test invocation that did not remove the test directory)

The function get_program_path could be extended in the future to implement
other program search policies.  The policy above is "use test program already on
the board, if found, then the one installed by Fuego."  Other policies might be
"use the test program installed by Fuego, then one already on the board, if present", or
"let the report() function figure out if the program is present or not in the test
directory" (this is Fuego's current policy).  But extending this to support other
policies (and adding the test program search policy to the fuego.conf file) will
be left for a future exercise.

Let me know what you think.  I had hoped to prototype something today,
but ran out of time working on other issues.  I'll see what I can whip up,
Please let me know if you see any problems with this approach.  I think it
covers your use case.  You will need to have a separate mechanism for
skipping the build and deploy steps for your use case (but Fuego supports
this with the test phase control options of ftc).
 -- Tim