[Fuego] Dynamic variables

[Fuego] Dynamic variables

[Daniel Sangorrin]

Hello Tim,

These patches add dynamic vars to ftc run-test so that one
can easily test variations of a base spec by overriding its
values. Although prolog.sh contains the values used by the
run, I also store a spec.json file on the log directory
for each run. I think this is cleaner and easier to
read than having to dig in prolog.sh

[PATCH 1/3] of: use logdir argument instead of prolog.sh
[PATCH 2/3] OF: create a spec.json on each run
[PATCH 3/3] ftc: add dynamic-vars to ftc run-test

Thanks,
Daniel

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

[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@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"
 
     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

[Fuego] [PATCH 2/3] OF: create a spec.json on each run

[Daniel Sangorrin]

This patch outputs the spec used in a run into a spec.json
file at the run's log folder. Although prolog.sh already
contains the variables from the spec, it is useful to have
a spec.json file that we can share. Also, this spec.json
file could be created by other means such as dynamic
variables instead of coming from a semi-permanent source.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/scripts/ovgen.py | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/engine/scripts/ovgen.py b/engine/scripts/ovgen.py
index 7c1f05f..ef09e85 100755
--- a/engine/scripts/ovgen.py
+++ b/engine/scripts/ovgen.py
@@ -397,7 +397,7 @@ def generateProlog(logdir, ofcls, classes, testdir, testspec):
 
         file.write(outfile, "\n")
 
-    ts = parseSpec(testdir, testspec)
+    ts = parseSpec(logdir, testdir, testspec)
     generateSpec(ts, outfile)
 
 # generateSpec - generate shell output for the spec
@@ -431,9 +431,24 @@ 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'
+    if not os.path.exists(specpath):
+        # create a spec.json by stripping the test's spec.json
+        test_specpath = '/fuego-core/engine/tests/%s/spec.json' % (testdir)
+        with open(test_specpath) as f:
+            try:
+                test_spec_data = json.load(f)
+            except:
+                error_out("Error parsing spec file %s" % specpath)
+        for key in test_spec_data['specs'].keys():
+            if key != testspec:
+                del test_spec_data['specs'][key]
+        print "test spec data" + str(test_spec_data)
+        with open(specpath, 'w+') as spec_file:
+            json.dump(test_spec_data, spec_file)
+
     ts = TestSpecs()
 
     debug_print("Parsing %s spec file" % (specpath))
-- 
2.7.4

[Fuego] [PATCH 3/3] ftc: add dynamic-vars to ftc run-test

[Daniel Sangorrin]

This patch adds the ability to override parameters of a
base spec (if not specified, the default spec is used)
from ftc run-test. The values are logged into the run's
log folder as spec.json. It is useful to modify the
parameters given to a test in a quick manner.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/scripts/ftc | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 2840f5c..875ed59 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -242,6 +242,7 @@ You can obtain a list of run_ids for runs on the local system with
     """Usage: ftc run-test -b <board> -t <test> [-s <spec>] [-p <phases>]
     [-k <kill timeout>] [--rebuild <true|false>] [--reboot <true|false>]
     [--precleanup <true|false>] [--postcleanup <true|false>]
+    [--dynamic-vars python_dict]
 Run the indicated test on the specified board.  Use the
 indicated spec, if one is provided.
 
@@ -251,6 +252,8 @@ rebuild: if true rebuild the test source even if it was already built.
 reboot: if true reboot the board before the test.
 precleanup: if true clean the board's test folder before the test.
 postcleanup: if true clean the board's test folder after the test.
+dynamic-vars: allows overriding the variables in a spec from the command line.
+  Example: ftc run-test -b bbb -t Benchmark.Dhrystone --dynamic-vars "{'LOOPS':'400000000'}"
 
 A list of phase characters may be provided, to execute only those phases
   p = pre_test, c=pre_check, b=build, d=deploy, r=run,
@@ -3219,6 +3222,18 @@ def do_run_test(conf, options):
     else:
         error_out("Run-test command requires a test name")
 
+    if '--dynamic-vars' in options:
+        try:
+            import ast
+            dyn_vars_str = options[options.index('--dynamic-vars') + 1]
+            dyn_vars = ast.literal_eval(dyn_vars_str)
+        except IndexError:
+            error_out('Dynamic vars not provided after --dynamic-vars.')
+        options.remove(dyn_vars_str)
+        options.remove('--dynamic-vars')
+    else:
+        dyn_vars = None
+
     if '-s' in options:
         try:
             spec_name = options[options.index('-s') + 1]
@@ -3226,7 +3241,8 @@ def do_run_test(conf, options):
             error_out('Testspec not provided after -s.')
         spec_list = get_specs(conf, test_name)
         if spec_name not in spec_list:
-            error_out('Unknown spec %s' % spec_name)
+            if not dyn_vars:
+                error_out('Unknown spec %s' % spec_name)
         options.remove(spec_name)
         options.remove('-s')
         test_dict["spec"] = spec_name
@@ -3405,6 +3421,12 @@ def do_run_test(conf, options):
         build_number = find_next_build_number_by_log_scan(logs_dir)
 
     build_data.build_number = build_number
+    build_data.test_logdir = conf.FUEGO_RW + '/logs/' + \
+        build_data.test_name    + '/' + \
+        build_data.board_name   + '.' + \
+        build_data.spec_name    + '.' + \
+        build_data.build_number + '.' + \
+        build_data.build_number
 
     os.environ["EXECUTOR_NUMBER"] = "0"
     os.environ["BUILD_ID"] = build_data.build_number
@@ -3437,6 +3459,36 @@ def do_run_test(conf, options):
     os.environ["Target_PreCleanup"] = build_data.precleanup_flag
     os.environ["Target_PostCleanup"] = build_data.postcleanup_flag
 
+    # prepare a per-run spec.json file
+    specpath = '%s/engine/tests/%s/spec.json' % (conf.FUEGO_CORE, test_name)
+    with open(specpath) as f:
+        try:
+            test_spec_data = json.load(f)
+        except:
+            error_out("Error parsing spec file %s" % specpath)
+
+    for key in test_spec_data['specs'].keys():
+        if key != test.spec:
+            del test_spec_data['specs'][key]
+
+    print "dynvars: " + str(dyn_vars)
+    if dyn_vars:
+        if test.spec not in test_spec_data['specs']:
+            test_spec_data['specs'][test.spec] = dyn_vars
+        else:
+            for key in dyn_vars.keys():
+                test_spec_data['specs'][test.spec][key] = dyn_vars[key]
+        # track what variables where modified
+        test_spec_data['specs'][test.spec]['dyn_vars'] = dyn_vars.keys()
+
+    print "test spec data" + str(test_spec_data)
+
+    # FIXTHIS: use a more pythonic way
+    os.system("mkdir -p " + build_data.test_logdir)
+
+    with open(build_data.test_logdir + '/spec.json', 'w+') as spec_file:
+        json.dump(test_spec_data, spec_file)
+
     # cd to buildzone directory
     saved_cur_dir = os.getcwd()
     os.chdir(build_data.workspace)
-- 
2.7.4

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

[Tim.Bird]

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

Re: [Fuego] [PATCH 2/3] OF: create a spec.json on each run

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin
> 
> This patch outputs the spec used in a run into a spec.json
> file at the run's log folder. Although prolog.sh already
> contains the variables from the spec, it is useful to have
> a spec.json file that we can share. Also, this spec.json
> file could be created by other means such as dynamic
> variables instead of coming from a semi-permanent source.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ovgen.py | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/engine/scripts/ovgen.py b/engine/scripts/ovgen.py
> index 7c1f05f..ef09e85 100755
> --- a/engine/scripts/ovgen.py
> +++ b/engine/scripts/ovgen.py
> @@ -397,7 +397,7 @@ def generateProlog(logdir, ofcls, classes, testdir,
> testspec):
> 
>          file.write(outfile, "\n")
> 
> -    ts = parseSpec(testdir, testspec)
> +    ts = parseSpec(logdir, testdir, testspec)
>      generateSpec(ts, outfile)
> 
>  # generateSpec - generate shell output for the spec
> @@ -431,9 +431,24 @@ 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'
> +    if not os.path.exists(specpath):
> +        # create a spec.json by stripping the test's spec.json
> +        test_specpath = '/fuego-core/engine/tests/%s/spec.json' % (testdir)
> +        with open(test_specpath) as f:
> +            try:
> +                test_spec_data = json.load(f)
> +            except:
> +                error_out("Error parsing spec file %s" % specpath)
> +        for key in test_spec_data['specs'].keys():
> +            if key != testspec:
> +                del test_spec_data['specs'][key]
> +        print "test spec data" + str(test_spec_data)

I'm changing this to a debug_print()

> +        with open(specpath, 'w+') as spec_file:
> +            json.dump(test_spec_data, spec_file)
> +
>      ts = TestSpecs()
> 
>      debug_print("Parsing %s spec file" % (specpath))
> --
> 2.7.4

OK - looks good.  Applied (and adjusted).
 -- Tim

Re: [Fuego] [PATCH 3/3] ftc: add dynamic-vars to ftc run-test

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin
> 
> This patch adds the ability to override parameters of a
> base spec (if not specified, the default spec is used)
> from ftc run-test. The values are logged into the run's
> log folder as spec.json. It is useful to modify the
> parameters given to a test in a quick manner.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ftc | 54
> +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index 2840f5c..875ed59 100755
> --- a/engine/scripts/ftc
> +++ b/engine/scripts/ftc
> @@ -242,6 +242,7 @@ You can obtain a list of run_ids for runs on the local
> system with
>      """Usage: ftc run-test -b <board> -t <test> [-s <spec>] [-p <phases>]
>      [-k <kill timeout>] [--rebuild <true|false>] [--reboot <true|false>]
>      [--precleanup <true|false>] [--postcleanup <true|false>]
> +    [--dynamic-vars python_dict]
>  Run the indicated test on the specified board.  Use the
>  indicated spec, if one is provided.
> 
> @@ -251,6 +252,8 @@ rebuild: if true rebuild the test source even if it was
> already built.
>  reboot: if true reboot the board before the test.
>  precleanup: if true clean the board's test folder before the test.
>  postcleanup: if true clean the board's test folder after the test.
> +dynamic-vars: allows overriding the variables in a spec from the command
> line.
> +  Example: ftc run-test -b bbb -t Benchmark.Dhrystone --dynamic-vars
> "{'LOOPS':'400000000'}"
I'm not a huge fan of the python/json syntax for simple name/value items.
However, I see the potential benefit of being able to specify arbitrary
data structures here.

Would you object to an addition to also support a simplified name=value syntax?
The idea would be something like this:
--dynamic-vars "LOOPS=400000000,OTHER_THING=foo -bar"
Which would be converted to:
"{'LOOPS':'400000000','OTHER_THING':'foo -bar'}"

The code could use the first character to determine which syntax was being used:
'{' for ast, and alpha-numeric for name=value synax.

Out of curiosity - do you expect the most common use case is for people to
type these on the command line, or have them coded into scripts, or have
them generated by some other tool?  (e.g. a variation-creating tool, or
a bisector tool?)

> 
>  A list of phase characters may be provided, to execute only those phases
>    p = pre_test, c=pre_check, b=build, d=deploy, r=run,
> @@ -3219,6 +3222,18 @@ def do_run_test(conf, options):
>      else:
>          error_out("Run-test command requires a test name")
> 
> +    if '--dynamic-vars' in options:
> +        try:
> +            import ast
> +            dyn_vars_str = options[options.index('--dynamic-vars') + 1]
> +            dyn_vars = ast.literal_eval(dyn_vars_str)
> +        except IndexError:
> +            error_out('Dynamic vars not provided after --dynamic-vars.')
> +        options.remove(dyn_vars_str)
> +        options.remove('--dynamic-vars')
> +    else:
> +        dyn_vars = None
> +
>      if '-s' in options:
>          try:
>              spec_name = options[options.index('-s') + 1]
> @@ -3226,7 +3241,8 @@ def do_run_test(conf, options):
>              error_out('Testspec not provided after -s.')
>          spec_list = get_specs(conf, test_name)
>          if spec_name not in spec_list:
> -            error_out('Unknown spec %s' % spec_name)
> +            if not dyn_vars:
> +                error_out('Unknown spec %s' % spec_name)
>          options.remove(spec_name)
>          options.remove('-s')
>          test_dict["spec"] = spec_name
> @@ -3405,6 +3421,12 @@ def do_run_test(conf, options):
>          build_number = find_next_build_number_by_log_scan(logs_dir)
> 
>      build_data.build_number = build_number
> +    build_data.test_logdir = conf.FUEGO_RW + '/logs/' + \
> +        build_data.test_name    + '/' + \
> +        build_data.board_name   + '.' + \
> +        build_data.spec_name    + '.' + \
> +        build_data.build_number + '.' + \
> +        build_data.build_number
> 
>      os.environ["EXECUTOR_NUMBER"] = "0"
>      os.environ["BUILD_ID"] = build_data.build_number
> @@ -3437,6 +3459,36 @@ def do_run_test(conf, options):
>      os.environ["Target_PreCleanup"] = build_data.precleanup_flag
>      os.environ["Target_PostCleanup"] = build_data.postcleanup_flag
> 
> +    # prepare a per-run spec.json file
> +    specpath = '%s/engine/tests/%s/spec.json' % (conf.FUEGO_CORE,
> test_name)
> +    with open(specpath) as f:
> +        try:
> +            test_spec_data = json.load(f)
> +        except:
> +            error_out("Error parsing spec file %s" % specpath)
> +
> +    for key in test_spec_data['specs'].keys():
> +        if key != test.spec:
> +            del test_spec_data['specs'][key]
> +
> +    print "dynvars: " + str(dyn_vars)
> +    if dyn_vars:
> +        if test.spec not in test_spec_data['specs']:
> +            test_spec_data['specs'][test.spec] = dyn_vars
> +        else:
> +            for key in dyn_vars.keys():
> +                test_spec_data['specs'][test.spec][key] = dyn_vars[key]
> +        # track what variables where modified
> +        test_spec_data['specs'][test.spec]['dyn_vars'] = dyn_vars.keys()
> +
> +    print "test spec data" + str(test_spec_data)
> +
> +    # FIXTHIS: use a more pythonic way
> +    os.system("mkdir -p " + build_data.test_logdir)
> +
> +    with open(build_data.test_logdir + '/spec.json', 'w+') as spec_file:
> +        json.dump(test_spec_data, spec_file)
> +
>      # cd to buildzone directory
>      saved_cur_dir = os.getcwd()
>      os.chdir(build_data.workspace)
> --
> 2.7.4

Ok - looks good.  Applied.

Just to clarify this feature:
 - if dynamic variables are specified, ftc creates logdir/spec.json with the dynamic variables overriding any
  values in the specified 'spec', that came from testdir/spec.json
    - also, the spec data is filtered to just the spec that will be used for the run
    - the overlay generator will use logdir/spec.json
    - the list of dynamic vars is noted in the generated spec.json
 - if no dynamic variables are specified, ftc does nothing, and the overlay generator copies
   the spec data from testdir/spec.json into logdir/spec.json
   - the spec data is filtered to just the spec that will be used for the run

This seems like a duplication of code.  Is this to support backwards compatibility with jobs
that still call main.sh, when we do the switchover to calling 'ftc run-test'? 
If so, thanks.  Good job!
 -- Tim

Re: [Fuego] [PATCH 3/3] ftc: add dynamic-vars to ftc run-test

[Daniel Sangorrin]

Hi Tim,

Sorry for the late reply!

> -----Original Message-----
> From: Tim.Bird@sony.com <Tim.Bird@sony.com>
> Sent: Saturday, September 8, 2018 3:56 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 3/3] ftc: add dynamic-vars to ftc run-test
> 
> > -----Original Message-----
> > From: Daniel Sangorrin
> >
> > This patch adds the ability to override parameters of a
> > base spec (if not specified, the default spec is used)
> > from ftc run-test. The values are logged into the run's
> > log folder as spec.json. It is useful to modify the
> > parameters given to a test in a quick manner.
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> >  engine/scripts/ftc | 54
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> ++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> > index 2840f5c..875ed59 100755
> > --- a/engine/scripts/ftc
> > +++ b/engine/scripts/ftc
> > @@ -242,6 +242,7 @@ You can obtain a list of run_ids for runs on the local
> > system with
> >      """Usage: ftc run-test -b <board> -t <test> [-s <spec>] [-p <phases>]
> >      [-k <kill timeout>] [--rebuild <true|false>] [--reboot <true|false>]
> >      [--precleanup <true|false>] [--postcleanup <true|false>]
> > +    [--dynamic-vars python_dict]
> >  Run the indicated test on the specified board.  Use the
> >  indicated spec, if one is provided.
> >
> > @@ -251,6 +252,8 @@ rebuild: if true rebuild the test source even if it was
> > already built.
> >  reboot: if true reboot the board before the test.
> >  precleanup: if true clean the board's test folder before the test.
> >  postcleanup: if true clean the board's test folder after the test.
> > +dynamic-vars: allows overriding the variables in a spec from the command
> > line.
> > +  Example: ftc run-test -b bbb -t Benchmark.Dhrystone --dynamic-vars
> > "{'LOOPS':'400000000'}"
> I'm not a huge fan of the python/json syntax for simple name/value items.
> However, I see the potential benefit of being able to specify arbitrary
> data structures here.
> 
> Would you object to an addition to also support a simplified name=value syntax?
> The idea would be something like this:
> --dynamic-vars "LOOPS=400000000,OTHER_THING=foo -bar"
> Which would be converted to:
> "{'LOOPS':'400000000','OTHER_THING':'foo -bar'}"
> 
> The code could use the first character to determine which syntax was being used:
> '{' for ast, and alpha-numeric for name=value synax.

The above should work with the patch I just sent.

> Out of curiosity - do you expect the most common use case is for people to
> type these on the command line, or have them coded into scripts, or have
> them generated by some other tool?  (e.g. a variation-creating tool, or
> a bisector tool?)

There are several use cases:
  - People trying to find the test parameters that best fit their boards. For example, the number of loops appropriated for their board. Once they found them, they can keep them in per-board files.
  - Developers that want to debug a failure and need to see what happends when they change the parameters.
  - Periodic CI scripts that run Fuego from the command line
  - Fuzzy tools that try to find corner cases where the test fails

I am interested in the latest use case. I want to provide a fuzzy tool for Fuego tests.

> >  A list of phase characters may be provided, to execute only those phases
> >    p = pre_test, c=pre_check, b=build, d=deploy, r=run,
> > @@ -3219,6 +3222,18 @@ def do_run_test(conf, options):
> >      else:
> >          error_out("Run-test command requires a test name")
> >
> > +    if '--dynamic-vars' in options:
> > +        try:
> > +            import ast
> > +            dyn_vars_str = options[options.index('--dynamic-vars') + 1]
> > +            dyn_vars = ast.literal_eval(dyn_vars_str)
> > +        except IndexError:
> > +            error_out('Dynamic vars not provided after --dynamic-vars.')
> > +        options.remove(dyn_vars_str)
> > +        options.remove('--dynamic-vars')
> > +    else:
> > +        dyn_vars = None
> > +
> >      if '-s' in options:
> >          try:
> >              spec_name = options[options.index('-s') + 1]
> > @@ -3226,7 +3241,8 @@ def do_run_test(conf, options):
> >              error_out('Testspec not provided after -s.')
> >          spec_list = get_specs(conf, test_name)
> >          if spec_name not in spec_list:
> > -            error_out('Unknown spec %s' % spec_name)
> > +            if not dyn_vars:
> > +                error_out('Unknown spec %s' % spec_name)
> >          options.remove(spec_name)
> >          options.remove('-s')
> >          test_dict["spec"] = spec_name
> > @@ -3405,6 +3421,12 @@ def do_run_test(conf, options):
> >          build_number = find_next_build_number_by_log_scan(logs_dir)
> >
> >      build_data.build_number = build_number
> > +    build_data.test_logdir = conf.FUEGO_RW + '/logs/' + \
> > +        build_data.test_name    + '/' + \
> > +        build_data.board_name   + '.' + \
> > +        build_data.spec_name    + '.' + \
> > +        build_data.build_number + '.' + \
> > +        build_data.build_number
> >
> >      os.environ["EXECUTOR_NUMBER"] = "0"
> >      os.environ["BUILD_ID"] = build_data.build_number
> > @@ -3437,6 +3459,36 @@ def do_run_test(conf, options):
> >      os.environ["Target_PreCleanup"] = build_data.precleanup_flag
> >      os.environ["Target_PostCleanup"] = build_data.postcleanup_flag
> >
> > +    # prepare a per-run spec.json file
> > +    specpath = '%s/engine/tests/%s/spec.json' % (conf.FUEGO_CORE,
> > test_name)
> > +    with open(specpath) as f:
> > +        try:
> > +            test_spec_data = json.load(f)
> > +        except:
> > +            error_out("Error parsing spec file %s" % specpath)
> > +
> > +    for key in test_spec_data['specs'].keys():
> > +        if key != test.spec:
> > +            del test_spec_data['specs'][key]
> > +
> > +    print "dynvars: " + str(dyn_vars)
> > +    if dyn_vars:
> > +        if test.spec not in test_spec_data['specs']:
> > +            test_spec_data['specs'][test.spec] = dyn_vars
> > +        else:
> > +            for key in dyn_vars.keys():
> > +                test_spec_data['specs'][test.spec][key] = dyn_vars[key]
> > +        # track what variables where modified
> > +        test_spec_data['specs'][test.spec]['dyn_vars'] = dyn_vars.keys()
> > +
> > +    print "test spec data" + str(test_spec_data)
> > +
> > +    # FIXTHIS: use a more pythonic way
> > +    os.system("mkdir -p " + build_data.test_logdir)
> > +
> > +    with open(build_data.test_logdir + '/spec.json', 'w+') as spec_file:
> > +        json.dump(test_spec_data, spec_file)
> > +
> >      # cd to buildzone directory
> >      saved_cur_dir = os.getcwd()
> >      os.chdir(build_data.workspace)
> > --
> > 2.7.4
> 
> Ok - looks good.  Applied.
> 
> Just to clarify this feature:
>  - if dynamic variables are specified, ftc creates logdir/spec.json with the dynamic
> variables overriding any
>   values in the specified 'spec', that came from testdir/spec.json
>     - also, the spec data is filtered to just the spec that will be used for the run
>     - the overlay generator will use logdir/spec.json
>     - the list of dynamic vars is noted in the generated spec.json
>  - if no dynamic variables are specified, ftc does nothing, and the overlay generator
> copies
>    the spec data from testdir/spec.json into logdir/spec.json
>    - the spec data is filtered to just the spec that will be used for the run

Yes, that's correct.

> This seems like a duplication of code.  Is this to support backwards compatibility
> with jobs
> that still call main.sh, when we do the switchover to calling 'ftc run-test'?

Yes that is also correct. I have send a patch that removes the backwards compatibility now that we switched over to calling ftc run-test, but you can turn it into a "Warning, this is deprecated" sort of thing. Probably, that is the wiser choice.

> If so, thanks.  Good job!
>  -- Tim

Thanks to you!
Daniel

Re: [Fuego] [PATCH 3/3] ftc: add dynamic-vars to ftc run-test

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin [mailto:daniel.sangorrin@toshiba.co.jp]
> > This seems like a duplication of code.  Is this to support backwards
> compatibility
> > with jobs
> > that still call main.sh, when we do the switchover to calling 'ftc run-test'?
> 
> Yes that is also correct. I have send a patch that removes the backwards
> compatibility now that we switched over to calling ftc run-test, but you can
> turn it into a "Warning, this is deprecated" sort of thing. Probably, that is the
> wiser choice.

Thanks.  That's what I did.  See my master branch for the warning message.
 -- Tim