[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