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

Hi Tim,

Thank you for clarifying me, please see my inline comments

>-----Original Message-----
>From: [email protected] <[email protected]>
>Sent: 08 September 2021 06:44
>To: pyla venkata(ＴＳＩＰ) <[email protected]>
>Cc: [email protected]; nguyen dat tho(ＴＳＤＶ Ｅｎｇ　１)
><[email protected]>
>Subject: RE: [PATCH 4/4] rt: search for the binary if the build phase is skipped
>
>> -----Original Message-----
>> From: [email protected] <[email protected]>
>>
>> Hi Tim,
>>
>> I have few questions on the below implementation for 'get_program_path'
>>
>> It looks like the function 'get_program_path' always first checks the program
>is installed in 'PATH', then it checks in path $2 and then $3.
>
>Path 3 is never checked, and it is not a list of directories to check.  It is the return
>value, if the program is not found in the other 2 paths, and if it is not specified,
>then it defaults to:
>$BOARD_TESTDIR/fuego.$TESTDIR/{program_name}
>
>I did this to most closely match the current behavior of Fuego.
>In the discussion that follows, I'm using the following shorthand:
> a)  board test dir = $BOARD_TESTDIR/fuego.$TESTDIR
> b) PATH = one of the directories in the target boards PATH variable
> c) alternate dirs. = one of the directories specified as an argument to
>get_program_path (2nd argument)
>
>Fuego has several kinds of invocations of programs on the board in fuego_test.sh
>report functions currently:
> - from the board test directory, using a relative path (e.g using 'cd board test dir
>; ./program_name')
>    - these are always for programs that Fuego has deployed, this is by far the
>most common case
> - from the board test directory, using a full path
>    - these are also always for programs that Fuego has deployed, this is very rare
> - from the PATH, using just the program name
>    - these are always for programs that are already assumed to be on the target
>(xrandr, bc, ls, time, dd, cat, java, etc.)
>       - this is also a fairly common case.  In this case, Fuego never intended to
>install this program.
>        - if the program is possibly not present, then often there is an
>assert_has_program in the test_pre_check to
>          make sure it is present before proceeding.
> - from other places
>    - LTP can be run from a pre-installed location now, but it requires manually
>configuring the spec with
>      a HOMEDIR argument (or the board with a FUNCTIONAL_LTP_HOMEDIR
>argument)
>
>> In this situation user cannot skip the installed programs in the PATH and
>instead use specified path in $2 or $3?
>That's correct.
>
>> It is not our use case anyway to skip the programs installed in PATH,
>> rather we want to use the installed programs in the PATH variable, But
>> I worried if I use 'get_program_path' in the fuego_test.sh, it will
>> always checks the program present in PATH first and then specified in
>> $2 or $3,
>> this will break for the users who want to use program that is compiled
>> and deployed stages of fuego and also when the program is installed in the
>PATH.
>
>That's a good observation.  Given that the current most-common behavior is to
>run the test program from the board test directory, that should be the default
>behavior of get_program_path, in order to change the behavior for existing tests
>as little as possible.
>
>Under normal circumstances, if the test deploys a program that was built by
>Fuego, we want to run that, since it is most likely that the arguments and output
>will be what the Fuego test is expecting and can work with.  If a different version
>of the test program is on the board, then it might cause unexpected problems
>running it and parsing the output.
>
>That is, if Fuego has deployed 'dhrystone' (for example) to the board test
>directory, we probably want to run that instead of one found on the board PATH.
>
>> e.g:
>>  function test_run {
>> -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./dhrystone
>$BENCHMARK_DHRYSTONE_LOOPS"
>> +    get_program_path 'dhrystone'
>> +    report "$PROGRAM_DHRYSTONE $BENCHMARK_DHRYSTONE_LOOPS"
>>  }
>
>As get_program_path is currently written, you are right, this gives preference to
>dhrystone on the PATH instead of the one in the board test dir.
>
>>
>> Instead I am thinking something like this, the function
>> 'get_program_path' should first check the path specified in $2 (where
>> the fuego build and deploy stage are performed) and, if not found then check
>in path $3 (where fuego build and deploy stages are skipped), this can be made
>default behaviour inside 'get_program_path' function and user can change if he
>want by passing different paths.
>I don't follow this.  Sorry.
>
>> function test_run {
>> -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./dhrystone
>$BENCHMARK_DHRYSTONE_LOOPS"
>> +    get_program_path 'dhrystone'  "$BOARD_TESTDIR/fuego.$TESTDIR" "/bin"
>I'm not sure those arguments match what you are saying.  I'm also not sure
>where you think the PATH check should occur, in the sequence of checks (if
>anywhere).
>
>> +    report "$PROGRAM_DHRYSTONE $BENCHMARK_DHRYSTONE_LOOPS"
>
>>  }
>>
>> Also the 'get_program_path' function can be called inside the report
>> function with default behaviour, and then path for $3 variable can be modified
>by taking from test spec variable.
>> With this one can change the test behaviour to use the test program from
>specified path in the test spec itself.
>I like the idea of allowing the spec to indicate an alternate path.
>
>>
>> Please let me know if this make sense, or clarify me if I misunderstood it
>entirely.
>You raise a good issue, but I'm not sure I understood exactly the use case you
>have.
>
>When you install the test program (eg. dhrystone) into the system, separately
>from the Fuego deploy step, are you planning on putting it in a regular system
>directory such as /usr/bin, or are you planning on putting it in the board test dir?

Our intention is to use the pre-installed binaries in the board while we skip the build and deploy phase of fuego.

>
>Also, if the base distribution on the board already has the program, do you want
>the Fuego test to run the program you installed, or the one from the base
>distribution?

If Build and deploy phase is used then it is better to use it from Fuego test directory, else search for the path order you mentioned below case #1, #2

>
>In any event, let me describe some use cases and some search order
>precedences, and see if they cover the cases we're interested in - then propose a
>different implementation of get_program_path.
>
>Use case #1 - regular Fuego build, deploy and run
>  - fuego phasese run: build, deploy, run (and others)
> - preferred search preference:
>    - board test dir
>    - alternate dir(s) (if specified)
>    - PATH dir(s)
>
> Use case #2 - Fuego run, without build and deploy
>   - fuego phases run: only 'run'
>   - preferred search preference:
>      - board test dir
>      - alternate dir(s) (if specified)
>      - PATH dir(s)
>
> Use case #3 - Fuego test that anticipates that the distro may already have the
>test program installed
>    - fuego phases run: only 'run'
>    - preferred search preference
>       - from alternate dir
>       - from PATH
>       - from board test dir
>
>Actually, in case 3, since there is no deploy step, there is no harm to keep the
>same search order as cases #1 and #2 (board test dir first), as the program
>should never be found in the board test dir.

Yes, I think the case #3 is not needed.

>
>Please let me know if this is correct.
>
>So, I'm inclined to re-write get_program_path to switch the search order to:
> - board test dir
> - alternate dir
> - PATH
>

I think this works for us, as we skip the build and deploy phase, so it should use program from the PATH.

>I find the $BOARD_TESTDIR/fuego.$TESTDIR to be annoying, and I had hoped to
>avoid using it as an argument.  If it's always the first place searched, then it
>doesn't need to be specified in the arguments.  If the reference to it could be
>removed from fuego_test.sh scripts, then it would give the Fuego system the
>flexibility to change it or use other shell variables in the future (which could be
>useful.)
>
>get_program_path would then have the following arguments:
>  $1 - program to search for
>  $2 - alternate dirs. to search (optional, and takes precedence over the PATH)
>
>This does not support the case where you want to override running the program
>from the board test dir (using an alternate location).  However, that case can be
>handled by not using get_program_path, and doing the check directly using
>something like this:
>if cmd "test -x $FUNCTIONAL_XXX_PROGRAM_FOO_PATH" ; then
>    report "$FUCTIONAL_XXX_PROGRAM_FOO_PATH ..."
>else
>   ... fall back to some other search path, or report failure ...
>fi
>
>What do you think?

I think changing the order of the search path in 'get_program_path' should work for us.

> -- Tim
>
>>
>> Thanks,
>> Venkata.
>>
>>
>> >-----Original Message-----
>> >From: Fuego <[email protected]> On Behalf Of
>> >[email protected]
>> >Sent: 02 September 2021 00:40
>> >To: nguyen dat tho(ＴＳＤＶ Ｅｎｇ　１) <[email protected]>
>> >Cc: [email protected]
>> >Subject: Re: [Fuego] [PATCH 4/4] rt: search for the binary if the
>> >build phase is skipped
>> >
>> >OK - I have completed work on a new function to support the feature you
>desire.
>> >
>> >It is called 'get_program_path', and it is documented here:
>> >http://fuegotest.org/wiki/function_get_program_path
>> >
>> >Please check out this function, and see if it does what you would like.
>> >I have also created a test (Functional.fuego_function_gpp_check) that
>> >tests get_program_path, to verify that it works correctly.
>> >Please try this test in your environment and let me know if you
>> >encounter any problems.
>> >
>> >Also, please note that I have added additional functionality to the
>> >report and report_append functions, to cd to the test directory on
>> >the board
>> >($BOARD_TESTDIR/fuego.$TESTDIR) automatically, as part of every call
>> >to those functions.
>> >
>> >This means that we can remove that 'cd' operation from a lot of
>> >report() and report_append() calls.
>> >
>> >I assume that because the PROGRAM_xxx variable will have a full path,
>> >the cd is no longer required in tests that use this as well.  I
>> >thought it safer, since you might be modifying the report() lines for
>> >multiple tests, to set the default working directory.  If needed, you
>> >can now remove the "cd $BOARD_TESTDIR/fuego.$TESTDIR" command from
>any tests you modify.
>> >
>> >This solves the problem that your patch originally addressed, but I
>> >have a nagging feeling that the higher level concept of installing
>> >pre-built packages as part of board provisioning might be addressed in
>another way.
>> >
>> >Are you familiar with the 'binary packages' feature of Fuego?  There
>> >is a tool to pre-build binary packages for a board, called
>> >fuego-core/scripts/make_cache.sh This is used to create a set of
>> >binary packages (also called 'target packages') that can be installed separate
>from Fuego test execution.
>> >
>> >This is a feature that was under development for some time, and then
>> >got put on the backburner.  Please see the page:
>> >http://fuegotest.org/wiki/Target_Packages.
>> >
>> >These packages are intended to replace the 'build' and 'deploy'
>> >phases of a test, when they are used.  The package contents are
>> >(usually) installed relative to $BOARD_TESTDIR/fuego.$TESTDIR, but it
>> >is possible to install them somewhere else (e.g. globally under say
>> >/opt/test/fuego)
>> >
>> >One idea for this, was to have pre-built binary packages for tests on
>> >a central server, so that someone could run Fuego and its tests
>> >without having to install a toolchain for a board at all.
>> >
>> >It sounds like you are doing something similar for integration with
>> >LAVA and/or kernelci.
>> >(using pre-installed test programs, and skipping the 'build' and
>> >'deploy' steps.)
>> >
>> >I'd like to get more details about what you are doing, to possibly
>> >consolidate these features and avoid fragmenting the code.
>> > -- Tim
>> >
>> >> -----Original Message-----
>> >> From: [email protected] <[email protected]>
>> >>
>> >> Dear Tim,
>> >>
>> >> > 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).
>> >> It sounds good.
>> >> When you finished, please let me know. I will update this patch
>> >> follow your
>> >idea.
>> >>
>> >> Thanks,
>> >> Tho
>> >>
>> >> -----Original Message-----
>> >> From: [email protected] <[email protected]>
>> >> Sent: Friday, August 27, 2021 12:28 PM
>> >> To: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ)
>> >> <[email protected]>
>> >> Cc: [email protected]; nguyen dat tho(ＴＳＤＶ Ｅｎｇ　１)
>> >> <[email protected]>
>> >> Subject: RE: [PATCH 4/4] rt: search for the binary if the build
>> >> phase is skipped
>> >>
>> >>
>> >>
>> >> > -----Original Message-----
>> >> > From: [email protected]
>> >> > <[email protected]>
>> >> >
>> >> > Hi Tim,
>> >> >
>> >> > Thanks again for your review.
>> >> > We will fix the patch and re-send.
>> >> >
>> >> > See my comments inline:
>> >> >
>> >> > > -----Original Message-----
>> >> > > From: [email protected] <[email protected]>
>> >> > > Sent: Saturday, August 21, 2021 5:16 AM
>> >> > > To: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ)
>> >> > > <[email protected]>
>> >> > > Cc: [email protected]; nguyen dat tho(ＴＳＤＶ Ｅｎｇ
>１
>> >)
>> >> > > <[email protected]>
>> >> > > Subject: RE: [PATCH 4/4] rt: search for the binary if the build
>> >> > > phase is skipped
>> >> > >
>> >> > > > -----Original Message-----
>> >> > > > From: Daniel Sangorrin <[email protected]>
>> >> > > >
>> >> > > > From: Nguyen Dat Tho <[email protected]>
>> >> > > >
>> >> > > > 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 <[email protected]>
>> >> > > > Signed-off-by: Daniel Sangorrin
>> >> > > > <[email protected]>
>> >> > > > ---
>> >> > > >  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
>> >
>> >_______________________________________________
>> >Fuego mailing list
>> >[email protected]
>> >https://lists.linuxfoundation.org/mailman/listinfo/fuego