[Fuego] A few fixes and a bug report

[Fuego] A few fixes and a bug report

[Daniel Sangorrin]

Hi Tim,

I did a few modifications and found a regression.

[PATCH 1/3] dynvars: add support for simple variable assignments
  -> this implements your proposal about using simple variable
     assignments for dynamic variables

[PATCH 2/3] run-test: use makedirs instead of mkdir
  -> I found cases where the "builds" folder had not been created
     so use os.makedirs instead to get an "mkdir -p" like functionality.

[PATCH 3/3] ovgen: remove test spec strip and generation code
  -> This removes some legacy code. If you want to keep the legacy code
     I am fine, you could leave a "this is deprecated" kind of message

About the regression. I already mentioned it once, but it seems that there
is a regression or at least some problem with the criteria handling code.
Benchmark.Dhrystone fails with these messages:
   Error: criteria missing reference value - returning SKIP
   ERROR: results did not satisfy the threshold

Thanks,
Daniel Sangorrin

[Fuego] [PATCH 1/3] dynvars: add support for simple variable assignments

[Daniel Sangorrin]

Before dynamic-vars require a python dictionary style notation
to define dynamic variables. This was an overkill for simple
variable assignments, as Tim noted. For that reason, this patch
introduces support for simple variable assignments in the
form of "VAR1=VALUE1,VAR2=VALUE2". Internally, these are parsed
and converted into a python dictionary.

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 4ab4ef2..23f09ca 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -243,7 +243,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]
+    [--dynamic-vars variable assignments or python_dict]
 Run the indicated test on the specified board.  Use the
 indicated spec, if one is provided.
 
@@ -254,6 +254,7 @@ 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 Functional.bc -s bc-add --dynamic-vars "EXPR=3+4,RESULT=7"
   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
@@ -3366,9 +3367,16 @@ def do_run_test(conf, options):
 
     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)
+            if dyn_vars_str[0] == '{':
+                import ast
+                dyn_vars = ast.literal_eval(dyn_vars_str)
+            else:
+                dyn_vars = dict()
+                for var_definition in dyn_vars_str.split(','):
+                    key = var_definition.split('=')[0]
+                    value = var_definition.split('=')[1]
+                    dyn_vars[key] = value
         except IndexError:
             error_out('Dynamic vars not provided after --dynamic-vars.')
         options.remove(dyn_vars_str)
-- 
2.7.4

[Fuego] [PATCH 2/3] run-test: use makedirs instead of mkdir

[Daniel Sangorrin]

This allows creating the run_dir recursively, so that if
the "builds" folder does not exist yet it will be created.

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 23f09ca..82ff297 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3672,7 +3672,7 @@ def do_run_test(conf, options):
             # create jenkins directory for this run
             run_dir = job_dir + "/builds/" + build_number
             if not os.path.isdir(run_dir):
-                os.mkdir(run_dir)
+                os.makedirs(run_dir)
 
             # write a build file
             write_build_xml_file_v2(run_dir, build_data)
-- 
2.7.4

[Fuego] [PATCH 3/3] ovgen: remove test spec strip and generation code

[Daniel Sangorrin]

This code was there for legacy Jenkins jobs that called main.sh
directly instead of ftc run-test.

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

diff --git a/engine/scripts/ovgen.py b/engine/scripts/ovgen.py
index 1227e99..25954fa 100755
--- a/engine/scripts/ovgen.py
+++ b/engine/scripts/ovgen.py
@@ -435,19 +435,8 @@ def parseSpec(logdir, testdir, testspec):
     # FIXTHIS: get fuego-core from env
     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]
-        debug_print("test spec data:" + str(test_spec_data))
-        with open(specpath, 'w+') as spec_file:
-            json.dump(test_spec_data, spec_file)
+        debug_print("Fuego now uses ftc run-test to generate the spec.json")
+        raise ParseException("Could not find %s" % (specpath))
 
     ts = TestSpecs()
 
-- 
2.7.4

Re: [Fuego] A few fixes and a bug report

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin
> 
> Hi Tim,
> 
> I did a few modifications and found a regression.
> 
> [PATCH 1/3] dynvars: add support for simple variable assignments
>   -> this implements your proposal about using simple variable
>      assignments for dynamic variables
> 
> [PATCH 2/3] run-test: use makedirs instead of mkdir
>   -> I found cases where the "builds" folder had not been created
>      so use os.makedirs instead to get an "mkdir -p" like functionality.
> 
> [PATCH 3/3] ovgen: remove test spec strip and generation code
>   -> This removes some legacy code. If you want to keep the legacy code
>      I am fine, you could leave a "this is deprecated" kind of message
> 
> About the regression. I already mentioned it once, but it seems that there
> is a regression or at least some problem with the criteria handling code.
> Benchmark.Dhrystone fails with these messages:
>    Error: criteria missing reference value - returning SKIP
>    ERROR: results did not satisfy the threshold

OK - I did a quick peak at the patches, and they look OK.  I won't
have time to apply and test until tomorrow.  I'm in the middle of a
big sweep through the ftc and core code changing logging, and introducing
a new phase 'snapshot'.  Details to follow.  One of the reasons for changing
the output is that it was hiding regressions like the one you found.  I
suspected that something in the criteria code broke - but I'm 
not sure when.  I'll try to find some time to look at it tomorrow.
 -- Tim

Re: [Fuego] [PATCH 3/3] ovgen: remove test spec strip and generation code

[Tim.Bird]

This one caused me some problems.
It is not backwards-compatible with jobs that call main.sh.

See another comment inline below.
 -- Tim

> -----Original Message-----
> From: Daniel Sangorrin
> 
> This code was there for legacy Jenkins jobs that called main.sh
> directly instead of ftc run-test.
I still have some jobs like that.

> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ovgen.py | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/engine/scripts/ovgen.py b/engine/scripts/ovgen.py
> index 1227e99..25954fa 100755
> --- a/engine/scripts/ovgen.py
> +++ b/engine/scripts/ovgen.py
> @@ -435,19 +435,8 @@ def parseSpec(logdir, testdir, testspec):
>      # FIXTHIS: get fuego-core from env
>      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]
> -        debug_print("test spec data:" + str(test_spec_data))
> -        with open(specpath, 'w+') as spec_file:
> -            json.dump(test_spec_data, spec_file)
> +        debug_print("Fuego now uses ftc run-test to generate the spec.json")
> +        raise ParseException("Could not find %s" % (specpath))
This should have be OFParseException().
> 
>      ts = TestSpecs()
> 
> --
> 2.7.4

I'm going to keep the old compatibility code for a version or two,
but I'll add your new code (as comments for now), and a warning
about needing to update jobs.
 -- Tim

Re: [Fuego] [PATCH 2/3] run-test: use makedirs instead of mkdir

[Tim.Bird]

Applied.  Thanks.
 -- Tim

> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> Sent: Thursday, October 04, 2018 6:31 PM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 2/3] run-test: use makedirs instead of mkdir
> 
> This allows creating the run_dir recursively, so that if
> the "builds" folder does not exist yet it will be created.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ftc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index 23f09ca..82ff297 100755
> --- a/engine/scripts/ftc
> +++ b/engine/scripts/ftc
> @@ -3672,7 +3672,7 @@ def do_run_test(conf, options):
>              # create jenkins directory for this run
>              run_dir = job_dir + "/builds/" + build_number
>              if not os.path.isdir(run_dir):
> -                os.mkdir(run_dir)
> +                os.makedirs(run_dir)
> 
>              # write a build file
>              write_build_xml_file_v2(run_dir, build_data)
> --
> 2.7.4
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.linuxfoundation.org_mailman_listinfo_fuego&d=DwICAg&c=fP4tf-
> -1dS0biCFlB0saz0I0kjO5v7-
> GLPtvShAo4cc&r=jjTc71ylyJg68rRxrFQuDFMMybIqPCnrHF85A-
> GzCRg&m=rm72eHJcyn3LfWnwSUjsrJ7VKUrzmbJVgbM-
> 0zqM_80&s=H67sRfdzeCESOBp7csXjk8tMpIdlozqNKaA8mV7wVkQ&e=

Re: [Fuego] [PATCH 1/3] dynvars: add support for simple variable assignments

[Tim.Bird]

Applied.  Nice!  Thanks very much.
 -- Tim

> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> Sent: Thursday, October 04, 2018 6:31 PM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 1/3] dynvars: add support for simple variable
> assignments
> 
> Before dynamic-vars require a python dictionary style notation
> to define dynamic variables. This was an overkill for simple
> variable assignments, as Tim noted. For that reason, this patch
> introduces support for simple variable assignments in the
> form of "VAR1=VALUE1,VAR2=VALUE2". Internally, these are parsed
> and converted into a python dictionary.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ftc | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index 4ab4ef2..23f09ca 100755
> --- a/engine/scripts/ftc
> +++ b/engine/scripts/ftc
> @@ -243,7 +243,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]
> +    [--dynamic-vars variable assignments or python_dict]
>  Run the indicated test on the specified board.  Use the
>  indicated spec, if one is provided.
> 
> @@ -254,6 +254,7 @@ 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 Functional.bc -s bc-add --dynamic-vars
> "EXPR=3+4,RESULT=7"
>    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
> @@ -3366,9 +3367,16 @@ def do_run_test(conf, options):
> 
>      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)
> +            if dyn_vars_str[0] == '{':
> +                import ast
> +                dyn_vars = ast.literal_eval(dyn_vars_str)
> +            else:
> +                dyn_vars = dict()
> +                for var_definition in dyn_vars_str.split(','):
> +                    key = var_definition.split('=')[0]
> +                    value = var_definition.split('=')[1]
> +                    dyn_vars[key] = value
>          except IndexError:
>              error_out('Dynamic vars not provided after --dynamic-vars.')
>          options.remove(dyn_vars_str)
> --
> 2.7.4
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.linuxfoundation.org_mailman_listinfo_fuego&d=DwICAg&c=fP4tf-
> -1dS0biCFlB0saz0I0kjO5v7-
> GLPtvShAo4cc&r=jjTc71ylyJg68rRxrFQuDFMMybIqPCnrHF85A-
> GzCRg&m=Ax3UE4Q_TSiyOQp63MESwdu6nz4J5pziqaltl_It1ec&s=Y1JMS9eZy
> fwNgYmusPVL22H1AV-FbJ5u3zyg6yofiv4&e=