[PATCH 0/4] buildman: Deal with unicode errors and thread exceptions

[PATCH 0/4] buildman: Deal with unicode errors and thread exceptions

[Simon Glass]

This series corrects the recent problems with gitlab reporting a unicode
exception during a build, for example here:

   https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/249514

Since exceptions in threads can cause buildman to hang, the series adds
support for dealing with these more gracefully, in case it happens again.


Simon Glass (4):
  buildman: Tidy up a few comments
  buildman: Use common code to send an result
  buildman: Handle exceptions in threads gracefully
  buildman: Use bytes for the environment

 tools/buildman/builder.py       | 22 +++++++++++----
 tools/buildman/builderthread.py | 50 +++++++++++++++++++++++----------
 tools/buildman/control.py       | 18 ++++++++----
 tools/buildman/func_test.py     | 40 ++++++++++++++++++++++++--
 tools/buildman/toolchain.py     | 24 ++++++++++------
 5 files changed, 119 insertions(+), 35 deletions(-)

-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH 1/4] buildman: Tidy up a few comments

[Simon Glass]

Add some function comments which are missing, or missing arguments.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/buildman/builderthread.py | 10 +++++++---
 tools/buildman/control.py       |  2 ++
 tools/buildman/func_test.py     | 13 +++++++++++--
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 06ed27203a2..6f08ff2da0c 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -89,10 +89,14 @@ class BuilderThread(threading.Thread):
     Members:
         builder: The builder which contains information we might need
         thread_num: Our thread number (0-n-1), used to decide on a
-                temporary directory. If this is -1 then there are no threads
-                and we are the (only) main process
+            temporary directory. If this is -1 then there are no threads
+            and we are the (only) main process
+        mrproper: Use 'make mrproper' before each reconfigure
+        per_board_out_dir: True to build in a separate persistent directory per
+            board rather than a thread-specific directory
+        test_exception: Used for testing; True to raise an exception instead of
+            reporting the build result
     """
-    def __init__(self, builder, thread_num, mrproper, per_board_out_dir):
         """Set up a new builder thread"""
         threading.Thread.__init__(self)
         self.builder = builder
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index a7675701466..5fcfba7ca36 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -124,6 +124,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
                 arguments. This setting is useful for tests.
         board: Boards() object to use, containing a list of available
                 boards. If this is None it will be created and scanned.
+        clean_dir: Used for tests only, indicates that the existing output_dir
+            should be removed before starting the build
     """
     global builder
 
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index 3dd2e6ee5bb..c6997d1ee25 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -219,12 +219,21 @@ class TestFunctional(unittest.TestCase):
         return command.RunPipe([[self._buildman_pathname] + list(args)],
                 capture=True, capture_stderr=True)
 
-    def _RunControl(self, *args, clean_dir=False, boards=None):
+        """Run buildman
+
+        Args:
+            args: List of arguments to pass
+            boards:
+            clean_dir: Used for tests only, indicates that the existing output_dir
+                should be removed before starting the build
+
+        Returns:
+            result code from buildman
+        """
         sys.argv = [sys.argv[0]] + list(args)
         options, args = cmdline.ParseArgs()
         result = control.DoBuildman(options, args, toolchains=self._toolchains,
                 make_func=self._HandleMake, boards=boards or self._boards,
-                clean_dir=clean_dir)
         self._builder = control.builder
         return result
 
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH 2/4] buildman: Use common code to send an result

[Simon Glass]

At present the code to report a build result is duplicated. Put it in a
common function to avoid this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/buildman/builderthread.py | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 6f08ff2da0c..76ffbb66be6 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -444,6 +444,17 @@ class BuilderThread(threading.Thread):
                         target = '%s-%s%s' % (base, dirname, ext)
                 shutil.copy(fname, os.path.join(build_dir, target))
 
+    def _SendResult(self, result):
+        """Send a result to the builder for processing
+
+        Args:
+            result: CommandResult object containing the results of the build
+        """
+        if self.thread_num != -1:
+            self.builder.out_queue.put(result)
+        else:
+            self.builder.ProcessResult(result)
+
     def RunJob(self, job):
         """Run a single job
 
@@ -517,10 +528,7 @@ class BuilderThread(threading.Thread):
 
                 # We have the build results, so output the result
                 self._WriteResult(result, job.keep_outputs, job.work_in_output)
-                if self.thread_num != -1:
-                    self.builder.out_queue.put(result)
-                else:
-                    self.builder.ProcessResult(result)
+                self._SendResult(result)
         else:
             # Just build the currently checked-out build
             result, request_config = self.RunCommit(None, brd, work_dir, True,
@@ -529,10 +537,7 @@ class BuilderThread(threading.Thread):
                         work_in_output=job.work_in_output)
             result.commit_upto = 0
             self._WriteResult(result, job.keep_outputs, job.work_in_output)
-            if self.thread_num != -1:
-                self.builder.out_queue.put(result)
-            else:
-                self.builder.ProcessResult(result)
+            self._SendResult(result)
 
     def run(self):
         """Our thread's run function
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH 3/4] buildman: Handle exceptions in threads gracefully

[Simon Glass]

There have been at least a few cases where an exception has occurred in a
thread and resulted in buildman hanging: running out of disk space and
getting a unicode error.

Handle these by collecting a list of exceptions, printing them out and
reporting failure if any are found. Add a test for this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/buildman/builder.py       | 22 +++++++++++++++++-----
 tools/buildman/builderthread.py | 14 +++++++++++++-
 tools/buildman/control.py       | 16 +++++++++++-----
 tools/buildman/func_test.py     | 15 +++++++++++++++
 4 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index be8a8fa13a6..ce852eb03a3 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -182,6 +182,7 @@ class Builder:
             only useful for testing in-tree builds.
         work_in_output: Use the output directory as the work directory and
             don't write to a separate output directory.
+        thread_exceptions: List of exceptions raised by thread jobs
 
     Private members:
         _base_board_dict: Last-summarised Dict of boards
@@ -235,7 +236,8 @@ class Builder:
                  no_subdirs=False, full_path=False, verbose_build=False,
                  mrproper=False, per_board_out_dir=False,
                  config_only=False, squash_config_y=False,
-                 warnings_as_errors=False, work_in_output=False):
+                 warnings_as_errors=False, work_in_output=False,
+                 test_thread_exceptions=False):
         """Create a new Builder object
 
         Args:
@@ -262,6 +264,9 @@ class Builder:
             warnings_as_errors: Treat all compiler warnings as errors
             work_in_output: Use the output directory as the work directory and
                 don't write to a separate output directory.
+            test_thread_exceptions: Uses for tests only, True to make the
+                threads raise an exception instead of reporting their result.
+                This simulates a failure in the code somewhere
         """
         self.toolchains = toolchains
         self.base_dir = base_dir
@@ -311,13 +316,16 @@ class Builder:
         self._re_migration_warning = re.compile(r'^={21} WARNING ={22}\n.*\n=+\n',
                                                 re.MULTILINE | re.DOTALL)
 
+        self.thread_exceptions = []
+        self.test_thread_exceptions = test_thread_exceptions
         if self.num_threads:
             self._single_builder = None
             self.queue = queue.Queue()
             self.out_queue = queue.Queue()
             for i in range(self.num_threads):
-                t = builderthread.BuilderThread(self, i, mrproper,
-                        per_board_out_dir)
+                t = builderthread.BuilderThread(
+                        self, i, mrproper, per_board_out_dir,
+                        test_exception=test_thread_exceptions)
                 t.setDaemon(True)
                 t.start()
                 self.threads.append(t)
@@ -1676,6 +1684,7 @@ class Builder:
             Tuple containing:
                 - number of boards that failed to build
                 - number of boards that issued warnings
+                - list of thread exceptions raised
         """
         self.commit_count = len(commits) if commits else 1
         self.commits = commits
@@ -1689,7 +1698,7 @@ class Builder:
         Print('\rStarting build...', newline=False)
         self.SetupBuild(board_selected, commits)
         self.ProcessResult(None)
-
+        self.thread_exceptions = []
         # Create jobs to build all commits for each board
         for brd in board_selected.values():
             job = builderthread.BuilderJob()
@@ -1728,5 +1737,8 @@ class Builder:
             rate = float(self.count) / duration.total_seconds()
             msg += ', duration %s, rate %1.2f' % (duration, rate)
         Print(msg)
+        if self.thread_exceptions:
+            Print('Failed: %d thread exceptions' % len(self.thread_exceptions),
+                  colour=self.col.RED)
 
-        return (self.fail, self.warned)
+        return (self.fail, self.warned, self.thread_exceptions)
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 76ffbb66be6..ddb3eab8c03 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -97,12 +97,15 @@ class BuilderThread(threading.Thread):
         test_exception: Used for testing; True to raise an exception instead of
             reporting the build result
     """
+    def __init__(self, builder, thread_num, mrproper, per_board_out_dir,
+                 test_exception=False):
         """Set up a new builder thread"""
         threading.Thread.__init__(self)
         self.builder = builder
         self.thread_num = thread_num
         self.mrproper = mrproper
         self.per_board_out_dir = per_board_out_dir
+        self.test_exception = test_exception
 
     def Make(self, commit, brd, stage, cwd, *args, **kwargs):
         """Run 'make' on a particular commit and board.
@@ -449,7 +452,12 @@ class BuilderThread(threading.Thread):
 
         Args:
             result: CommandResult object containing the results of the build
+
+        Raises:
+            ValueError if self.test_exception is true (for testing)
         """
+        if self.test_exception:
+            raise ValueError('test exception')
         if self.thread_num != -1:
             self.builder.out_queue.put(result)
         else:
@@ -547,5 +555,9 @@ class BuilderThread(threading.Thread):
         """
         while True:
             job = self.builder.queue.get()
-            self.RunJob(job)
+            try:
+                self.RunJob(job)
+            except Exception as e:
+                print('Thread exception:', e)
+                self.builder.thread_exceptions.append(e)
             self.builder.queue.task_done()
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 5fcfba7ca36..a98d1b4c06f 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -110,7 +110,7 @@ def ShowToolchainPrefix(boards, toolchains):
     return None
 
 def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
-               clean_dir=False):
+               clean_dir=False, test_thread_exceptions=False):
     """The main control code for buildman
 
     Args:
@@ -126,6 +126,9 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
                 boards. If this is None it will be created and scanned.
         clean_dir: Used for tests only, indicates that the existing output_dir
             should be removed before starting the build
+        test_thread_exceptions: Uses for tests only, True to make the threads
+            raise an exception instead of reporting their result. This simulates
+            a failure in the code somewhere
     """
     global builder
 
@@ -330,7 +333,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
             config_only=options.config_only,
             squash_config_y=not options.preserve_config_y,
             warnings_as_errors=options.warnings_as_errors,
-            work_in_output=options.work_in_output)
+            work_in_output=options.work_in_output,
+            test_thread_exceptions=test_thread_exceptions)
     builder.force_config_on_failure = not options.quick
     if make_func:
         builder.do_make = make_func
@@ -370,9 +374,11 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
         if options.summary:
             builder.ShowSummary(commits, board_selected)
         else:
-            fail, warned = builder.BuildBoards(commits, board_selected,
-                                options.keep_outputs, options.verbose)
-            if fail:
+            fail, warned, excs = builder.BuildBoards(
+                commits, board_selected, options.keep_outputs, options.verbose)
+            if excs:
+                return 102
+            elif fail:
                 return 100
             elif warned and not options.ignore_warnings:
                 return 101
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index c6997d1ee25..61e3012d2b1 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -16,6 +16,7 @@ from buildman import toolchain
 from patman import command
 from patman import gitutil
 from patman import terminal
+from patman import test_util
 from patman import tools
 
 settings_data = '''
@@ -219,6 +220,8 @@ class TestFunctional(unittest.TestCase):
         return command.RunPipe([[self._buildman_pathname] + list(args)],
                 capture=True, capture_stderr=True)
 
+    def _RunControl(self, *args, boards=None, clean_dir=False,
+                    test_thread_exceptions=False):
         """Run buildman
 
         Args:
@@ -226,6 +229,9 @@ class TestFunctional(unittest.TestCase):
             boards:
             clean_dir: Used for tests only, indicates that the existing output_dir
                 should be removed before starting the build
+            test_thread_exceptions: Uses for tests only, True to make the threads
+                raise an exception instead of reporting their result. This simulates
+                a failure in the code somewhere
 
         Returns:
             result code from buildman
@@ -234,6 +240,8 @@ class TestFunctional(unittest.TestCase):
         options, args = cmdline.ParseArgs()
         result = control.DoBuildman(options, args, toolchains=self._toolchains,
                 make_func=self._HandleMake, boards=boards or self._boards,
+                clean_dir=clean_dir,
+                test_thread_exceptions=test_thread_exceptions)
         self._builder = control.builder
         return result
 
@@ -597,3 +605,10 @@ class TestFunctional(unittest.TestCase):
         with self.assertRaises(SystemExit) as e:
             self._RunControl('-w', clean_dir=False)
         self.assertIn("specify -o", str(e.exception))
+
+    def testThreadExceptions(self):
+        """Test that exceptions in threads are reported"""
+        with test_util.capture_sys_output() as (stdout, stderr):
+            self.assertEqual(102, self._RunControl('-o', self._output_dir,
+                                                   test_thread_exceptions=True))
+        self.assertIn('Thread exception: test exception', stdout.getvalue())
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH 4/4] buildman: Use bytes for the environment

[Simon Glass]

At present we sometimes see problems in gitlab where the environment has
0x80 characters or sequences which are not valid UTF-8.

Avoid this by using bytes for the environment, both internal to buildman
and when writing out the 'env' file. Add a test to make sure this works
as expected.

Reported-by: Marek Vasut <marex@denx.de>
Fixes: e5fc79ea718 ("buildman: Write the environment out to an 'env' file")
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/buildman/builderthread.py |  5 ++---
 tools/buildman/func_test.py     | 12 ++++++++++++
 tools/buildman/toolchain.py     | 24 ++++++++++++++++--------
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index ddb3eab8c03..48128cf6732 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -351,10 +351,9 @@ class BuilderThread(threading.Thread):
 
             # Write out the image and function size information and an objdump
             env = result.toolchain.MakeEnvironment(self.builder.full_path)
-            with open(os.path.join(build_dir, 'out-env'), 'w',
-                      encoding='utf-8') as fd:
+            with open(os.path.join(build_dir, 'out-env'), 'wb') as fd:
                 for var in sorted(env.keys()):
-                    print('%s="%s"' % (var, env[var]), file=fd)
+                    fd.write(b'%s="%s"' % (var, env[var]))
             lines = []
             for fname in BASE_ELF_FILENAMES:
                 cmd = ['%snm' % self.toolchain.cross, '--size-sort', fname]
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index 61e3012d2b1..7edbee0652f 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -572,6 +572,18 @@ class TestFunctional(unittest.TestCase):
         self.assertTrue(os.path.exists(os.path.join(board0_dir, 'done')))
         self.assertTrue(os.path.exists(os.path.join(board0_dir, 'out-env')))
 
+    def testEnvironmentUnicode(self):
+        """Test there are no unicode errors when the env has non-ASCII chars"""
+        try:
+            varname = b'buildman_test_var'
+            os.environb[varname] = b'strange\x80chars'
+            self.assertEqual(0, self._RunControl('-o', self._output_dir))
+            board0_dir = os.path.join(self._output_dir, 'current', 'board0')
+            self.assertTrue(os.path.exists(os.path.join(board0_dir, 'done')))
+            self.assertTrue(os.path.exists(os.path.join(board0_dir, 'out-env')))
+        finally:
+            del os.environb[varname]
+
     def testWorkInOutput(self):
         """Test the -w option which should write directly to the output dir"""
         board_list = board.Boards()
diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py
index acb5a29c8fb..fd137f7300e 100644
--- a/tools/buildman/toolchain.py
+++ b/tools/buildman/toolchain.py
@@ -179,27 +179,35 @@ class Toolchain:
         output and possibly unicode encoded output of all build tools by
         adding LC_ALL=C.
 
+        Note that os.environb is used to obtain the environment, since in some
+        cases the environment many contain non-ASCII characters and we see
+        errors like:
+
+          UnicodeEncodeError: 'utf-8' codec can't encode characters in position
+             569-570: surrogates not allowed
+
         Args:
             full_path: Return the full path in CROSS_COMPILE and don't set
                 PATH
         Returns:
-            Dict containing the environemnt to use. This is based on the current
-            environment, with changes as needed to CROSS_COMPILE, PATH and
-            LC_ALL.
+            Dict containing the (bytes) environment to use. This is based on the
+            current environment, with changes as needed to CROSS_COMPILE, PATH
+            and LC_ALL.
         """
-        env = dict(os.environ)
+        env = dict(os.environb)
         wrapper = self.GetWrapper()
 
         if self.override_toolchain:
             # We'll use MakeArgs() to provide this
             pass
         elif full_path:
-            env['CROSS_COMPILE'] = wrapper + os.path.join(self.path, self.cross)
+            env[b'CROSS_COMPILE'] = tools.ToBytes(
+                wrapper + os.path.join(self.path, self.cross))
         else:
-            env['CROSS_COMPILE'] = wrapper + self.cross
-            env['PATH'] = self.path + ':' + env['PATH']
+            env[b'CROSS_COMPILE'] = tools.ToBytes(wrapper + self.cross)
+            env[b'PATH'] = tools.ToBytes(self.path) + b':' + env[b'PATH']
 
-        env['LC_ALL'] = 'C'
+        env[b'LC_ALL'] = b'C'
 
         return env
 
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH 4/4] buildman: Use bytes for the environment

[Simon Glass]

At present we sometimes see problems in gitlab where the environment has
0x80 characters or sequences which are not valid UTF-8.

Avoid this by using bytes for the environment, both internal to buildman
and when writing out the 'env' file. Add a test to make sure this works
as expected.

Reported-by: Marek Vasut <marex@denx.de>
Fixes: e5fc79ea718 ("buildman: Write the environment out to an 'env' file")
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/buildman/builderthread.py |  5 ++---
 tools/buildman/func_test.py     | 12 ++++++++++++
 tools/buildman/toolchain.py     | 24 ++++++++++++++++--------
 3 files changed, 30 insertions(+), 11 deletions(-)

Applied to u-boot-dm, thanks!

[PATCH 3/4] buildman: Handle exceptions in threads gracefully

[Simon Glass]

There have been at least a few cases where an exception has occurred in a
thread and resulted in buildman hanging: running out of disk space and
getting a unicode error.

Handle these by collecting a list of exceptions, printing them out and
reporting failure if any are found. Add a test for this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/buildman/builder.py       | 22 +++++++++++++++++-----
 tools/buildman/builderthread.py | 14 +++++++++++++-
 tools/buildman/control.py       | 16 +++++++++++-----
 tools/buildman/func_test.py     | 15 +++++++++++++++
 4 files changed, 56 insertions(+), 11 deletions(-)

Applied to u-boot-dm, thanks!

[PATCH 2/4] buildman: Use common code to send an result

[Simon Glass]

At present the code to report a build result is duplicated. Put it in a
common function to avoid this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/buildman/builderthread.py | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Applied to u-boot-dm, thanks!

[PATCH 1/4] buildman: Tidy up a few comments

[Simon Glass]

Add some function comments which are missing, or missing arguments.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/buildman/builderthread.py | 10 +++++++---
 tools/buildman/control.py       |  2 ++
 tools/buildman/func_test.py     | 13 +++++++++++--
 3 files changed, 20 insertions(+), 5 deletions(-)

Applied to u-boot-dm, thanks!