[Fuego] [PATCH 02/30] testplan: dbench4 does not have a testdir spec anymore

[Fuego] [PATCH 02/30] testplan: dbench4 does not have a testdir spec anymore

[Daniel Sangorrin]

Use default instead

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/overlays/testplans/testplan_docker.json | 3 +--
 engine/overlays/testplans/testplan_lava.json   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/engine/overlays/testplans/testplan_docker.json b/engine/overlays/testplans/testplan_docker.json
index 673a26d..f43a5ec 100644
--- a/engine/overlays/testplans/testplan_docker.json
+++ b/engine/overlays/testplans/testplan_docker.json
@@ -8,8 +8,7 @@
             "spec": "100M"
         },
         {
-            "testName": "Benchmark.dbench4",
-            "spec": "testdir"
+            "testName": "Benchmark.dbench4"
         },
         {
             "testName": "Benchmark.hackbench"
diff --git a/engine/overlays/testplans/testplan_lava.json b/engine/overlays/testplans/testplan_lava.json
index dd63784..02a6abc 100644
--- a/engine/overlays/testplans/testplan_lava.json
+++ b/engine/overlays/testplans/testplan_lava.json
@@ -8,8 +8,7 @@
             "spec": "100M"
         },
         {
-            "testName": "Benchmark.dbench4",
-            "spec": "testdir"
+            "testName": "Benchmark.dbench4"
         },
         {
             "testName": "Benchmark.hackbench"
-- 
2.7.4

[Fuego] [PATCH 03/30] reboot: reboot the board if reboot flag is set to true

[Daniel Sangorrin]

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

diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
index 7d1cdac..654a1ee 100755
--- a/engine/scripts/functions.sh
+++ b/engine/scripts/functions.sh
@@ -413,6 +413,10 @@ function target_setup_route_to_host () {
 }
 
 function pre_test {
+    if [ "$Reboot" == "true" ]; then
+        target_reboot ${MAX_REBOOT_RETRIES:-20}
+    fi
+
     # Make sure the target is alive, and prepare workspace for the test
     source $FUEGO_RO/toolchains/tools.sh
     export SSHPASS=$PASSWORD
-- 
2.7.4

[Fuego] [PATCH 04/30] plantest_class: rename it and override values in order

[Daniel Sangorrin]

I replaced plantest_class by test_class because it
is used to describe a test, and not only one that comes
from a testplan.

Then I added logic so that test flags (reboot, rebuild etc)
are overriden according to the next criteria:

commandline flags > per-test flags > default_xxx flags > DEFAULT_DEFAULTS

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index a9c0c4f..449b9cf 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -605,17 +605,28 @@ class board_class:
     # FIXTHIS - board_class should have methods to read board and dist file
 
 
-class plantest_class:
-    def __init__(self, test_dict, defaults):
+class test_class:
+    DEFAULT_DEFAULTS = {
+        'timeout' : '30m',
+        'spec'    : 'default',
+        'reboot'  : 'false',
+        'rebuild' : 'false',
+        'precleanup'  : 'true',
+        'postcleanup' : 'true'
+    }
+    def __init__(self, test_dict, test_flags={}):
+        merged_defaults = dict(self.DEFAULT_DEFAULTS)
+        for key, value in test_flags.iteritems():
+            merged_defaults[key] = value
         self.name = str(test_dict["testName"])
         self.test_type = self.name.split(".")[0]
         self.base_name = self.name.split(".")[1]
-        self.spec = str(test_dict.get("spec", defaults['spec']))
-        self.timeout = str(test_dict.get("timeout", defaults['timeout']))
-        self.reboot = str(test_dict.get("reboot", defaults['reboot']))
-        self.rebuild = str(test_dict.get("rebuild", defaults['rebuild']))
-        self.precleanup = str(test_dict.get("precleanup", defaults['precleanup']))
-        self.postcleanup = str(test_dict.get("postcleanup", defaults['postcleanup']))
+        self.spec = str(test_dict.get("spec", merged_defaults['spec']))
+        self.timeout = str(test_dict.get("timeout", merged_defaults['timeout']))
+        self.reboot = str(test_dict.get("reboot", merged_defaults['reboot']))
+        self.rebuild = str(test_dict.get("rebuild", merged_defaults['rebuild']))
+        self.precleanup = str(test_dict.get("precleanup", merged_defaults['precleanup']))
+        self.postcleanup = str(test_dict.get("postcleanup", merged_defaults['postcleanup']))
 
 
 class run_class:
@@ -1366,42 +1377,54 @@ def create_batch_job(board, testplan, plan_tests):
         sys.exit(1)
 
 
-# returns a list of plantest_class instances
-def parse_testplan(testplan, defaults):
+# parse the testplan and returns a list of test_class instances where
+# test flags (timeout, reboot, etc) have been merged in this order
+# commandline flags > per-test flags > default_xxx flags > DEFAULT_DEFAULTS
+def parse_testplan(testplan, test_dict):
     abspath = '/fuego-core/engine/overlays/testplans/' + testplan + '.json'
 
-    plan_tests = []
     with open(abspath, "r") as f:
         plan = json.load(f)
 
-    if 'default_timeout' in plan:
-        defaults['timeout'] = plan['default_timeout']
+    plan_tests = []
+    for plan_test_dict in plan['tests']:
+        # set testplan flags
+        test_flags = dict()
+        if 'default_timeout' in plan:
+            test_flags['timeout'] = plan['default_timeout']
 
-    if 'default_spec' in plan:
-        defaults['spec'] = plan['default_spec']
+        if 'default_spec' in plan:
+            test_flags['spec'] = plan['default_spec']
 
-    if 'default_reboot' in plan:
-        defaults['reboot'] = plan['default_reboot']
+        if 'default_reboot' in plan:
+            test_flags['reboot'] = plan['default_reboot']
 
-    if 'default_rebuild' in plan:
-        defaults['rebuild'] = plan['default_rebuild']
+        if 'default_rebuild' in plan:
+            test_flags['rebuild'] = plan['default_rebuild']
 
-    if 'default_precleanup' in plan:
-        defaults['precleanup'] = plan['default_precleanup']
+        if 'default_precleanup' in plan:
+            test_flags['precleanup'] = plan['default_precleanup']
 
-    if 'default_postcleanup' in plan:
-        defaults['postcleanup'] = plan['default_postcleanup']
+        if 'default_postcleanup' in plan:
+            test_flags['postcleanup'] = plan['default_postcleanup']
 
+        # override with testplan per-test flags
+        for key, value in plan_test_dict.iteritems():
+            if key == "testName":
+                test_dict[key] = value
+                continue
+            test_flags[key] = value
 
-    for test_dict in plan['tests']:
-        # test_dict is a dictionary with values for the test
-        test = plantest_class(test_dict, defaults)
+        # test_class overrides flags with those in test_dict and applies
+        # DEFAULT_DEFAULTS for non-specified flags
+        test = test_class(test_dict, test_flags)
         plan_tests.append(test)
 
     return plan_tests
 
 
 def do_add_jobs(conf, options):
+    test_dict = {}
     if '-b' in options:
         try:
             board = options[options.index('-b') + 1]
@@ -1409,7 +1432,7 @@ def do_add_jobs(conf, options):
             error_out("Board not provided after -b.")
         options.remove('-b')
         options.remove(board)
-
+        test_dict["board"] = board
         boards = board.split(",")
         board_list = get_fuego_boards(conf).keys()
         for board in boards:
@@ -1426,8 +1449,10 @@ def do_add_jobs(conf, options):
             error_out('Rebuild option not provided after --rebuild')
         if rebuild not in ['true', 'false']:
             error_out("Invalid rebuild option '%s'" % rebuild)
+        options.remove(rebuild)
+        options.remove('--rebuild')
+        test_dict["rebuild"] = rebuild
 
-    timeout = '30m'
     if '-p' in options:
         try:
             testplan = options[options.index('-p') + 1]
@@ -1442,6 +1467,7 @@ def do_add_jobs(conf, options):
     elif '-t' in options:
         # FIXTHIS: have add-jobs support wildcards in the test name
         test_name, options = get_test_arg("Add-jobs", conf, options)
+        test_dict["testName"] = test_name
         if '-s' in options:
             try:
                 spec = options[options.index('-s') + 1]
@@ -1452,42 +1478,26 @@ def do_add_jobs(conf, options):
                 error_out('Unknown spec %s' % spec)
             options.remove('-s')
             options.remove(spec)
-        else:
-            spec = 'default'
+            test_dict["spec"] = spec
+
         if '-k' in options:
             try:
                 timeout = options[options.index('-k') + 1]
                 options.remove('-k')
             except IndexError:
                 error_out('No timeout specified after -k')
-
+            test_dict["timeout"] = timeout
     else:
         error_out('No testplan or testcase supplied.')
 
-    defaults = {
-        'timeout' : '30m',
-        'spec'    : 'default',
-        'reboot'  : 'false',
-        'rebuild' : 'false',
-        'precleanup'  : 'true',
-        'postcleanup' : 'true'
-    }
-
     if test_name:
-        # FIXTHIS - we could parse more parameters for the job here, from the ftc command line
-        # use all defaults, except for the spec
-        tp_dict = {"testName": test_name, "timeout": timeout, "spec": spec }
-        if rebuild:
-            tp_dict["rebuild"] = rebuild
-        test = plantest_class(tp_dict, defaults)
+        test = test_class(test_dict)
         for board in boards:
             create_job(board, test)
     else:
-        plan_tests = parse_testplan(testplan, defaults)
+        plan_tests = parse_testplan(testplan, test_dict)
         for board in boards:
             for test in plan_tests:
-                if rebuild:
-                    test.rebuild = rebuild
                 create_job(board, test)
             create_batch_job(board, testplan, plan_tests)
     sys.exit(0)
-- 
2.7.4

[Fuego] [PATCH 05/30] ftc: divide cleanup into pre and postcleanup

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 449b9cf..af399e1 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -25,7 +25,7 @@
 # - add do_run_request - to run a job request from the server
 #    if no arguments, run the next available request on the server
 # - finish do_run_test
-#    - support flags: Reboot, Target_Cleanup, Rebuild
+#    - support flags: Reboot, Rebuild
 #    - get log results into file (switch to subprocess?)
 #    - make Jenkins recognize ftc test result
 # - finish do_set
@@ -717,7 +717,7 @@ class run_class:
         # FIXTHIS - in run_class, convert build.xml startTime from seconds to timestamp
 
         # lower-case some things:
-        for key in ["TESTPLAN", "Reboot", "Rebuild", "Target_Cleanup", "Device"]:
+        for key in ["TESTPLAN", "Reboot", "Rebuild", "Target_PreCleanup", "Target_PostCleanup", "Device"]:
             lkey = key.lower()
             try:
                 self.__dict__[lkey] = self.__dict__[key]
@@ -746,7 +746,8 @@ class run_class:
                 ("testplan_name", "testplan"),
                 ("reboot_flag", "reboot"),
                 ("rebuild_flag", "rebuild"),
-                ("target_cleanup_flag", "target_cleanup"),
+                ("precleanup_flag", "precleanup"),
+                ("postcleanup_flag", "postcleanup"),
                 ("board_name", "device"),
         ]
 
@@ -770,7 +771,7 @@ class run_class:
         self.files = ["devlog.txt", "syslog.before.txt", "syslog.after.txt",
             "testlog.txt", "consolelog.txt", "build.xml"]
         keylist = ["test_name", "timestamp", "num", "host", "board",
-            "result", "device", "reboot", "rebuild", "testplan", "target_cleanup",
+            "result", "device", "reboot", "rebuild", "testplan", "precleanup", "postcleanup",
             "start_time", "description", "duration", "charset", "keep_log",
             "built_on", "workspace", "cause",
             "files"]
@@ -2648,7 +2649,7 @@ def do_run_request(conf, options):
     except:
         error_out("Request %s has invalid board %s" % (req_id, board_name))
 
-    # FIXTHIS - run_request: should process Reboot, Rebuild and Target_cleanup flags
+    # FIXTHIS - run_request: should process Reboot, Rebuild and Pre/Post cleanup flags
     print("Executing test %s on board %s (using %s)" % (test_name, board_name, testplan))
     do_run_test(conf, ['-b', board_name, '-t', test_name, '-p', testplan])
 
@@ -2712,11 +2713,11 @@ class data_class:
         else:
             return item
 
-
+# FIXTHIS: this is dead code
 def write_run_build_xml_file(run_dir, build_data):
     ### the format str needs build_data to have the following attributes:
     # board_name
-    # reboot_flag, rebuild_flag, target_cleanup_flag
+    # reboot_flag, rebuild_flag, precleanup_flag, postcleanup_flag
     #    the flags must be one of (lower case) 'true' or 'false'
     # testplan_name
     # build_number
@@ -2754,9 +2755,14 @@ def write_run_build_xml_file(run_dir, build_data):
           <value>%(rebuild_flag)s</value>
         </hudson.model.BooleanParameterValue>
         <hudson.model.BooleanParameterValue>
-          <name>Target_Cleanup</name>
+          <name>Target_PreCleanup</name>
+          <description></description>
+          <value>%(precleanup_flag)s</value>
+        </hudson.model.BooleanParameterValue>
+        <hudson.model.BooleanParameterValue>
+          <name>Target_PostCleanup</name>
           <description></description>
-          <value>true</value>
+          <value>%(postcleanup_flag)s</value>
         </hudson.model.BooleanParameterValue>
         <hudson.model.StringParameterValue>
           <name>TESTPLAN</name>
@@ -2798,11 +2804,10 @@ def write_run_build_xml_file(run_dir, build_data):
 
     fd.close()
 
-
 def write_build_xml_file(run_dir, build_data):
     ### the format str needs build_data to have the following attributes:
     # board_name
-    # reboot_flag, rebuild_flag, target_cleanup_flag
+    # reboot_flag, rebuild_flag, precleanup_flag, postcleanup_flag
     #    the flags must be one of (lower case) 'true' or 'false'
     # testplan_name
     # build_number
@@ -2840,9 +2845,14 @@ def write_build_xml_file(run_dir, build_data):
           <value>%(rebuild_flag)s</value>
         </hudson.model.BooleanParameterValue>
         <hudson.model.BooleanParameterValue>
-          <name>Target_Cleanup</name>
+          <name>Target_PreCleanup</name>
+          <description></description>
+          <value>%(precleanup_flag)s</value>
+        </hudson.model.BooleanParameterValue>
+        <hudson.model.BooleanParameterValue>
+          <name>Target_PostCleanup</name>
           <description></description>
-          <value>true</value>
+          <value>%(postcleanup_flag)s</value>
         </hudson.model.BooleanParameterValue>
         <hudson.model.StringParameterValue>
           <name>TESTPLAN</name>
@@ -2994,7 +3004,8 @@ def do_put_request(conf, options):
             "board": board,
             "reboot": reboot,
             "rebuild": "False",
-            "target_cleanup": "False",
+            "precleanup": "True",
+            "postcleanup": "True",
             "testplan": plan,
             "requestor": "Tim",
             }
@@ -3146,6 +3157,26 @@ def do_run_test(conf, options):
         options.remove('-p')
         os.environ["FUEGO_TEST_PHASES"] = test_phases
 
+    if '--precleanup' in options:
+        try:
+            precleanup = options[options.index('--precleanup') + 1]
+        except IndexError:
+            error_out('Precleanup option not provided after --precleanup')
+        if precleanup not in ['true', 'false']:
+            error_out("Invalid precleanup option '%s'" % precleanup)
+    else:
+        precleanup = 'true'
+
+    if '--postcleanup' in options:
+        try:
+            postcleanup = options[options.index('--postcleanup') + 1]
+        except IndexError:
+            error_out('Postcleanup option not provided after --postcleanup')
+        if postcleanup not in ['true', 'false']:
+            error_out("Invalid postcleanup option '%s'" % postcleanup)
+    else:
+        postcleanup = 'true'
+
     # detect whether this is a Jenkins job or not:
     try:
         jenkins_url = os.environ("JENKINS_URL")
@@ -3193,7 +3224,8 @@ def do_run_test(conf, options):
     # FIXTHIS - do_run_test: read flag options (e.g. reboot) from command line (from 'options')
     build_data.reboot_flag = "false"
     build_data.rebuild_flag = "false"
-    build_data.target_cleanup_flag = "true"
+    build_data.precleanup_flag = precleanup
+    build_data.postcleanup_flag = postcleanup
 
     # FIXTHIS - do_run_test: set job description
     # set job description in run json file
@@ -3269,9 +3301,8 @@ def do_run_test(conf, options):
 
     os.environ["Reboot"] = build_data.reboot_flag
     os.environ["Rebuild"] = build_data.rebuild_flag
-    # FIXTHIS - do_run_test: support separate pre and post cleanup flags
-    os.environ["Target_PreCleanup"] = build_data.target_cleanup_flag
-    os.environ["Target_PostCleanup"] = build_data.target_cleanup_flag
+    os.environ["Target_PreCleanup"] = build_data.precleanup_flag
+    os.environ["Target_PostCleanup"] = build_data.postcleanup_flag
 
     # cd to buildzone directory
     saved_cur_dir = os.getcwd()
-- 
2.7.4

[Fuego] [PATCH 06/30] add_jobs: use error_out

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index af399e1..b03bee3 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -1438,9 +1438,9 @@ def do_add_jobs(conf, options):
         board_list = get_fuego_boards(conf).keys()
         for board in boards:
             if board not in board_list:
-                raise Exception("Board '%s' not found." % board)
+                error_out("Board '%s' not found." % board)
     else:
-        raise Exception("No board name supplied.")
+        error_out("No board name supplied.")
 
     rebuild = ''
     if '--rebuild' in options:
-- 
2.7.4

[Fuego] [PATCH 07/30] add-jobs: put the options in the error message

[Daniel Sangorrin]

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 b03bee3..d6182e8 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -1489,7 +1489,7 @@ def do_add_jobs(conf, options):
                 error_out('No timeout specified after -k')
             test_dict["timeout"] = timeout
     else:
-        error_out('No testplan or testcase supplied.')
+        error_out('No testplan (-p) or test (-t) supplied.')
 
     if test_name:
         test = test_class(test_dict)
-- 
2.7.4

[Fuego] [PATCH 08/30] add-jobs: allow spec and timeout to be always specified

[Daniel Sangorrin]

The command line values have the highest priority and will
override everything else.

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index d6182e8..4ef790e 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -1469,28 +1469,29 @@ def do_add_jobs(conf, options):
         # FIXTHIS: have add-jobs support wildcards in the test name
         test_name, options = get_test_arg("Add-jobs", conf, options)
         test_dict["testName"] = test_name
-        if '-s' in options:
-            try:
-                spec = options[options.index('-s') + 1]
-            except IndexError:
-                error_out('Testspec not provided after -s.')
-            specnames = get_specs(conf, test_name)
-            if spec not in specnames:
-                error_out('Unknown spec %s' % spec)
-            options.remove('-s')
-            options.remove(spec)
-            test_dict["spec"] = spec
-
-        if '-k' in options:
-            try:
-                timeout = options[options.index('-k') + 1]
-                options.remove('-k')
-            except IndexError:
-                error_out('No timeout specified after -k')
-            test_dict["timeout"] = timeout
     else:
         error_out('No testplan (-p) or test (-t) supplied.')
 
+    if '-s' in options:
+        try:
+            spec = options[options.index('-s') + 1]
+        except IndexError:
+            error_out('Testspec not provided after -s.')
+        specnames = get_specs(conf, test_name)
+        if spec not in specnames:
+            error_out('Unknown spec %s' % spec)
+        options.remove('-s')
+        options.remove(spec)
+        test_dict["spec"] = spec
+
+    if '-k' in options:
+        try:
+            timeout = options[options.index('-k') + 1]
+            options.remove('-k')
+        except IndexError:
+            error_out('No timeout specified after -k')
+        test_dict["timeout"] = timeout
+
     if test_name:
         test = test_class(test_dict)
         for board in boards:
-- 
2.7.4

[Fuego] [PATCH 09/30] add-jobs: remove timeout options outside the try

[Daniel Sangorrin]

FIXTHIS: move to argparse in the close future

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 4ef790e..cf1af30 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -1487,9 +1487,10 @@ def do_add_jobs(conf, options):
     if '-k' in options:
         try:
             timeout = options[options.index('-k') + 1]
-            options.remove('-k')
         except IndexError:
             error_out('No timeout specified after -k')
+        options.remove('-k')
+        options.remove(timeout)
         test_dict["timeout"] = timeout
 
     if test_name:
-- 
2.7.4

[Fuego] [PATCH 10/30] timeout: add a regex match to the format of the timeout

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index cf1af30..c92a1a8 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -1489,6 +1489,8 @@ def do_add_jobs(conf, options):
             timeout = options[options.index('-k') + 1]
         except IndexError:
             error_out('No timeout specified after -k')
+        if re.match('^\d+[dhms]', timeout) is None:
+            error_out('%s: Timeout format not supported.' % timeout)
         options.remove('-k')
         options.remove(timeout)
         test_dict["timeout"] = timeout
-- 
2.7.4

[Fuego] [PATCH 11/30] add-jobs: support reboot, rebuild, pre and postcleanup flags

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index c92a1a8..f50795c 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -25,7 +25,6 @@
 # - add do_run_request - to run a job request from the server
 #    if no arguments, run the next available request on the server
 # - finish do_run_test
-#    - support flags: Reboot, Rebuild
 #    - get log results into file (switch to subprocess?)
 #    - make Jenkins recognize ftc test result
 # - finish do_set
@@ -1442,7 +1441,6 @@ def do_add_jobs(conf, options):
     else:
         error_out("No board name supplied.")
 
-    rebuild = ''
     if '--rebuild' in options:
         try:
             rebuild = options[options.index('--rebuild') + 1]
@@ -1472,6 +1470,39 @@ def do_add_jobs(conf, options):
     else:
         error_out('No testplan (-p) or test (-t) supplied.')
 
+    if '--reboot' in options:
+        try:
+            reboot = options[options.index('--reboot') + 1]
+        except IndexError:
+            error_out('Reboot option not provided after --reboot')
+        if reboot not in ['true', 'false']:
+            error_out("Invalid reboot option '%s'" % reboot)
+        options.remove(reboot)
+        options.remove('--reboot')
+        test_dict["reboot"] = reboot
+
+    if '--precleanup' in options:
+        try:
+            precleanup = options[options.index('--precleanup') + 1]
+        except IndexError:
+            error_out('Precleanup option not provided after --precleanup')
+        if precleanup not in ['true', 'false']:
+            error_out("Invalid precleanup option '%s'" % precleanup)
+        options.remove(precleanup)
+        options.remove('--precleanup')
+        test_dict["precleanup"] = precleanup
+
+    if '--postcleanup' in options:
+        try:
+            postcleanup = options[options.index('--postcleanup') + 1]
+        except IndexError:
+            error_out('Postcleanup option not provided after --postcleanup')
+        if postcleanup not in ['true', 'false']:
+            error_out("Invalid postcleanup option '%s'" % postcleanup)
+        options.remove(postcleanup)
+        options.remove('--postcleanup')
+        test_dict["postcleanup"] = postcleanup
+
     if '-s' in options:
         try:
             spec = options[options.index('-s') + 1]
-- 
2.7.4

[Fuego] [PATCH 12/30] ftc: modify misleading comment

[Daniel Sangorrin]

LOGDIR is something different

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 f50795c..7a9d23c 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -1579,7 +1579,7 @@ def do_rm_jobs(conf, options):
         if not confirm[0] in "Yy":
             sys.exit(1)
         if remove_logs:
-            # remove files and folders in LOGDIR
+            # remove files and folders under the logs directory
             path = conf.FUEGO_RW + "/logs/*"
             files = glob.glob(path)
             for f in files:
-- 
2.7.4

[Fuego] [PATCH 13/30] ftc exec: convert timeout to time units

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 7a9d23c..fc900ed 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -2966,7 +2966,14 @@ def ftc_exec_command(command, timeout):
 
     # specify timeout for command operation
     signal.signal(signal.SIGALRM, alarm_handler)
-    signal.alarm(timeout)
+
+    # timeout is passed as integer[dhms] (dhms: days, hours, minutes, seconds)
+    print "timeout is: " + timeout
+    units = timeout[:-1]
+    multiplier = {"d": 24*60*60, "h": 60*60, "m": 60, "s": 1}
+    time = int(units)*multiplier[timeout[-1]]
+    print "setting alarm to: " + str(time) + " seconds"
+    signal.alarm(time)
 
     try:
         # p.poll returns exit code when process completes
-- 
2.7.4

[Fuego] [PATCH 14/30] run-test: error out if no test is provided

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index fc900ed..091be7e 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3158,10 +3158,11 @@ def do_run_test(conf, options):
     board, options = get_board_arg("Run-test", conf, options)
     board_name = board.name
 
-    test_name = None
     if '-t' in options:
         # FIXTHIS: have run-test support wildcards in the test name
         test_name, options = get_test_arg("Run-test", conf, options)
+    else:
+        error_out("Run-test command requires a test name")
 
     if '-s' in options:
         try:
-- 
2.7.4

[Fuego] [PATCH 15/30] run-test: add support for timeout, rebuild, reboot flags

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 091be7e..ac9a28b 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3200,6 +3200,41 @@ def do_run_test(conf, options):
         options.remove('-p')
         os.environ["FUEGO_TEST_PHASES"] = test_phases
 
+    # FIXTHIS: default values should be obtained from test_class
+
+    if '--timeout' in options:
+        try:
+            timeout = options[options.index('--timeout') + 1]
+        except IndexError:
+            error_out('Timeout (e.g. 30m) not provided after --timeout.')
+
+        if re.match('^\d+[dhms]', timeout) is None:
+            error_out('%s: Timeout format not supported.' % timeout)
+        options.remove(timeout)
+        options.remove('--timeout')
+    else:
+        timeout = "30m"
+
+    if '--rebuild' in options:
+        try:
+            rebuild = options[options.index('--rebuild') + 1]
+        except IndexError:
+            error_out('Rebuild option not provided after --rebuild')
+        if rebuild not in ['true', 'false']:
+            error_out("Invalid rebuild option '%s'" % rebuild)
+    else:
+        rebuild = 'false'
+
+    if '--reboot' in options:
+        try:
+            reboot = options[options.index('--reboot') + 1]
+        except IndexError:
+            error_out('Reboot option not provided after --reboot')
+        if reboot not in ['true', 'false']:
+            error_out("Invalid reboot option '%s'" % reboot)
+    else:
+        reboot = 'false'
+
     if '--precleanup' in options:
         try:
             precleanup = options[options.index('--precleanup') + 1]
@@ -3262,11 +3297,8 @@ def do_run_test(conf, options):
     build_data.job_name = job_name
     build_data.workspace = conf.FUEGO_RW+"/buildzone"
     build_data.start_time = long(time.time() * 1000)
-
-    # force these for now, but
-    # FIXTHIS - do_run_test: read flag options (e.g. reboot) from command line (from 'options')
-    build_data.reboot_flag = "false"
-    build_data.rebuild_flag = "false"
+    build_data.reboot_flag = reboot
+    build_data.rebuild_flag = rebuild
     build_data.precleanup_flag = precleanup
     build_data.postcleanup_flag = postcleanup
 
@@ -3383,8 +3415,6 @@ def do_run_test(conf, options):
     fcntl.fcntl(tail_fd, fcntl.F_SETFL, flag | os.O_NONBLOCK)
 
     command = "/bin/bash -xe %s" % (tempfilename)
-    # FIXTHIS - in do_run_test: timeout is hardcoded to 5 minutes
-    timeout = 300    # 5 minutes
     rcode = ftc_exec_command(command, timeout)
     log.flush()
 
-- 
2.7.4

[Fuego] [PATCH 16/30] run-test: use an extra variable to indicate the caller

[Daniel Sangorrin]

This way we can also handle nested calls

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index ac9a28b..6a8fbdb 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3255,15 +3255,6 @@ def do_run_test(conf, options):
     else:
         postcleanup = 'true'
 
-    # detect whether this is a Jenkins job or not:
-    try:
-        jenkins_url = os.environ("JENKINS_URL")
-        fuego_caller = "jenkins"
-    except:
-        jenkins_url = None
-        fuego_caller = "ftc"
-        print "Notice: non-Jenkins test request detected"
-
     print "Running test '%s' on board '%s' using spec '%s'" % (test_name, board_name, spec_name)
 
     # check if there's a Jenkins job that matches this request
@@ -3306,6 +3297,27 @@ def do_run_test(conf, options):
     # set job description in run json file
     build_data.description = "Test %s run by ftc" % build_data.test_name
 
+    # run_test can be used in different situations
+    #    - called from jenkins
+    #        - FUEGO_CALLER="jenkins"
+    #    - called from the command line (ftc)
+    #        - FUEGO_CALLER not set
+    #    - called from fuego_test.sh (nested)
+    #        - FUEGO_CALLER="ftc"
+
+    try:
+        fuego_caller = os.environ["FUEGO_CALLER"]
+        if fuego_caller == "jenkins":
+            print("ftc was called from Jenkins")
+        elif fuego_caller == "ftc":
+            print("ftc was called from fuego_test (nested)")
+        else:
+            error_out("fuego_caller has an unexpected value: " + fuego_caller)
+    except:
+        print("ftc was called from the command line")
+        fuego_caller = "ftc"
+        os.environ["FUEGO_CALLER"] = "ftc"
+
     # export other vars (that Jenkins would export)
     # FIXTHIS: do_run_test: variables have changed (e.g. TESTSPEC, ..)
     os.environ["TESTPLAN"] = build_data.testplan_name
-- 
2.7.4

[Fuego] [PATCH 17/30] run-test: fix job dir and build number creation

[Daniel Sangorrin]

This makes it possible to run a job from jenkins or
the command line and keep the nextbuildnumber synchronized.

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 6a8fbdb..142c31b 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3257,15 +3257,7 @@ def do_run_test(conf, options):
 
     print "Running test '%s' on board '%s' using spec '%s'" % (test_name, board_name, spec_name)
 
-    # check if there's a Jenkins job that matches this request
-    # Job names have the syntax: <board>.<spec>.<test_name>
     job_name = "%s.%s.%s" % (board_name, spec_name, test_name)
-    job_dir = conf.JENKINS_HOME+"/jobs/"+job_name
-    if not os.path.isfile(job_dir+"/config.xml"):
-        print "Warning: no matching Jenkins job found.  Not populating Jenkins build directory"
-        job_dir = None
-
-    #pvar("job_dir")
 
     print "!!! >>> Ready to run test! <<< !!!"
 
@@ -3318,6 +3310,16 @@ def do_run_test(conf, options):
         fuego_caller = "ftc"
         os.environ["FUEGO_CALLER"] = "ftc"
 
+    # check if there is a jenkins job that matches ours.
+    # Example: /var/lib/jenkins/jobs/bbb.default.Benchmark.iperf3/builds/2/
+    job_dir = conf.JENKINS_HOME + "/jobs/" + build_data.job_name
+    if not os.path.isdir(job_dir):
+        if fuego_caller == "jenkins":
+            error_out("Jenkins did not create jobs folder " + job_dir)
+        print "No matching Jenkins job found."
+        os.mkdir(job_dir)
+        os.mkdir(job_dir + "/builds/")
+
     # export other vars (that Jenkins would export)
     # FIXTHIS: do_run_test: variables have changed (e.g. TESTSPEC, ..)
     os.environ["TESTPLAN"] = build_data.testplan_name
@@ -3340,25 +3342,25 @@ def do_run_test(conf, options):
     timestamp = time.strftime("%FT%T%z")
     build_data.timestamp = timestamp
 
-    if job_dir:
-        build_number = find_next_build_number(job_dir)
-        pvar("build_number")
+    # set build_data.build_number depending on the situation
+    if fuego_caller == "jenkins":
+        build_data.build_number = os.environ["BUILD_NUMBER"]
+    else:
         filename = job_dir + "/nextBuildNumber"
-        next_build_number = str(int(build_number)+1)
-        # update nextBuildNumber
+        if not os.path.isfile(filename):
+            print "First time we run this job"
+            build_data.build_number = "1"
+        else:
+            build_data.build_number = find_next_build_number(job_dir)
+        next_build_number = str(int(build_data.build_number)+1)
         try:
+            print "Updating nextBuildNumber: " + filename
             fd = open(filename, "w+")
             fd.write(next_build_number+'\n')
             fd.close()
         except:
-            print "Error: problem writing to file %s" % filename
-    else:
-        logs_dir = conf.FUEGO_RW + "/logs/%s" % test_name
-        if not os.path.isdir(logs_dir):
-             os.mkdir(logs_dir)
-        build_number = find_next_build_number_by_log_scan(logs_dir)
-
-    build_data.build_number = build_number
+            error_out("Problem writing to nextBuildNumber: %s" % filename)
+    dprint("build number: " + build_data.build_number)
 
     os.environ["EXECUTOR_NUMBER"] = "0"
     os.environ["BUILD_ID"] = build_data.build_number
-- 
2.7.4

[Fuego] [PATCH 18/30] run-test: put job_name directly

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 142c31b..c307c5c 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3257,8 +3257,6 @@ def do_run_test(conf, options):
 
     print "Running test '%s' on board '%s' using spec '%s'" % (test_name, board_name, spec_name)
 
-    job_name = "%s.%s.%s" % (board_name, spec_name, test_name)
-
     print "!!! >>> Ready to run test! <<< !!!"
 
     # make sure environment variables are set:
@@ -3277,7 +3275,7 @@ def do_run_test(conf, options):
     build_data.testplan_name = "None"
     build_data.spec_name = spec_name
     build_data.testdir = test_name
-    build_data.job_name = job_name
+    build_data.job_name = "%s.%s.%s" % (board_name, spec_name, test_name)
     build_data.workspace = conf.FUEGO_RW+"/buildzone"
     build_data.start_time = long(time.time() * 1000)
     build_data.reboot_flag = reboot
-- 
2.7.4

[Fuego] [PATCH 19/30] run-test: remove redundant print

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index c307c5c..d6d5ede 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3257,8 +3257,6 @@ def do_run_test(conf, options):
 
     print "Running test '%s' on board '%s' using spec '%s'" % (test_name, board_name, spec_name)
 
-    print "!!! >>> Ready to run test! <<< !!!"
-
     # make sure environment variables are set:
 
     # export stuff from configuration into environment
-- 
2.7.4

[Fuego] [PATCH 20/30] run-test: reorder os environment settings

[Daniel Sangorrin]

Be clear about which variables emulate jenkins variables
and which are pure fuego variables

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index d6d5ede..57976d8 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3257,15 +3257,6 @@ def do_run_test(conf, options):
 
     print "Running test '%s' on board '%s' using spec '%s'" % (test_name, board_name, spec_name)
 
-    # make sure environment variables are set:
-
-    # export stuff from configuration into environment
-    export_list = ["FUEGO_CORE", "FUEGO_RO", "FUEGO_RW"]
-    for var in export_list:
-        os.environ[var] = conf.__dict__[var]
-
-    os.environ["FUEGO_HOST"] = conf.host
-
     # construct the build_data map to hold information about this build
     build_data = data_class()
     build_data.board_name = board_name
@@ -3316,28 +3307,6 @@ def do_run_test(conf, options):
         os.mkdir(job_dir)
         os.mkdir(job_dir + "/builds/")
 
-    # export other vars (that Jenkins would export)
-    # FIXTHIS: do_run_test: variables have changed (e.g. TESTSPEC, ..)
-    os.environ["TESTPLAN"] = build_data.testplan_name
-    os.environ["JOB_NAME"] = build_data.job_name
-    os.environ["NODE_NAME"] = build_data.board_name
-    os.environ["NODE_LABELS"] = build_data.board_name
-    os.environ["Device"] = build_data.board_name
-    os.environ["TESTDIR"] = build_data.testdir
-    os.environ["WORKSPACE"] = build_data.workspace
-    os.environ["TESTSPEC"] = build_data.spec_name
-    # FIXTHIS - set this if user specifies --debug in options
-    os.environ["FUEGO_DEBUG"] = "1"
-
-    for var_name in board.env_vars.keys():
-        os.environ[var_name] = board.env_vars[var_name]
-
-    # and do synchronization with jenkins job at end, if possible
-
-    # set BUILD_NUMBER, BUILD_ID, BUILD_TAG and EXECUTOR_NUMBER
-    timestamp = time.strftime("%FT%T%z")
-    build_data.timestamp = timestamp
-
     # set build_data.build_number depending on the situation
     if fuego_caller == "jenkins":
         build_data.build_number = os.environ["BUILD_NUMBER"]
@@ -3358,36 +3327,47 @@ def do_run_test(conf, options):
             error_out("Problem writing to nextBuildNumber: %s" % filename)
     dprint("build number: " + build_data.build_number)
 
-    os.environ["EXECUTOR_NUMBER"] = "0"
-    os.environ["BUILD_ID"] = build_data.build_number
-    os.environ["BUILD_NUMBER"] = build_data.build_number
-    os.environ["BUILD_TIMESTAMP"] = build_data.timestamp
-
-    # logdir path is ${NODE_NAME}.${TESTSPEC}.${BUILD_NUMBER}.${BUILD_ID}
-    # FIXTHIS - do_run_test: logdir path should have timestamp in it
-    log_dir = conf.FUEGO_RW + "/logs/%(test_name)s/%(board_name)s.%(spec_name)s.%(build_number)s.%(build_number)s"  % build_data
-
-    if not os.path.isdir(log_dir):
-        os.mkdir(log_dir)
-
-    # create build_number directory
-    if job_dir:
-        run_dir = job_dir + "/builds/" + build_number
-        os.mkdir(run_dir)
-        console_log_filename = "log"
-    else:
-        run_dir = log_dir
-        console_log_filename = "consolelog.txt"
-
-    os.environ["BUILD_TAG"] = "ftc-%s-%s" % (test_name, build_number)
-
-    os.environ["JENKINS_HOME"] = conf.JENKINS_HOME
-    os.environ["JENKINS_URL"] = conf.JENKINS_URL
-
+    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
+
+    # set environment variables
+    ## set environment variables that Jenkins would export
+    if fuego_caller != "jenkins":
+        os.environ["BUILD_NUMBER"] = build_data.build_number
+        os.environ["BUILD_ID"] = build_data.build_number
+        os.environ["EXECUTOR_NUMBER"] = "0"
+        os.environ["BUILD_TAG"] = "ftc-%s-%s" % (test_name, build_data.build_number)
+        os.environ["JENKINS_HOME"] = conf.JENKINS_HOME
+        os.environ["JENKINS_URL"] = conf.JENKINS_URL
+        os.environ["JOB_NAME"] = build_data.job_name
+        os.environ["NODE_NAME"] = build_data.board_name
+        os.environ["NODE_LABELS"] = build_data.board_name
+        os.environ["WORKSPACE"] = build_data.workspace
+
+    ## set Fuego-only environment variables
     os.environ["Reboot"] = build_data.reboot_flag
     os.environ["Rebuild"] = build_data.rebuild_flag
     os.environ["Target_PreCleanup"] = build_data.precleanup_flag
     os.environ["Target_PostCleanup"] = build_data.postcleanup_flag
+    os.environ["TESTSPEC"] = build_data.spec_name
+    os.environ["TESTDIR"] = build_data.testdir
+    os.environ["TESTPLAN"] = build_data.testplan_name
+    os.environ["Device"] = build_data.board_name
+    os.environ["FUEGO_HOST"] = conf.host
+    os.environ["LOGDIR"] = build_data.test_logdir
+    if debug is 1:
+        os.environ["FUEGO_DEBUG"] = "1"
+    for var_name in board.env_vars.keys():
+        os.environ[var_name] = board.env_vars[var_name]
+    for var in ["FUEGO_CORE", "FUEGO_RO", "FUEGO_RW"]:
+        os.environ[var] = conf.__dict__[var]
+    timestamp = time.strftime("%FT%T%z")
+    build_data.timestamp = timestamp
+    os.environ["BUILD_TIMESTAMP"] = build_data.timestamp
 
     # cd to buildzone directory
     saved_cur_dir = os.getcwd()
-- 
2.7.4

[Fuego] [PATCH 21/30] run-test: use console.txt as the log name

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 57976d8..28ccb68 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3393,6 +3393,7 @@ def do_run_test(conf, options):
     # stream output to log file while test is running
 
     # create log file
+    console_log_filename = "consolelog.txt"
     log_filename = run_dir + os.sep + console_log_filename
     log = open(log_filename, "w+")
     log.write("LOG HEADER from FTC")
-- 
2.7.4

[Fuego] [PATCH 22/30] run-test: run the command with and without debug

[Daniel Sangorrin]

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 28ccb68..21955a2 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3153,7 +3153,7 @@ def get_test_arg(cmd, conf, options):
     return (test_name, options)
 
 def do_run_test(conf, options):
-    global log, tail_fd
+    global log, tail_fd, debug
 
     board, options = get_board_arg("Run-test", conf, options)
     board_name = board.name
@@ -3369,29 +3369,6 @@ def do_run_test(conf, options):
     build_data.timestamp = timestamp
     os.environ["BUILD_TIMESTAMP"] = build_data.timestamp
 
-    # cd to buildzone directory
-    saved_cur_dir = os.getcwd()
-    os.chdir(build_data.workspace)
-
-    print("Running remotely on '%(board_name)s' in workspace %(workspace)s" % build_data)
-
-    # FIXTHIS - do_run_test - get timeout from testplan
-    # FIXTHIS - do_run_test - handle timeout myself in ftc_exec_command
-    command = "timeout --signal=9 30m /bin/bash $FUEGO_CORE/engine/scripts/main.sh"
-    pvar("command")
-
-    # write command to temp file, and execute that with
-    #    '/bin/sh -xe /tmp/hudson123456.sh'
-    try:
-        tempfilename = make_temp_file(command)
-    except:
-        os.chdir(saved_cur_dir)
-        error_out("Could not make temp file")
-
-    pvar("tempfilename")
-
-    # stream output to log file while test is running
-
     # create log file
     console_log_filename = "consolelog.txt"
     log_filename = run_dir + os.sep + console_log_filename
@@ -3405,7 +3382,14 @@ def do_run_test(conf, options):
     flag = fcntl.fcntl(tail_fd, fcntl.F_GETFL)
     fcntl.fcntl(tail_fd, fcntl.F_SETFL, flag | os.O_NONBLOCK)
 
-    command = "/bin/bash -xe %s" % (tempfilename)
+    # execute the test
+    print("Running remotely on '%(board_name)s' in workspace %(workspace)s" % build_data)
+    saved_cur_dir = os.getcwd()
+    os.chdir(build_data.workspace)
+    if debug is 1:
+        command = '/bin/bash -xe ' + conf.FUEGO_CORE + '/engine/scripts/main.sh'
+    else:
+        command = '/bin/bash -e ' + conf.FUEGO_CORE + '/engine/scripts/main.sh'
     rcode = ftc_exec_command(command, timeout)
     log.flush()
 
-- 
2.7.4

[Fuego] [PATCH 23/30] run-test: a few more fixes

[Daniel Sangorrin]

This fixes should have been merged with previous commits
sorry.

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 21955a2..7da4f27 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3397,6 +3397,7 @@ def do_run_test(conf, options):
     if rcode != 0:
         build_data.result = "FAILURE"
 
+    # FIXTHIS: this is deadcode
     comment_out = """
     # now, execute the test post_test
     # FIXTHIS - this is hard-coded
@@ -3454,7 +3455,7 @@ def do_run_test(conf, options):
     #run.write_run_json_file(log_dir)
 
     # update all the Jenkins build-related files
-    if job_dir:
+    if fuego_caller != "jenkins":
         write_build_xml_file(run_dir, build_data)
 
         # write changelog.xml file
@@ -3473,14 +3474,14 @@ def do_run_test(conf, options):
                 os.unlink(linkname)
             except:
                 pass
-            os.symlink(build_number, linkname)
+            os.symlink(build_data.build_number, linkname)
 
             linkname = job_dir + "/builds/lastSuccessfulBuild"
             try:
                 os.unlink(linkname)
             except:
                 pass
-            os.symlink(build_number, linkname)
+            os.symlink(build_data.build_number, linkname)
 
         if build_data.result == "FAILED":
             linkname = job_dir + "/builds/lastUnsuccessfulBuild"
@@ -3488,14 +3489,14 @@ def do_run_test(conf, options):
                 os.unlink(linkname)
             except:
                 pass
-            os.symlink(build_number, linkname)
+            os.symlink(build_data.build_number, linkname)
 
             linkname = job_dir + "/builds/lastFailedBuild"
             try:
                 os.unlink(linkname)
             except:
                 pass
-            os.symlink(build_number, linkname)
+            os.symlink(build_data.build_number, linkname)
 
         if build_data.result == "ABORTED":
             linkname = job_dir + "/builds/lastUnsuccessfulBuild"
@@ -3503,14 +3504,10 @@ def do_run_test(conf, options):
                 os.unlink(linkname)
             except:
                 pass
-            os.symlink(build_number, linkname)
+            os.symlink(build_data.build_number, linkname)
 
+    # FIXTHIS: if called as root, we need to become root again before moving here
     os.chdir(saved_cur_dir)
-
-    # remove the tempfiles
-    os.unlink(tempfilename)
-    #os.unlink(tempfilename2)
-
     log.close()
     log_tail.close()
 
-- 
2.7.4

[Fuego] [PATCH 24/30] run-test: support dynamic variables

[Daniel Sangorrin]

A spec is created and saved in the logdir folder. This is
good not only for dynamic variables but also for static specs
because we can easily package the logdir folder with all
the important data.

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index 7da4f27..bfdc3d4 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -3164,6 +3164,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]
@@ -3171,7 +3183,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')
     else:
@@ -3369,6 +3382,38 @@ def do_run_test(conf, options):
     build_data.timestamp = timestamp
     os.environ["BUILD_TIMESTAMP"] = build_data.timestamp
 
+    # create a folder for this run
+    run_dir = job_dir + "/builds/" + build_data.build_number
+    if not os.path.isdir(run_dir):
+        if fuego_caller == "jenkins":
+            error_out("Jenkins did not create run folder " + run_dir)
+        os.mkdir(run_dir)
+
+    # 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 != spec_name:
+            del test_spec_data['specs'][key]
+
+    if dyn_vars:
+        if spec_name not in test_spec_data['specs']:
+            test_spec_data['specs'][spec_name] = dyn_vars
+        else:
+            for key in dyn_vars.keys():
+                test_spec_data['specs'][spec_name][key] = dyn_vars[key]
+
+    # 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)
+
     # create log file
     console_log_filename = "consolelog.txt"
     log_filename = run_dir + os.sep + console_log_filename
-- 
2.7.4

[Fuego] [PATCH 25/30] ftc: put a fixthis for the documentation

[Daniel Sangorrin]

with argparse it would be automatic

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index bfdc3d4..d0b6a94 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -100,7 +100,8 @@ command_help = {
   list for the <board> argument. e.g.
      ftc add-jobs -b board1,board2 -t Functional.foo
 
-  This interface may change in the future."""),
+  This interface may change in the future.
+  FIXTHIS: add documentation about timeout,reboot,rebuild"""),
 
 "rm-jobs": ("Removes jobs from Jenkins.",
     """Usage: ftc rm-jobs [--remove-logs] <board>.<testspec>.<testtype>.<testcase>
@@ -239,6 +240,9 @@ would run the pre_test, pre_check, build and deploy phases of the test.
 This is useful during development of a test (ie. for testing tests).
 Use caution running later phases of a test without their normal
 precursors (e.g. run, without build or deploy).
+
+FIXTHIS: add documentation about timeout,reboot,rebuild,dynamic-vars
+
 """),
 
 "build-jobs": ("Build one or more jobs (to execute tests) in Jenkins.",
-- 
2.7.4

[Fuego] [PATCH 26/30] bang: replace the jenkins script

[Daniel Sangorrin]

This alone is a milestone!

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

diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index d0b6a94..12bedb3 100755
--- a/engine/scripts/ftc
+++ b/engine/scripts/ftc
@@ -1206,6 +1206,13 @@ def link_key(item):
 
 
 def create_job(board, test):
+    global debug
+
+    if debug == 1:
+        debug_param = "--debug"
+    else:
+        debug_param = ""
+
     flot_link = '<flotile.FlotPublisher plugin="flot@1.0-SNAPSHOT"/>'
 
     # prepare links for the descriptionsetter plugin
@@ -1291,14 +1298,8 @@ def create_job(board, test):
     <customWorkspace>$FUEGO_RW/buildzone</customWorkspace>
     <builders>
     <hudson.tasks.Shell>
-        <command>export Reboot={reboot}
-export Rebuild={rebuild}
-export Target_PreCleanup={precleanup}
-export Target_PostCleanup={postcleanup}
-export TESTDIR={testdir}
-export TESTSPEC={testspec}
-#export FUEGO_DEBUG=1
-timeout --signal=9 {timeout} /bin/bash $FUEGO_CORE/engine/scripts/main.sh
+        <command>export FUEGO_CALLER="jenkins"
+ftc run-test -b $NODE_NAME -t {testdir} -s {testspec} --timeout {timeout} --reboot {reboot} --rebuild {rebuild} --precleanup {precleanup} --postcleanup {postcleanup} {debug_param}
 </command>
     </hudson.tasks.Shell>
     </builders>
@@ -1317,7 +1318,8 @@ timeout --signal=9 {timeout} /bin/bash $FUEGO_CORE/engine/scripts/main.sh
 """.format(board=board, reboot=test.reboot, rebuild=test.rebuild,
         precleanup=test.precleanup, postcleanup=test.postcleanup,
         testdir=test.name, testspec=test.spec, timeout=test.timeout,
-        flot_link=flot_link, success_links=success_links, fail_links=fail_links))
+        flot_link=flot_link, success_links=success_links, fail_links=fail_links,
+        debug_param=debug_param))
     fd.close()
 
     job_name = board + "." + test.spec + "." + test.name
-- 
2.7.4

[Fuego] [PATCH 27/30] functions: logdir not required here

[Daniel Sangorrin]

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/scripts/functions.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
index 654a1ee..68e31e7 100755
--- a/engine/scripts/functions.sh
+++ b/engine/scripts/functions.sh
@@ -453,8 +453,6 @@ function pre_test {
     # Target cleanup flag check
     [ "$Target_PreCleanup" = "true" ] && target_cleanup $TESTDIR || true
 
-    export LOGDIR="$FUEGO_RW/logs/$TESTDIR/${NODE_NAME}.${TESTSPEC}.${BUILD_NUMBER}.${BUILD_ID}"
-
     # see if test dependencies (expressed by NEED_ vars) are met
     check_needs || abort_job "Test dependencies (expressed by NEED variables) not met"
 
-- 
2.7.4

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

[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@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}.${BUILD_NUMBER}.${BUILD_ID}"
-    mkdir -p $LOGDIR
-
-    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'
     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()
     run(test_args)
 
 run()
-- 
2.7.4

[Fuego] [PATCH 29/30] tarball: simplify extraction

[Daniel Sangorrin]

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/scripts/functions.sh | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
index 68e31e7..ad34ff5 100755
--- a/engine/scripts/functions.sh
+++ b/engine/scripts/functions.sh
@@ -87,13 +87,7 @@ function untar {
     is_empty "$tarball"
 
     echo "Unpacking $tarball"
-    case ${tarball/*./} in
-        gz|tgz) key=z ;;
-        bz2) key=j ;;
-        tar) key= ;;
-        *) echo "Unknown $tarball file format. Not unpacking."; return 1;;
-    esac
-    tar ${key}xf $TEST_HOME/$tarball --strip-components=1
+    tar xvf $TEST_HOME/$tarball --strip-components=1
 }
 
 # Unpacks/clones the test source code into the current directory.
-- 
2.7.4

[Fuego] [PATCH 30/30] LTP: short comment to show how to get the rt tests list

[Daniel Sangorrin]

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/tests/Functional.LTP/fuego_test.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/engine/tests/Functional.LTP/fuego_test.sh b/engine/tests/Functional.LTP/fuego_test.sh
index ecbf758..a378573 100755
--- a/engine/tests/Functional.LTP/fuego_test.sh
+++ b/engine/tests/Functional.LTP/fuego_test.sh
@@ -24,6 +24,7 @@ fs_ext4             ltp-aio-stress.part1  net_stress.broken_ip  pty"
 
 ALLPTSTESTS="AIO MEM MSG SEM SIG THR TMR TPS"
 
+# This list can be obtained by doing ./testscripts/test_realtime.sh -t list
 ALLRTTESTS="
 perf/latency
 func/measurement
-- 
2.7.4

Re: [Fuego] [PATCH 02/30] testplan: dbench4 does not have a testdir spec anymore

[Tim.Bird]

Looks good. Applied.

Thanks,
 -- Tim


> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> Sent: Monday, June 04, 2018 12:18 AM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 02/30] testplan: dbench4 does not have a testdir
> spec anymore
> 
> Use default instead
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/overlays/testplans/testplan_docker.json | 3 +--
>  engine/overlays/testplans/testplan_lava.json   | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/engine/overlays/testplans/testplan_docker.json
> b/engine/overlays/testplans/testplan_docker.json
> index 673a26d..f43a5ec 100644
> --- a/engine/overlays/testplans/testplan_docker.json
> +++ b/engine/overlays/testplans/testplan_docker.json
> @@ -8,8 +8,7 @@
>              "spec": "100M"
>          },
>          {
> -            "testName": "Benchmark.dbench4",
> -            "spec": "testdir"
> +            "testName": "Benchmark.dbench4"
>          },
>          {
>              "testName": "Benchmark.hackbench"
> diff --git a/engine/overlays/testplans/testplan_lava.json
> b/engine/overlays/testplans/testplan_lava.json
> index dd63784..02a6abc 100644
> --- a/engine/overlays/testplans/testplan_lava.json
> +++ b/engine/overlays/testplans/testplan_lava.json
> @@ -8,8 +8,7 @@
>              "spec": "100M"
>          },
>          {
> -            "testName": "Benchmark.dbench4",
> -            "spec": "testdir"
> +            "testName": "Benchmark.dbench4"
>          },
>          {
>              "testName": "Benchmark.hackbench"
> --
> 2.7.4
> 
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH 03/30] reboot: reboot the board if reboot flag is set to true

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/functions.sh | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
> index 7d1cdac..654a1ee 100755
> --- a/engine/scripts/functions.sh
> +++ b/engine/scripts/functions.sh
> @@ -413,6 +413,10 @@ function target_setup_route_to_host () {
>  }
> 
>  function pre_test {
> +    if [ "$Reboot" == "true" ]; then
> +        target_reboot ${MAX_REBOOT_RETRIES:-20}
> +    fi
> +
>      # Make sure the target is alive, and prepare workspace for the test
>      source $FUEGO_RO/toolchains/tools.sh
>      export SSHPASS=$PASSWORD
> --
> 2.7.4

Hmmm.  I can't find any other uses of this variable.  Maybe
in the distant past it got refactored out.  But it's nice to add
back in functionality that is supposed to be there.  Though
a little embarrassing that we haven't noticed until now.
Maybe we don't use this very often.

Thanks!  Applied.
 -- Tim

Re: [Fuego] [PATCH 13/30] ftc exec: convert timeout to time units

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin
> Sent: Monday, June 04, 2018 12:18 AM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 13/30] ftc exec: convert timeout to time units
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ftc | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index 7a9d23c..fc900ed 100755
> --- a/engine/scripts/ftc
> +++ b/engine/scripts/ftc
> @@ -2966,7 +2966,14 @@ def ftc_exec_command(command, timeout):
> 
>      # specify timeout for command operation
>      signal.signal(signal.SIGALRM, alarm_handler)
> -    signal.alarm(timeout)
> +
> +    # timeout is passed as integer[dhms] (dhms: days, hours, minutes,
> seconds)
> +    print "timeout is: " + timeout
> +    units = timeout[:-1]
> +    multiplier = {"d": 24*60*60, "h": 60*60, "m": 60, "s": 1}
> +    time = int(units)*multiplier[timeout[-1]]
We have a global 'time' module defined, which this
name conflicts with.  I'm changing this to 'time_secs',
unless you have some objection.

> +    print "setting alarm to: " + str(time) + " seconds"
> +    signal.alarm(time)
> 
>      try:
>          # p.poll returns exit code when process completes
> --
> 2.7.4

Thanks.  BTW - everything up to this is applied.  I'm working
through the list. :-)
 -- Tim

Re: [Fuego] [PATCH 24/30] run-test: support dynamic variables

[Tim.Bird]

OK - I got to here in my code review.  I've applied previous
patches up to this point (1 through 23), with a few issues
I tagged to come back and check on.

I'll discuss those issues in a separate e-mail tomorrow.
My brain is kind of full tonight, so I don't trust myself to
finish this without some additional thought.  One issue
with this patch is mentioned below.

I'll try to finish off the series and do some testing tomorrow.
 -- Tim


> -----Original Message-----
> From: Daniel Sangorrin
> 
> A spec is created and saved in the logdir folder. This is
> good not only for dynamic variables but also for static specs
> because we can easily package the logdir folder with all
> the important data.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ftc | 47
> ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index 7da4f27..bfdc3d4 100755
> --- a/engine/scripts/ftc
> +++ b/engine/scripts/ftc
> @@ -3164,6 +3164,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]
> @@ -3171,7 +3183,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')
>      else:
> @@ -3369,6 +3382,38 @@ def do_run_test(conf, options):
>      build_data.timestamp = timestamp
>      os.environ["BUILD_TIMESTAMP"] = build_data.timestamp
> 
> +    # create a folder for this run
> +    run_dir = job_dir + "/builds/" + build_data.build_number
> +    if not os.path.isdir(run_dir):
> +        if fuego_caller == "jenkins":
> +            error_out("Jenkins did not create run folder " + run_dir)
> +        os.mkdir(run_dir)
> +
> +    # prepare a per-run spec.json file
> +    specpath = '%s/engine/tests/%s/spec.json' % (conf.FUEGO_CORE,
> test_name)

I had also intended to support per-board spec files (like we do with
per-board criteria files).  But that can be added later.  With dynamic
variables, the need for this is significantly reduced.

> +    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 != spec_name:
> +            del test_spec_data['specs'][key]
> +
> +    if dyn_vars:
> +        if spec_name not in test_spec_data['specs']:
> +            test_spec_data['specs'][spec_name] = dyn_vars
> +        else:
> +            for key in dyn_vars.keys():
> +                test_spec_data['specs'][spec_name][key] = dyn_vars[key]
> +
> +    # 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)
> +
>      # create log file
>      console_log_filename = "consolelog.txt"
>      log_filename = run_dir + os.sep + console_log_filename
> --
> 2.7.4
> 
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH 29/30] tarball: simplify extraction

[Tim.Bird]

OK I lied.  I'm still perusing the patches.

Below are some questions about this one.

> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> Sent: Monday, June 04, 2018 12:18 AM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 29/30] tarball: simplify extraction
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/functions.sh | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
> index 68e31e7..ad34ff5 100755
> --- a/engine/scripts/functions.sh
> +++ b/engine/scripts/functions.sh
> @@ -87,13 +87,7 @@ function untar {
>      is_empty "$tarball"
> 
>      echo "Unpacking $tarball"
> -    case ${tarball/*./} in
> -        gz|tgz) key=z ;;
> -        bz2) key=j ;;
> -        tar) key= ;;
> -        *) echo "Unknown $tarball file format. Not unpacking."; return 1;;
> -    esac
> -    tar ${key}xf $TEST_HOME/$tarball --strip-components=1
> +    tar xvf $TEST_HOME/$tarball --strip-components=1

Are z and j not needed?  Does tar autodetect this now?

Separately - do we really want to make the extraction verbose (v option)?
It hasn't been before, and some of these tarballs are big, so the logs will
be a bit chatty.  Maybe the 'v' should be tied to a debug flag?

Just asking...

 -- Tim

>  }
> 
>  # Unpacks/clones the test source code into the current directory.
> --
> 2.7.4
> 
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH 29/30] tarball: simplify extraction

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> Sent: Wednesday, June 6, 2018 12:03 PM
> To: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 29/30] tarball: simplify extraction
> 
> OK I lied.  I'm still perusing the patches.
> 
> Below are some questions about this one.
> 
> > -----Original Message-----
> > From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> > bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> > Sent: Monday, June 04, 2018 12:18 AM
> > To: fuego@lists.linuxfoundation.org
> > Subject: [Fuego] [PATCH 29/30] tarball: simplify extraction
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> >  engine/scripts/functions.sh | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
> > index 68e31e7..ad34ff5 100755
> > --- a/engine/scripts/functions.sh
> > +++ b/engine/scripts/functions.sh
> > @@ -87,13 +87,7 @@ function untar {
> >      is_empty "$tarball"
> >
> >      echo "Unpacking $tarball"
> > -    case ${tarball/*./} in
> > -        gz|tgz) key=z ;;
> > -        bz2) key=j ;;
> > -        tar) key= ;;
> > -        *) echo "Unknown $tarball file format. Not unpacking."; return 1;;
> > -    esac
> > -    tar ${key}xf $TEST_HOME/$tarball --strip-components=1
> > +    tar xvf $TEST_HOME/$tarball --strip-components=1
> 
> Are z and j not needed?  Does tar autodetect this now?
> 

Yes, for quite a while I think. Also tar.xz is autodetected.

> Separately - do we really want to make the extraction verbose (v option)?
> It hasn't been before, and some of these tarballs are big, so the logs will
> be a bit chatty.  Maybe the 'v' should be tied to a debug flag?

Good catch, please remove the v.

Thanks,
Daniel

> 
> Just asking...
> 
>  -- Tim
> 
> >  }
> >
> >  # Unpacks/clones the test source code into the current directory.
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH 24/30] run-test: support dynamic variables

[Tim.Bird]

A few comments inline below...

> -----Original Message-----
> From: Daniel Sangorrin
> 
> A spec is created and saved in the logdir folder. This is
> good not only for dynamic variables but also for static specs
> because we can easily package the logdir folder with all
> the important data.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ftc | 47
> ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index 7da4f27..bfdc3d4 100755
> --- a/engine/scripts/ftc
> +++ b/engine/scripts/ftc
> @@ -3164,6 +3164,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)
Interesting.  This means that the argument for --dynamic-vars
is in python syntax, not JSON syntax.  Off the top of my head,
I can't think of any differences that will cause us problems in
our common Fuego use cases.
But python ast does allow '\n' and other character escapes, that
JSON does not.  That this is python syntax is something that needs
to be documented, IMHO.

Just a note - using an existing syntax beats us creating our own new
one with its own set of potential issues.  So, good job!  I wouldn't
have thought to do it this way.   This is nice and compact for parsing
potentially multiple vars in a single shot.

> +        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]
> @@ -3171,7 +3183,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')
>      else:
> @@ -3369,6 +3382,38 @@ def do_run_test(conf, options):
>      build_data.timestamp = timestamp
>      os.environ["BUILD_TIMESTAMP"] = build_data.timestamp
> 
> +    # create a folder for this run
> +    run_dir = job_dir + "/builds/" + build_data.build_number
> +    if not os.path.isdir(run_dir):
> +        if fuego_caller == "jenkins":
> +            error_out("Jenkins did not create run folder " + run_dir)
> +        os.mkdir(run_dir)
> +
> +    # 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 != spec_name:
> +            del test_spec_data['specs'][key]
> +
> +    if dyn_vars:
> +        if spec_name not in test_spec_data['specs']:
> +            test_spec_data['specs'][spec_name] = dyn_vars
> +        else:
> +            for key in dyn_vars.keys():
> +                test_spec_data['specs'][spec_name][key] = dyn_vars[key]
> +
> +    # 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)

OK - one issue with this approach is that it could be misleading.
If we are using a 'default' spec, and applying some dynamic variables,
then the spec.json in the logdir will have a spec name of 'default', but with
variable values that are NOT those of the default spec (from the original spec.json).

I think it would be good to somehow indicate any variables that
have dynamic (changed) values, from those in the original spec.
I'm not sure if it would be better to add an annotation per variable,
or to add a list of names of changed variables.  But it would be good
to capture that, IMHO, to avoid confusion.  Maybe even just adding
a single attribute indicating "some variables were changed" would
be good.  That way a user would know that any difference in test
results (from some other run using the same spec name) might be due
to changes in the variables.

BTW - is this spec (the one in logdir) the one used by the overlay
generator during the actual test run?  Or is it just recording the
values for review after the test?

> +
>      # create log file
>      console_log_filename = "consolelog.txt"
>      log_filename = run_dir + os.sep + console_log_filename
> --
> 2.7.4

I'm applying this, but will do some testing before I push it out to
the master branch.

One other note.  The dynamic variables feature is nice for people
doing manual testing and wanting to change a single variable for
some purpose.  But if people end up using this in a scripted way,
that just puts the variable definitions into the script, rather than
into a proper spec.json file, which defeats the goal of sharing
useful test variable values and combinations with other Fuego users.
The proper way to codify a permanent change in the test variables
is to put them into spec files.  That's what spec files exist for.  And
if a test variable is being determined at runtime based on some
conditions at the time of the test, that is something that really
should be pushed into the test itself.

I'm not opposed to the feature - just noting that it could have
detrimental side effects if misused.
 -- Tim

Re: [Fuego] [PATCH 25/30] ftc: put a fixthis for the documentation

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin
> 
> with argparse it would be automatic
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ftc | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index bfdc3d4..d0b6a94 100755
> --- a/engine/scripts/ftc
> +++ b/engine/scripts/ftc
> @@ -100,7 +100,8 @@ command_help = {
>    list for the <board> argument. e.g.
>       ftc add-jobs -b board1,board2 -t Functional.foo
> 
> -  This interface may change in the future."""),
> +  This interface may change in the future.
> +  FIXTHIS: add documentation about timeout,reboot,rebuild"""),

OK - I don't like this very much.  This is not a normal FIXTHIS, in
that it's visible to end users.  Most FIXTHIS comments are
comments inside the code.  This one is, IMHO, very high priority.
Other FIXTHIS items range from low priority feature suggestions,
to "should check this some time" items.

User's shouldn't see notes about the state of the code when they
are looking for help.  

Can you work on this soon?  I don't want this one to go out in a
release.  I'm considering re-wording it "MUSTFIX:", and having a
regression check for such terms in my release test.

Let me know what you think.
  -- Tim

> 
>  "rm-jobs": ("Removes jobs from Jenkins.",
>      """Usage: ftc rm-jobs [--remove-logs]
> <board>.<testspec>.<testtype>.<testcase>
> @@ -239,6 +240,9 @@ would run the pre_test, pre_check, build and deploy
> phases of the test.
>  This is useful during development of a test (ie. for testing tests).
>  Use caution running later phases of a test without their normal
>  precursors (e.g. run, without build or deploy).
> +
> +FIXTHIS: add documentation about timeout,reboot,rebuild,dynamic-vars
Same issue as above.

> +
>  """),
> 
>  "build-jobs": ("Build one or more jobs (to execute tests) in Jenkins.",
> --
> 2.7.4


Applied - but changed to MUSTFIX.
 -- Tim

Re: [Fuego] [PATCH 26/30] bang: replace the jenkins script

[Tim.Bird]

Comments inline below...

> -----Original Message-----
> From: Daniel Sangorrin
> This alone is a milestone!
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/ftc | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index d0b6a94..12bedb3 100755
> --- a/engine/scripts/ftc
> +++ b/engine/scripts/ftc
> @@ -1206,6 +1206,13 @@ def link_key(item):
> 
> 
>  def create_job(board, test):
> +    global debug
> +
> +    if debug == 1:
> +        debug_param = "--debug"
> +    else:
> +        debug_param = ""
> +
This merging of the environment variable used by the Fuego
core scripts, and the 'debug' variable used by ftc, results in
a loss of functionality.  The FUEGO_DEBUG variable supports
multiple bit-wise flags, to debug different parts of the
run.  The 'ftc' debug variable did not.
See http://fuegotest.org/wiki/FUEGO_DEBUG

What probably needs to happen is to have '--debug' take an argument,
for the debug value.  I'm applying this patch, because the more important
aspect of this is the conversion from calling main.sh to ftc from the
Jenkins job.  But one of us should circle back and fix the debug flag
value handling.

>      flot_link = '<flotile.FlotPublisher plugin="flot@1.0-SNAPSHOT"/>'
> 
>      # prepare links for the descriptionsetter plugin
> @@ -1291,14 +1298,8 @@ def create_job(board, test):
>      <customWorkspace>$FUEGO_RW/buildzone</customWorkspace>
>      <builders>
>      <hudson.tasks.Shell>
> -        <command>export Reboot={reboot}
> -export Rebuild={rebuild}
> -export Target_PreCleanup={precleanup}
> -export Target_PostCleanup={postcleanup}
> -export TESTDIR={testdir}
> -export TESTSPEC={testspec}
> -#export FUEGO_DEBUG=1
> -timeout --signal=9 {timeout} /bin/bash
> $FUEGO_CORE/engine/scripts/main.sh
> +        <command>export FUEGO_CALLER="jenkins"
> +ftc run-test -b $NODE_NAME -t {testdir} -s {testspec} --timeout {timeout} --
> reboot {reboot} --rebuild {rebuild} --precleanup {precleanup} --postcleanup
> {postcleanup} {debug_param}
>  </command>
>      </hudson.tasks.Shell>
>      </builders>
> @@ -1317,7 +1318,8 @@ timeout --signal=9 {timeout} /bin/bash
> $FUEGO_CORE/engine/scripts/main.sh
>  """.format(board=board, reboot=test.reboot, rebuild=test.rebuild,
>          precleanup=test.precleanup, postcleanup=test.postcleanup,
>          testdir=test.name, testspec=test.spec, timeout=test.timeout,
> -        flot_link=flot_link, success_links=success_links, fail_links=fail_links))
> +        flot_link=flot_link, success_links=success_links, fail_links=fail_links,
> +        debug_param=debug_param))
>      fd.close()
> 
>      job_name = board + "." + test.spec + "." + test.name
> --
> 2.7.4


Applied, but not pushed yet.
 -- Tim

Re: [Fuego] [PATCH 27/30] functions: logdir not required here

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/functions.sh | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
> index 654a1ee..68e31e7 100755
> --- a/engine/scripts/functions.sh
> +++ b/engine/scripts/functions.sh
> @@ -453,8 +453,6 @@ function pre_test {
>      # Target cleanup flag check
>      [ "$Target_PreCleanup" = "true" ] && target_cleanup $TESTDIR || true
> 
> -    export
> LOGDIR="$FUEGO_RW/logs/$TESTDIR/${NODE_NAME}.${TESTSPEC}.${BUIL
> D_NUMBER}.${BUILD_ID}"
> -
>      # see if test dependencies (expressed by NEED_ vars) are met
>      check_needs || abort_job "Test dependencies (expressed by NEED
> variables) not met"
> 
> --
> 2.7.4

NAK on this one.  I still have plenty of jobs that call main.sh, without going
through ftc first.  To be backwards-compatible with those, I'd like to leave
this definition here.  I know it's redundant with the definition made by
ftc.

Maybe we should have a FIXTHIS here, reminding us to remove this when
we're willing to pull the plug on running pre-1.4 Fuego jobs.
 -- Tim

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

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

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

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.

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?

>      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

Re: [Fuego] [PATCH 29/30] tarball: simplify extraction

[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: Monday, June 04, 2018 12:18 AM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 29/30] tarball: simplify extraction
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/functions.sh | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
> index 68e31e7..ad34ff5 100755
> --- a/engine/scripts/functions.sh
> +++ b/engine/scripts/functions.sh
> @@ -87,13 +87,7 @@ function untar {
>      is_empty "$tarball"
> 
>      echo "Unpacking $tarball"
> -    case ${tarball/*./} in
> -        gz|tgz) key=z ;;
> -        bz2) key=j ;;
> -        tar) key= ;;
> -        *) echo "Unknown $tarball file format. Not unpacking."; return 1;;
> -    esac
> -    tar ${key}xf $TEST_HOME/$tarball --strip-components=1
> +    tar xvf $TEST_HOME/$tarball --strip-components=1
>  }
> 
>  # Unpacks/clones the test source code into the current directory.
> --
> 2.7.4
> 
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH 25/30] ftc: put a fixthis for the documentation

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> Sent: Thursday, June 7, 2018 5:30 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 25/30] ftc: put a fixthis for the documentation
> 
> 
> 
> > -----Original Message-----
> > From: Daniel Sangorrin
> >
> > with argparse it would be automatic
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> >  engine/scripts/ftc | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> > index bfdc3d4..d0b6a94 100755
> > --- a/engine/scripts/ftc
> > +++ b/engine/scripts/ftc
> > @@ -100,7 +100,8 @@ command_help = {
> >    list for the <board> argument. e.g.
> >       ftc add-jobs -b board1,board2 -t Functional.foo
> >
> > -  This interface may change in the future."""),
> > +  This interface may change in the future.
> > +  FIXTHIS: add documentation about timeout,reboot,rebuild"""),
> 
> OK - I don't like this very much.  This is not a normal FIXTHIS, in
> that it's visible to end users.  Most FIXTHIS comments are
> comments inside the code.  This one is, IMHO, very high priority.
> Other FIXTHIS items range from low priority feature suggestions,
> to "should check this some time" items.
> 
> User's shouldn't see notes about the state of the code when they
> are looking for help.
> 
> Can you work on this soon?  I don't want this one to go out in a
> release.  I'm considering re-wording it "MUSTFIX:", and having a
> regression check for such terms in my release test.
> 
> Let me know what you think.

Sorry, I'll fix this soon.
Daniel

>   -- Tim
> 
> >
> >  "rm-jobs": ("Removes jobs from Jenkins.",
> >      """Usage: ftc rm-jobs [--remove-logs]
> > <board>.<testspec>.<testtype>.<testcase>
> > @@ -239,6 +240,9 @@ would run the pre_test, pre_check, build and deploy
> > phases of the test.
> >  This is useful during development of a test (ie. for testing tests).
> >  Use caution running later phases of a test without their normal
> >  precursors (e.g. run, without build or deploy).
> > +
> > +FIXTHIS: add documentation about timeout,reboot,rebuild,dynamic-vars
> Same issue as above.
> 
> > +
> >  """),
> >
> >  "build-jobs": ("Build one or more jobs (to execute tests) in Jenkins.",
> > --
> > 2.7.4
> 
> 
> Applied - but changed to MUSTFIX.
>  -- Tim
> 

Re: [Fuego] [PATCH 24/30] run-test: support dynamic variables

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> Sent: Thursday, June 7, 2018 4:56 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 24/30] run-test: support dynamic variables
> 
> A few comments inline below...
> 
> > -----Original Message-----
> > From: Daniel Sangorrin
> >
> > A spec is created and saved in the logdir folder. This is
> > good not only for dynamic variables but also for static specs
> > because we can easily package the logdir folder with all
> > the important data.
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> >  engine/scripts/ftc | 47
> > ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> > index 7da4f27..bfdc3d4 100755
> > --- a/engine/scripts/ftc
> > +++ b/engine/scripts/ftc
> > @@ -3164,6 +3164,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)
> Interesting.  This means that the argument for --dynamic-vars
> is in python syntax, not JSON syntax.  Off the top of my head,
> I can't think of any differences that will cause us problems in
> our common Fuego use cases.
> But python ast does allow '\n' and other character escapes, that
> JSON does not.  That this is python syntax is something that needs
> to be documented, IMHO.

I thought about using JSON, but specs are basically a set of key-values so I thought that using a python dictionary would be good enoguh.

> 
> Just a note - using an existing syntax beats us creating our own new
> one with its own set of potential issues.  So, good job!  I wouldn't
> have thought to do it this way.   This is nice and compact for parsing
> potentially multiple vars in a single shot.
> 
> > +        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]
> > @@ -3171,7 +3183,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')
> >      else:
> > @@ -3369,6 +3382,38 @@ def do_run_test(conf, options):
> >      build_data.timestamp = timestamp
> >      os.environ["BUILD_TIMESTAMP"] = build_data.timestamp
> >
> > +    # create a folder for this run
> > +    run_dir = job_dir + "/builds/" + build_data.build_number
> > +    if not os.path.isdir(run_dir):
> > +        if fuego_caller == "jenkins":
> > +            error_out("Jenkins did not create run folder " + run_dir)
> > +        os.mkdir(run_dir)
> > +
> > +    # 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 != spec_name:
> > +            del test_spec_data['specs'][key]
> > +
> > +    if dyn_vars:
> > +        if spec_name not in test_spec_data['specs']:
> > +            test_spec_data['specs'][spec_name] = dyn_vars
> > +        else:
> > +            for key in dyn_vars.keys():
> > +                test_spec_data['specs'][spec_name][key] = dyn_vars[key]
> > +
> > +    # 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)
> 
> OK - one issue with this approach is that it could be misleading.
> If we are using a 'default' spec, and applying some dynamic variables,
> then the spec.json in the logdir will have a spec name of 'default', but with
> variable values that are NOT those of the default spec (from the original spec.json).
> 
> I think it would be good to somehow indicate any variables that
> have dynamic (changed) values, from those in the original spec.
> I'm not sure if it would be better to add an annotation per variable,
> or to add a list of names of changed variables.  But it would be good
> to capture that, IMHO, to avoid confusion.  Maybe even just adding
> a single attribute indicating "some variables were changed" would
> be good.  That way a user would know that any difference in test
> results (from some other run using the same spec name) might be due
> to changes in the variables.

OK, I have added a new key ("dyn_vars") to specs generated from dynamic variables. It contains an array with the variables that have been modified.
 
> BTW - is this spec (the one in logdir) the one used by the overlay
> generator during the actual test run?  Or is it just recording the
> values for review after the test?

It is used by the overlay generator, not just recording. The idea is that we want to be able to reproduce a test from the contents of the log directory.

> > +
> >      # create log file
> >      console_log_filename = "consolelog.txt"
> >      log_filename = run_dir + os.sep + console_log_filename
> > --
> > 2.7.4
> 
> I'm applying this, but will do some testing before I push it out to
> the master branch.

OK. I will wait until they are pushed to add my fixes.

> One other note.  The dynamic variables feature is nice for people
> doing manual testing and wanting to change a single variable for
> some purpose.  

Just to clarify: not necessarily a single variable, you can change any variable you want or create a whole new spec.
And the purpose is to test for new bugs. In other words, using the same parameters all the time is for "regression" bugs, but it's not going to find new bugs. Being able to randomize the parameters allows for fuzzy testing (I plan to add scripts for fuzzy testing).

> But if people end up using this in a scripted way,
> that just puts the variable definitions into the script, rather than
> into a proper spec.json file, which defeats the goal of sharing
> useful test variable values and combinations with other Fuego users.

When results are shared we will publish the artifacts from the log directory. That includes the spec.json that was used for the test. Once it's on our cloud app, any user can download that spec from the dashboard and use it.
Developers can then check what specs are interesting and include them
in Fuego as board-dependent specs.

By the way, I am thinking about the command line options for publishing results. Currently we have

ftc put-run run_id | file.frp

I was thinking that we may need to be compatible with different web apps (e.g. fuego custom dashboard, squad, kernelci, others..). So perhaps something like this would be better:

ftc put-run -r run_id -w squad --params {squadparams} 

> The proper way to codify a permanent change in the test variables
> is to put them into spec files.  That's what spec files exist for.  And
> if a test variable is being determined at runtime based on some
> conditions at the time of the test, that is something that really
> should be pushed into the test itself.
> 
> I'm not opposed to the feature - just noting that it could have
> detrimental side effects if misused.
>  -- Tim
> 

Thanks,
Daniel

Re: [Fuego] [PATCH 25/30] ftc: put a fixthis for the documentation

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> > +  This interface may change in the future.
> > +  FIXTHIS: add documentation about timeout,reboot,rebuild"""),
> 
> OK - I don't like this very much.  This is not a normal FIXTHIS, in
> that it's visible to end users.  Most FIXTHIS comments are
> comments inside the code.  This one is, IMHO, very high priority.
> Other FIXTHIS items range from low priority feature suggestions,
> to "should check this some time" items.
> 
> User's shouldn't see notes about the state of the code when they
> are looking for help.
> 
> Can you work on this soon?  I don't want this one to go out in a
> release.  I'm considering re-wording it "MUSTFIX:", and having a
> regression check for such terms in my release test.
> 
> Let me know what you think.

OK, I fixed this. I will send the patches on top of your tree once you have merged them.

Thanks,
Daniel

> >  "rm-jobs": ("Removes jobs from Jenkins.",
> >      """Usage: ftc rm-jobs [--remove-logs]
> > <board>.<testspec>.<testtype>.<testcase>
> > @@ -239,6 +240,9 @@ would run the pre_test, pre_check, build and deploy
> > phases of the test.
> >  This is useful during development of a test (ie. for testing tests).
> >  Use caution running later phases of a test without their normal
> >  precursors (e.g. run, without build or deploy).
> > +
> > +FIXTHIS: add documentation about timeout,reboot,rebuild,dynamic-vars
> Same issue as above.
> 
> > +
> >  """),
> >
> >  "build-jobs": ("Build one or more jobs (to execute tests) in Jenkins.",
> > --
> > 2.7.4
> 
> 
> Applied - but changed to MUSTFIX.
>  -- Tim
> 

Re: [Fuego] [PATCH 26/30] bang: replace the jenkins script

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> Sent: Thursday, June 7, 2018 8:00 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 26/30] bang: replace the jenkins script
> 
> Comments inline below...
> 
> > -----Original Message-----
> > From: Daniel Sangorrin
> > This alone is a milestone!
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> >  engine/scripts/ftc | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> > index d0b6a94..12bedb3 100755
> > --- a/engine/scripts/ftc
> > +++ b/engine/scripts/ftc
> > @@ -1206,6 +1206,13 @@ def link_key(item):
> >
> >
> >  def create_job(board, test):
> > +    global debug
> > +
> > +    if debug == 1:
> > +        debug_param = "--debug"
> > +    else:
> > +        debug_param = ""
> > +
> This merging of the environment variable used by the Fuego
> core scripts, and the 'debug' variable used by ftc, results in
> a loss of functionality.  The FUEGO_DEBUG variable supports
> multiple bit-wise flags, to debug different parts of the
> run.  The 'ftc' debug variable did not.
> See http://fuegotest.org/wiki/FUEGO_DEBUG
> 
> What probably needs to happen is to have '--debug' take an argument,
> for the debug value.  I'm applying this patch, because the more important
> aspect of this is the conversion from calling main.sh to ftc from the
> Jenkins job.  But one of us should circle back and fix the debug flag
> value handling.

OK, I will fix this soon.
Thanks



> 
> >      flot_link = '<flotile.FlotPublisher plugin="flot@1.0-SNAPSHOT"/>'
> >
> >      # prepare links for the descriptionsetter plugin
> > @@ -1291,14 +1298,8 @@ def create_job(board, test):
> >      <customWorkspace>$FUEGO_RW/buildzone</customWorkspace>
> >      <builders>
> >      <hudson.tasks.Shell>
> > -        <command>export Reboot={reboot}
> > -export Rebuild={rebuild}
> > -export Target_PreCleanup={precleanup}
> > -export Target_PostCleanup={postcleanup}
> > -export TESTDIR={testdir}
> > -export TESTSPEC={testspec}
> > -#export FUEGO_DEBUG=1
> > -timeout --signal=9 {timeout} /bin/bash
> > $FUEGO_CORE/engine/scripts/main.sh
> > +        <command>export FUEGO_CALLER="jenkins"
> > +ftc run-test -b $NODE_NAME -t {testdir} -s {testspec} --timeout {timeout} --
> > reboot {reboot} --rebuild {rebuild} --precleanup {precleanup} --postcleanup
> > {postcleanup} {debug_param}
> >  </command>
> >      </hudson.tasks.Shell>
> >      </builders>
> > @@ -1317,7 +1318,8 @@ timeout --signal=9 {timeout} /bin/bash
> > $FUEGO_CORE/engine/scripts/main.sh
> >  """.format(board=board, reboot=test.reboot, rebuild=test.rebuild,
> >          precleanup=test.precleanup, postcleanup=test.postcleanup,
> >          testdir=test.name, testspec=test.spec, timeout=test.timeout,
> > -        flot_link=flot_link, success_links=success_links, fail_links=fail_links))
> > +        flot_link=flot_link, success_links=success_links, fail_links=fail_links,
> > +        debug_param=debug_param))
> >      fd.close()
> >
> >      job_name = board + "." + test.spec + "." + test.name
> > --
> > 2.7.4
> 
> 
> Applied, but not pushed yet.
>  -- Tim
> 

Re: [Fuego] [PATCH 26/30] bang: replace the jenkins script

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> Sent: Thursday, June 7, 2018 8:00 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 26/30] bang: replace the jenkins script
> 
> Comments inline below...
> 
> > -----Original Message-----
> > From: Daniel Sangorrin
> > This alone is a milestone!
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> >  engine/scripts/ftc | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> > index d0b6a94..12bedb3 100755
> > --- a/engine/scripts/ftc
> > +++ b/engine/scripts/ftc
> > @@ -1206,6 +1206,13 @@ def link_key(item):
> >
> >
> >  def create_job(board, test):
> > +    global debug
> > +
> > +    if debug == 1:
> > +        debug_param = "--debug"
> > +    else:
> > +        debug_param = ""
> > +
> This merging of the environment variable used by the Fuego
> core scripts, and the 'debug' variable used by ftc, results in
> a loss of functionality.  The FUEGO_DEBUG variable supports
> multiple bit-wise flags, to debug different parts of the
> run.  The 'ftc' debug variable did not.
> See http://fuegotest.org/wiki/FUEGO_DEBUG
> 
> What probably needs to happen is to have '--debug' take an argument,
> for the debug value.  I'm applying this patch, because the more important
> aspect of this is the conversion from calling main.sh to ftc from the
> Jenkins job.  But one of us should circle back and fix the debug flag
> value handling.

OK, I fixed it. I will send the patches on top of the previous ones once you merge them. Is that OK?

Thanks,
Daniel


> 
> >      flot_link = '<flotile.FlotPublisher plugin="flot@1.0-SNAPSHOT"/>'
> >
> >      # prepare links for the descriptionsetter plugin
> > @@ -1291,14 +1298,8 @@ def create_job(board, test):
> >      <customWorkspace>$FUEGO_RW/buildzone</customWorkspace>
> >      <builders>
> >      <hudson.tasks.Shell>
> > -        <command>export Reboot={reboot}
> > -export Rebuild={rebuild}
> > -export Target_PreCleanup={precleanup}
> > -export Target_PostCleanup={postcleanup}
> > -export TESTDIR={testdir}
> > -export TESTSPEC={testspec}
> > -#export FUEGO_DEBUG=1
> > -timeout --signal=9 {timeout} /bin/bash
> > $FUEGO_CORE/engine/scripts/main.sh
> > +        <command>export FUEGO_CALLER="jenkins"
> > +ftc run-test -b $NODE_NAME -t {testdir} -s {testspec} --timeout {timeout} --
> > reboot {reboot} --rebuild {rebuild} --precleanup {precleanup} --postcleanup
> > {postcleanup} {debug_param}
> >  </command>
> >      </hudson.tasks.Shell>
> >      </builders>
> > @@ -1317,7 +1318,8 @@ timeout --signal=9 {timeout} /bin/bash
> > $FUEGO_CORE/engine/scripts/main.sh
> >  """.format(board=board, reboot=test.reboot, rebuild=test.rebuild,
> >          precleanup=test.precleanup, postcleanup=test.postcleanup,
> >          testdir=test.name, testspec=test.spec, timeout=test.timeout,
> > -        flot_link=flot_link, success_links=success_links, fail_links=fail_links))
> > +        flot_link=flot_link, success_links=success_links, fail_links=fail_links,
> > +        debug_param=debug_param))
> >      fd.close()
> >
> >      job_name = board + "." + test.spec + "." + test.name
> > --
> > 2.7.4
> 
> 
> Applied, but not pushed yet.
>  -- Tim
> 

Re: [Fuego] [PATCH 27/30] functions: logdir not required here

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> Sent: Thursday, June 7, 2018 8:09 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 27/30] functions: logdir not required here
> 
> 
> 
> > -----Original Message-----
> > From: Daniel Sangorrin
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> >  engine/scripts/functions.sh | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/engine/scripts/functions.sh b/engine/scripts/functions.sh
> > index 654a1ee..68e31e7 100755
> > --- a/engine/scripts/functions.sh
> > +++ b/engine/scripts/functions.sh
> > @@ -453,8 +453,6 @@ function pre_test {
> >      # Target cleanup flag check
> >      [ "$Target_PreCleanup" = "true" ] && target_cleanup $TESTDIR || true
> >
> > -    export
> > LOGDIR="$FUEGO_RW/logs/$TESTDIR/${NODE_NAME}.${TESTSPEC}.${BUIL
> > D_NUMBER}.${BUILD_ID}"
> > -
> >      # see if test dependencies (expressed by NEED_ vars) are met
> >      check_needs || abort_job "Test dependencies (expressed by NEED
> > variables) not met"
> >
> > --
> > 2.7.4
> 
> NAK on this one.  I still have plenty of jobs that call main.sh, without going
> through ftc first.  To be backwards-compatible with those, I'd like to leave
> this definition here.  I know it's redundant with the definition made by
> ftc.
> 
> Maybe we should have a FIXTHIS here, reminding us to remove this when
> we're willing to pull the plug on running pre-1.4 Fuego jobs.

Yes, please add a FIXTHIS to remember.

Thanks,
Daniel

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

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> Sent: Thursday, June 7, 2018 8:29 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@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@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

Re: [Fuego] [PATCH 25/30] ftc: put a fixthis for the documentation

[Tim.Bird]

> -----Original Message-----
> From: Daniel 
> > -----Original Message-----
> > From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> > > +  This interface may change in the future.
> > > +  FIXTHIS: add documentation about timeout,reboot,rebuild"""),
> >
> > OK - I don't like this very much.  This is not a normal FIXTHIS, in
> > that it's visible to end users.  Most FIXTHIS comments are
> > comments inside the code.  This one is, IMHO, very high priority.
> > Other FIXTHIS items range from low priority feature suggestions,
> > to "should check this some time" items.
> >
> > User's shouldn't see notes about the state of the code when they
> > are looking for help.
> >
> > Can you work on this soon?  I don't want this one to go out in a
> > release.  I'm considering re-wording it "MUSTFIX:", and having a
> > regression check for such terms in my release test.
> >
> > Let me know what you think.
> 
> OK, I fixed this. I will send the patches on top of your tree once you have
> merged them.

OK - sounds good.
 -- Tim
> 
> > >  "rm-jobs": ("Removes jobs from Jenkins.",
> > >      """Usage: ftc rm-jobs [--remove-logs]
> > > <board>.<testspec>.<testtype>.<testcase>
> > > @@ -239,6 +240,9 @@ would run the pre_test, pre_check, build and
> deploy
> > > phases of the test.
> > >  This is useful during development of a test (ie. for testing tests).
> > >  Use caution running later phases of a test without their normal
> > >  precursors (e.g. run, without build or deploy).
> > > +
> > > +FIXTHIS: add documentation about timeout,reboot,rebuild,dynamic-
> vars
> > Same issue as above.
> >
> > > +
> > >  """),
> > >
> > >  "build-jobs": ("Build one or more jobs (to execute tests) in Jenkins.",
> > > --
> > > 2.7.4
> >
> >
> > Applied - but changed to MUSTFIX.
> >  -- Tim
> >
> 
> 
> 

Re: [Fuego] [PATCH 26/30] bang: replace the jenkins script

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin 
> > -----Original Message-----
> > From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> > Sent: Thursday, June 7, 2018 8:00 AM
> > To: daniel.sangorrin@toshiba.co.jp; fuego@lists.linuxfoundation.org
> > Subject: RE: [Fuego] [PATCH 26/30] bang: replace the jenkins script
> >
> > Comments inline below...
> >
> > > -----Original Message-----
> > > From: Daniel Sangorrin
> > > This alone is a milestone!
> > >
> > > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > > ---
> > >  engine/scripts/ftc | 20 +++++++++++---------
> > >  1 file changed, 11 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> > > index d0b6a94..12bedb3 100755
> > > --- a/engine/scripts/ftc
> > > +++ b/engine/scripts/ftc
> > > @@ -1206,6 +1206,13 @@ def link_key(item):
> > >
> > >
> > >  def create_job(board, test):
> > > +    global debug
> > > +
> > > +    if debug == 1:
> > > +        debug_param = "--debug"
> > > +    else:
> > > +        debug_param = ""
> > > +
> > This merging of the environment variable used by the Fuego
> > core scripts, and the 'debug' variable used by ftc, results in
> > a loss of functionality.  The FUEGO_DEBUG variable supports
> > multiple bit-wise flags, to debug different parts of the
> > run.  The 'ftc' debug variable did not.
> > See http://fuegotest.org/wiki/FUEGO_DEBUG
> >
> > What probably needs to happen is to have '--debug' take an argument,
> > for the debug value.  I'm applying this patch, because the more important
> > aspect of this is the conversion from calling main.sh to ftc from the
> > Jenkins job.  But one of us should circle back and fix the debug flag
> > value handling.
> 
> OK, I fixed it. I will send the patches on top of the previous ones once you
> merge them. Is that OK?


That will be fine.  I ran into problems with the patches, that I would like
to resolve in a 'next' branch before I commit it to master.  I'm working
to rebase your previous patches and push them to my own 'next' branch,
and get your fixes (and additional fixes I'm making) on that branch,
before pulling it back to the master branch.

Let me know if this workflow will work for you.

Unfortunately, I'm traveling tomorrow, so I'll see how much of this I can
get done.  I'm hoping to have all the dust settle on these changes by the
end of this week.  But I've got travel, meetings, absence from my
home development environment, and jet lag to deal with
in that time frame - so we'll see how it goes.
 -- Tim

> >
> > >      flot_link = '<flotile.FlotPublisher plugin="flot@1.0-SNAPSHOT"/>'
> > >
> > >      # prepare links for the descriptionsetter plugin
> > > @@ -1291,14 +1298,8 @@ def create_job(board, test):
> > >      <customWorkspace>$FUEGO_RW/buildzone</customWorkspace>
> > >      <builders>
> > >      <hudson.tasks.Shell>
> > > -        <command>export Reboot={reboot}
> > > -export Rebuild={rebuild}
> > > -export Target_PreCleanup={precleanup}
> > > -export Target_PostCleanup={postcleanup}
> > > -export TESTDIR={testdir}
> > > -export TESTSPEC={testspec}
> > > -#export FUEGO_DEBUG=1
> > > -timeout --signal=9 {timeout} /bin/bash
> > > $FUEGO_CORE/engine/scripts/main.sh
> > > +        <command>export FUEGO_CALLER="jenkins"
> > > +ftc run-test -b $NODE_NAME -t {testdir} -s {testspec} --timeout
> {timeout} --
> > > reboot {reboot} --rebuild {rebuild} --precleanup {precleanup} --
> postcleanup
> > > {postcleanup} {debug_param}
> > >  </command>
> > >      </hudson.tasks.Shell>
> > >      </builders>
> > > @@ -1317,7 +1318,8 @@ timeout --signal=9 {timeout} /bin/bash
> > > $FUEGO_CORE/engine/scripts/main.sh
> > >  """.format(board=board, reboot=test.reboot, rebuild=test.rebuild,
> > >          precleanup=test.precleanup, postcleanup=test.postcleanup,
> > >          testdir=test.name, testspec=test.spec, timeout=test.timeout,
> > > -        flot_link=flot_link, success_links=success_links, fail_links=fail_links))
> > > +        flot_link=flot_link, success_links=success_links, fail_links=fail_links,
> > > +        debug_param=debug_param))
> > >      fd.close()
> > >
> > >      job_name = board + "." + test.spec + "." + test.name
> > > --
> > > 2.7.4
> >
> >
> > Applied, but not pushed yet.
> >  -- Tim
> >
> 
> 

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

[Tim.Bird]

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

Re: [Fuego] [PATCH 26/30] bang: replace the jenkins script

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
...
> That will be fine.  I ran into problems with the patches, that I would like
> to resolve in a 'next' branch before I commit it to master.  I'm working
> to rebase your previous patches and push them to my own 'next' branch,
> and get your fixes (and additional fixes I'm making) on that branch,
> before pulling it back to the master branch.
> 
> Let me know if this workflow will work for you.

Sure.

> Unfortunately, I'm traveling tomorrow, so I'll see how much of this I can
> get done.  I'm hoping to have all the dust settle on these changes by the
> end of this week.  But I've got travel, meetings, absence from my
> home development environment, and jet lag to deal with
> in that time frame - so we'll see how it goes.

OK, no need to rush it.

Thanks,
Daniel

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

[Daniel Sangorrin]

> -----Original Message-----
> From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> > > 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"

OK, I see what you mean.
You mean that it causes backwards-compatibility with jobs created in the past.
I can fix that.

On the other hand, I think we should mark these backwards compatibility so that we can remove it in the future (I think it is called expand/collapse pattern or something like that) to keep the source maintainable.
 
Thanks
Daniel

Re: [Fuego] [PATCH 26/30] bang: replace the jenkins script

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin 
> > -----Original Message-----
> > From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> ...
> > That will be fine.  I ran into problems with the patches, that I would like
> > to resolve in a 'next' branch before I commit it to master.  I'm working
> > to rebase your previous patches and push them to my own 'next' branch,
> > and get your fixes (and additional fixes I'm making) on that branch,
> > before pulling it back to the master branch.
> >
> > Let me know if this workflow will work for you.
> 
> Sure.

OK - my 'next' branch has the patches.  Can you test it out, or do continued
integration (for these features) on that branch?

One issue I saw with the switchover to Jenkins calling 'ftc run-test', was that
abort messages don't show up in the Jenkins console log.  There must
be some pipe flushing problem between ftc and Jenkins.  I'm not sure if this
shows up on the ftc command line (run outside of Jenkins).

A secondary issue I'd like to get working again (it may have broken sometime
in the 1.3 release), is to support running a test when there is no matching
jenkins job.  Some of the code I saw made it look like this support was removed,
but I could have misinterpreted.  (e.g. find_next_build_number_by_log_scan).

Also, the code for adding a build.xml file to Jenkins for a run that was run outside
of Jenkins control, is commented out (as you noted in your patch).  The reason
for this is that the format of the build.xml file changed when we upgraded Jenkins
versions, and I didn't have time to go back and support the new format.  One
big change in the build.xml format is how the trigger is expressed.  It's now
much more convoluted.

Sorry I'm busy with other things this week, but I may have time to take a look
at some things tomorrow.

In general, I don't see backwards compatibility issues with the dynamic variables
stuff, or support for ftc command line options for reboot, precleanup, etc.
So, if those patches are separate from the 'call ftc run-test from jenkins' feature,
these could be pulled over to the master branch independently (and sooner).

Thanks,
 -- Tim 

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

[Tim.Bird]

> -----Original Message-----
> From: Daniel Sangorrin 
> > -----Original Message-----
> > From: Tim.Bird@sony.com [mailto:Tim.Bird@sony.com]
> > > > 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"
> 
> OK, I see what you mean.
> You mean that it causes backwards-compatibility with jobs created in the
> past.
> I can fix that.
> 
> On the other hand, I think we should mark these backwards compatibility so
> that we can remove it in the future (I think it is called expand/collapse
> pattern or something like that) to keep the source maintainable.

Agreed.
 -- Tim