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

Tim.Bird at sony.com Tim.Bird at sony.com
Mon Jun 11 21:49:19 UTC 2018



> -----Original Message-----
> From: Daniel Sangorrin 
> > -----Original Message-----
> > From: Tim Bird
> > > -----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.
Ah.  OK.   I think that everything needed for a replay is in
prolog.sh.  For sharing, however, this sounds like a good place
to store it (users can see if it works as expected, and then
"promote" it to be a shared spec by checking it in to 
the test directory, or use it as a per-board spec (when that is
supported properly).

Thanks.
> 
> > 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.

Yeah - I think I was confusing per-board criteria with per-board specs.
I don't see support in the code for per-board specs either.

> 
> > 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.

No. I mean something like this (in ovgen.py):
specpath = logdir + "/spec.json"
if not os.path.exists(spec_path):
    specpath = testdir + "/spec.json"

So that if a test is not called with ftc run-test (called via main.sh, the old way)
it can still detect the old spec file, but not the dynamic vars.
    
> 
> >
> > >      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