[Fuego] [PATCH 1/3] of: use logdir argument instead of prolog.sh

Tim.Bird at sony.com Tim.Bird at sony.com
Fri Sep 7 18:25:21 UTC 2018



> -----Original Message-----
> From: Daniel Sangorrin
> 
> This patch does not change the behavior of the program, it
> still generates a prolog.sh in the current job's log
> directory. It only prepares for the ability to leave
> other files (such as a dynamically generated spec.json)
> in the log directory.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin at toshiba.co.jp>
> ---
>  engine/scripts/overlays.sh |  2 +-
>  engine/scripts/ovgen.py    | 10 +++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/engine/scripts/overlays.sh b/engine/scripts/overlays.sh
> index bbc5d16..9e3eb55 100644
> --- a/engine/scripts/overlays.sh
> +++ b/engine/scripts/overlays.sh
> @@ -73,7 +73,7 @@ function set_overlay_vars() {
> 
>      rm -f $LOGDIR/prolog.sh
> 
> -    run_python $OF_OVGEN $OF_DEBUG_ARGS --classdir $OF_CLASSDIRS --
> ovfiles $OF_DISTRIB_FILE $OF_BOARD_FILE $OF_BOARD_VAR_FILE --testdir
> $TESTDIR --testspec $TESTSPEC --output $LOGDIR/prolog.sh || abort_job
> "Error while prolog.sh file generation"
> +    run_python $OF_OVGEN $OF_DEBUG_ARGS --classdir $OF_CLASSDIRS --
> ovfiles $OF_DISTRIB_FILE $OF_BOARD_FILE $OF_BOARD_VAR_FILE --testdir
> $TESTDIR --testspec $TESTSPEC --logdir $LOGDIR || abort_job "Error while
> prolog.sh file generation"

There are a few issues that I'm thinking about here:
1) this means the name "prolog.sh" is now hardcoded into the overlay generator.
I don't know if that's a problem or not.  I can't think of any use cases
where a user of the overlay generator would want to use a different name.
But it was kind of nice that this wasn't part of the protocol between Fuego
core and the overlay generator.  Now, it is. Implicitly Fuego core expects
the overlay generator to produce a specific "magic" filename.  Probably,
the overlay generator is one of the elements of Fuego least likely to be
used by external systems, but still I hesitate to make it less general.
On the other hand, it simplifies the code to just use a well-known filename.

2) I'm not a fan of using the argument string "--logdir".  I think my preference
would be "--outdir", or even "--rundir".  This is for the same reason - to keep the overlay
generator generic.  The artifacts created by the overlay generator have nothing
to do with logging, and more to do with processing required during the run.
Well, maybe the generated spec is a form of log (collection of information about the
run).

These are minor nits.  It's OK for now.  (Perfect is the enemy of good enough!)

Maybe it's sufficient if there's a comment above this line, indicating that the overlay
generator will populate the logdir with prolog.sh and spec.json.
I think I'll just add that.

> 
>      if [ ! -f "$LOGDIR/prolog.sh" ]
>      then
> diff --git a/engine/scripts/ovgen.py b/engine/scripts/ovgen.py
> index 98cdf82..7c1f05f 100755
> --- a/engine/scripts/ovgen.py
> +++ b/engine/scripts/ovgen.py
> @@ -366,7 +366,8 @@ def parseOverrideFile(overrideFile, layer, ofcls):
> 
>      return classes
> 
> -def generateProlog(outFilePath, ofcls, classes, testdir, testspec):
> +def generateProlog(logdir, ofcls, classes, testdir, testspec):
> +    outFilePath = logdir + '/prolog.sh'
>      outfile = open(outFilePath, "w")
> 
>      for ofc in classes:
> @@ -402,7 +403,6 @@ def generateProlog(outFilePath, ofcls, classes, testdir,
> testspec):
>  # generateSpec - generate shell output for the spec
>  #     ts:TestSpecs  -  parsed specs for our testdir
>  #     testspec: String  -   name of test spec (e.g. 'default')
> -#     outFilePath - file output descriptor opened -> prolog.sh
>  def generateSpec(ts, fout):
>      debug_print("generating spec %s" % (ts.name))
> 
> @@ -470,7 +470,7 @@ def run(test_args=None):
>      parser.add_argument('--ovfiles', nargs='+', metavar='OVFILE', help='list of
> directories containing .override files', required=True)
>      parser.add_argument('--testdir', help='e.g.: Benchmark.Dhrystone',
> required=True)
>      parser.add_argument('--testspec', nargs='+', metavar='TESTSPEC',
> help='testspec', required=True)
> -    parser.add_argument('--output', help='output file name', required=True)
> +    parser.add_argument('--logdir', help='log dir', required=True)
>      parser.add_argument('--debug', help='{1,2,3} debug level (default is no
> debugging)', type=int, required=False)
> 
>      args = parser.parse_args(args=test_args)
> @@ -493,10 +493,10 @@ def run(test_args=None):
>          classes = classes + parseOverrideFile(ovf, layers, ofcls)
>          debug_print ("parsed %s override\n------------\n" % (ovf))
> 
> -    generateProlog(args.output, ofcls, classes, args.testdir, args.testspec[0])
> +    generateProlog(args.logdir, ofcls, classes, args.testdir, args.testspec[0])
> 
>  def testrun():
> -    test_args =  "--debug 2 --classdir /fuego-core/engine/overlays/base/ --
> ovfiles /fuego-ro/boards/qemu-arm.board --testdir Benchmark.Dhrystone --
> testspec default --output prolog.sh".split()
> +    test_args =  "--debug 2 --classdir /fuego-core/engine/overlays/base/ --
> ovfiles /fuego-ro/boards/qemu-arm.board --testdir Benchmark.Dhrystone --
> testspec default --output ./".split()
>      run(test_args)
> 
>  run()
> --
> 2.7.4

Ok - looks good. Applied.
 -- Tim



More information about the Fuego mailing list