[Fuego] [PATCH 28/30] logdir: a few changes

Daniel Sangorrin daniel.sangorrin at toshiba.co.jp
Thu Jun 7 02:17:12 UTC 2018


> -----Original Message-----
> From: Tim.Bird at sony.com [mailto:Tim.Bird at sony.com]
> Sent: Thursday, June 7, 2018 8:29 AM
> To: daniel.sangorrin at toshiba.co.jp; fuego at lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 28/30] logdir: a few changes
> 
> 
> 
> > -----Original Message-----
> > From: Daniel Sangorrin
> >
> > logdir is now created by ftc run-test. Also we use logdir
> > to put the spec so fix that in ovgen.
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin at toshiba.co.jp>
> > ---
> >  engine/scripts/overlays.sh | 17 +++++++----------
> >  engine/scripts/ovgen.py    | 23 +++++++++++------------
> >  2 files changed, 18 insertions(+), 22 deletions(-)
> >
> > diff --git a/engine/scripts/overlays.sh b/engine/scripts/overlays.sh
> > index 186bd8a..7a66910 100644
> > --- a/engine/scripts/overlays.sh
> > +++ b/engine/scripts/overlays.sh
> > @@ -56,7 +56,7 @@ function set_overlay_vars() {
> >      d_value=${d_valtemp%'"'}
> >      DISTRIB="${d_value}"
> >      set -e
> > -
> > +
> >      if [ ! "$DISTRIB" ]; then
> >          # FIXTHIS: automatically discover the best option
> >          DISTRIB="nosyslogd.dist"
> > @@ -67,17 +67,14 @@ function set_overlay_vars() {
> >      fi
> >      echo "Using $DISTRIB overlay"
> >
> > -    # Create the log directory for this test run here so we can place the
> > prolog.sh
> > -    export
> > LOGDIR="$FUEGO_RW/logs/$TESTDIR/${NODE_NAME}.${TESTSPEC}.${BUIL
> > D_NUMBER}.${BUILD_ID}"
> > -    mkdir -p $LOGDIR
> As stated in previous patch, we shouldn't assume that we're always being
> run by ftc, just yet.  I'm not sure whether the pre_test export or this
> one come first, but I think we should leave this code for now.
> 
> NAK on this hunk.
> 
> > -
> > -    rm -f $LOGDIR/prolog.sh
> > +    if [ -f $LOGDIR/prolog.sh ]; then
> > +        abort_job "$LOGDIR directory contains data from a previous test"
> > +    fi
> >
> > -    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"
> >
> > -    if [ ! -f "$LOGDIR/prolog.sh" ]
> > -    then
> > -        abort_job "$LOGDIR/prolog.sh not found"
> > +    if [ ! -f "$LOGDIR/prolog.sh" ]; then
> > +        abort_job "$LOGDIR/prolog.sh was not created by ovgen.py"
> >      fi
> >
> >      # FIXTHIS: add BUILD_TIMESTAMP and other variables to prolog.sh for --
> > replay
> > diff --git a/engine/scripts/ovgen.py b/engine/scripts/ovgen.py
> > index 98cdf82..7aba86d 100755
> > --- a/engine/scripts/ovgen.py
> > +++ b/engine/scripts/ovgen.py
> > @@ -366,8 +366,8 @@ def parseOverrideFile(overrideFile, layer, ofcls):
> >
> >      return classes
> >
> > -def generateProlog(outFilePath, ofcls, classes, testdir, testspec):
> > -    outfile = open(outFilePath, "w")
> > +def generateProlog(logdir, ofcls, classes, testdir, testspec):
> > +    outfile = open(logdir + '/prolog.sh', "w")
> >
> >      for ofc in classes:
> >          # ofc = ofcls[name]
> > @@ -379,30 +379,29 @@ def generateProlog(outFilePath, ofcls, classes,
> > testdir, testspec):
> >          for var in ofc.vars:
> >              outStr = "%s=\"%s\"" % (var, ofc.vars[var])
> >              outStr = outStr.replace('"', '\"')
> > -            debug_print("%s <- %s" % (outFilePath, outStr))
> > +            debug_print("%s <- %s" % (logdir, outStr))
> >              file.write(outfile, outStr+"\n")
> >
> >          for cap in ofc.cap_list:
> >              outStr = "CAP_%s=\"yes\"" % (cap)
> > -            debug_print("%s <- %s" % (outFilePath, outStr))
> > +            debug_print("%s <- %s" % (logdir, outStr))
> >              file.write(outfile, outStr+"\n")
> >
> >          file.write(outfile, "\n")
> >
> >          for func in ofc.funcs:
> >              body = ofc.funcs[func]
> > -            debug_print("%s <- %s()" % (outFilePath, func))
> > +            debug_print("%s <- %s()" % (logdir, func))
> >              file.write(outfile, body+"\n")
> >
> >          file.write(outfile, "\n")
> >
> > -    ts = parseSpec(testdir, testspec)
> > +    ts = parseSpec(logdir, testdir, testspec)
> >      generateSpec(ts, outfile)
> >
> >  # 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))
> >
> > @@ -431,9 +430,9 @@ def generateSpec(ts, fout):
> >              fout.write(outPattern + "\n")
> >              fout.write(outMessage + "\n")
> >
> > -def parseSpec(testdir, testspec):
> > +def parseSpec(logdir, testdir, testspec):
> >      # FIXTHIS: get fuego-core from env
> > -    specpath = '/fuego-core/engine/tests/%s/spec.json' % (testdir)
> > +    specpath = logdir + '/spec.json'
> OK - this seems to be the major reason for this change.
> 
> In general, I think that using a generic output path is better
> (more decoupled from the rest of Fuego), than using a
> logdir parameter.

I put it in logdir because it is where run.json and other artifacts
needed for replaying/sharing exist.

> Also, it's a bit weird that we have to pass a TESTSPEC variable, when
> there is only one spec in the spec.json file (if I'm reading things correctly).
I am striping the original spec.json file and leaving only the necessary data for replaying/sharing the test. You are right that once we have generated the logdir/spec.json we no longer need the TESTSPEC variable.

> This is required for dynamic vars to work, right?  That is, this looks
> like the final change to actually start using the vars that were deposited
> in $LOGDIR/spec.json by ftc.

Yes. It is also used for normal specs (e.g. engine/tests/Benchmark.Dhrystone/spec.json).

It doesn't work with per-board specs (fuego-ro/boards/board-testsuite-spec.json) yet because "get_specs" is not checking per-board specs
as far as I can see. It doesn't work with variables defined into board files either. What should we do with board file variables? they cause differences between logdir/prolog.sh and logdir/spec.json. I think we should discourage such usage and favour per-board specs.

> This change also makes the code backwards-incompatible with previously-defined
> jobs.  To support backwards-compatibility, can we check for spec.json
> in logdir, and if not present fall back to reading spec.json from the test directory?

Do you mean that in a previous version a spec.json was recorded into the logdir? I haven't seen this functionality.

> 
> >      ts = TestSpecs()
> >
> >      debug_print("Parsing %s spec file" % (specpath))
> > @@ -470,7 +469,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 +492,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()
> This should use --logdir instead of --output, to be consistent.
> 
> >      run(test_args)
> >
> >  run()
> > --
> > 2.7.4
> 
> I did not apply this patch, pending discussion of points above.
>  -- Tim

Thanks a lot for the review!!
Daniel




More information about the Fuego mailing list