[PATCH 1/3] buildman: Write output even on fatal error

[PATCH 1/3] buildman: Write output even on fatal error

[Simon Glass]

At present buildman does not write any output (to the 'out' and 'err)
files if the build terminates with a fatal error. This is to avoid adding
lots of spam to the logs.

However there are times when this is actually useful, such as when the
build fails for an obscure reason such as a Kconfig loop.

Update the logic to always write the output, so that the user gets a clue
as to what is happening.

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

 tools/buildman/builderthread.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 48128cf6732..3e450e40670 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -300,16 +300,12 @@ class BuilderThread(threading.Thread):
             work_in_output: Use the output directory as the work directory and
                 don't write to a separate output directory.
         """
-        # Fatal error
-        if result.return_code < 0:
-            return
-
         # If we think this might have been aborted with Ctrl-C, record the
         # failure but not that we are 'done' with this board. A retry may fix
         # it.
-        maybe_aborted =  result.stderr and 'No child processes' in result.stderr
+        maybe_aborted = result.stderr and 'No child processes' in result.stderr
 
-        if result.already_done:
+        if result.return_code >= 0 and result.already_done:
             return
 
         # Write the output and stderr
@@ -332,6 +328,10 @@ class BuilderThread(threading.Thread):
         elif os.path.exists(errfile):
             os.remove(errfile)
 
+        # Fatal error
+        if result.return_code < 0:
+            return
+
         if result.toolchain:
             # Write the build result and toolchain information.
             done_file = self.builder.GetDoneFile(result.commit_upto,
-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH 2/3] buildman: Detect Kconfig loops

[Simon Glass]

Hex and int Kconfig options are supposed to have defaults. This is so we
can configure U-Boot without having to enter particular values for the
items that don't have specific values in the board's defconfig file.

If this rule is not followed, then introducing a new Kconfig can produce
a loop like this:

   Break things (BREAK_ME) [] (NEW)
   Error in reading or end of file.

   Break things (BREAK_ME) [] (NEW)
   Error in reading or end of file.

The continues forever since buildman passes /dev/null to 'conf', and
the build system just tries again. Eventually there is so much output that
buildman runs out of memory.

We can detect this situation by looking for a symbol (like 'BREAK_ME')
which has no default (the '[]' above) and is marked as new. If this
appears multiple times in the output, we know something is wrong.

Add a filter function for the output which detects this situation. Allow
it to return True to terminate the process. Implement this termination in
cros_subprocess.

With this we get a nice message:

   buildman --board sandbox -T0
   Building current source for 1 boards (0 threads, 32 jobs per thread)
      sandbox:  w+   sandbox
   +.config:66:warning: symbol value '' invalid for BREAK_ME
   +
   +Error in reading or end of file.
   +make[3]: *** [scripts/kconfig/Makefile:75: syncconfig] Terminated
   +make[2]: *** [Makefile:569: syncconfig] Terminated
   +make: *** [Makefile:177: sub-make] Terminated
   +(** did you define an int/hex Kconfig with no default? **)

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

 tools/buildman/builder.py       | 43 ++++++++++++++++++++++++++++++++-
 tools/patman/command.py         |  7 ++++--
 tools/patman/cros_subprocess.py | 10 ++++++--
 3 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index ce852eb03a3..122f0d14065 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -24,6 +24,17 @@ from patman import gitutil
 from patman import terminal
 from patman.terminal import Print
 
+# This indicates an new int or hex Kconfig property with no default
+# It hangs the build since the 'conf' tool cannot proceed without valid input.
+#
+# We get a repeat sequence of something like this:
+# >>
+# Break things (BREAK_ME) [] (NEW)
+# Error in reading or end of file.
+# <<
+# which indicates that BREAK_ME has an empty default
+RE_NO_DEFAULT = re.compile(b'\((\w+)\) \[] \(NEW\)')
+
 """
 Theory of Operation
 
@@ -200,6 +211,8 @@ class Builder:
         _working_dir: Base working directory containing all threads
         _single_builder: BuilderThread object for the singer builder, if
             threading is not being used
+        _terminated: Thread was terminated due to an error
+        _restarting_config: True if 'Restart config' is detected in output
     """
     class Outcome:
         """Records a build outcome for a single make invocation
@@ -304,6 +317,8 @@ class Builder:
         self.work_in_output = work_in_output
         if not self.squash_config_y:
             self.config_filenames += EXTRA_CONFIG_FILENAMES
+        self._terminated = False
+        self._restarting_config = False
 
         self.warnings_as_errors = warnings_as_errors
         self.col = terminal.Color()
@@ -429,9 +444,35 @@ class Builder:
             args: Arguments to pass to make
             kwargs: Arguments to pass to command.RunPipe()
         """
+
+        def check_output(stream, data):
+            if b'Restart config' in data:
+                self._restarting_config = True
+
+            # If we see 'Restart config' following by multiple errors
+            if self._restarting_config:
+                m = RE_NO_DEFAULT.findall(data)
+
+                # Number of occurences of each Kconfig item
+                multiple = [m.count(val) for val in set(m)]
+
+                # If any of them occur more than once, we have a loop
+                if [val for val in multiple if val > 1]:
+                    self._terminated = True
+                    return True
+            return False
+
+        self._restarting_config = False
+        self._terminated  = False
         cmd = [self.gnu_make] + list(args)
         result = command.RunPipe([cmd], capture=True, capture_stderr=True,
-                cwd=cwd, raise_on_error=False, infile='/dev/null', **kwargs)
+                cwd=cwd, raise_on_error=False, infile='/dev/null',
+                output_func=check_output, **kwargs)
+
+        if self._terminated:
+            # Try to be helpful
+            result.stderr += '(** did you define an int/hex Kconfig with no default? **)'
+
         if self.verbose_build:
             result.stdout = '%s\n' % (' '.join(cmd)) + result.stdout
             result.combined = '%s\n' % (' '.join(cmd)) + result.combined
diff --git a/tools/patman/command.py b/tools/patman/command.py
index bf8ea6c8c3c..d54b1e0efce 100644
--- a/tools/patman/command.py
+++ b/tools/patman/command.py
@@ -49,7 +49,8 @@ test_result = None
 
 def RunPipe(pipe_list, infile=None, outfile=None,
             capture=False, capture_stderr=False, oneline=False,
-            raise_on_error=True, cwd=None, binary=False, **kwargs):
+            raise_on_error=True, cwd=None, binary=False,
+            output_func=None, **kwargs):
     """
     Perform a command pipeline, with optional input/output filenames.
 
@@ -63,6 +64,8 @@ def RunPipe(pipe_list, infile=None, outfile=None,
         capture: True to capture output
         capture_stderr: True to capture stderr
         oneline: True to strip newline chars from output
+        output_func: Output function to call with each output fragment
+            (if it returns True the function terminates)
         kwargs: Additional keyword arguments to cros_subprocess.Popen()
     Returns:
         CommandResult object
@@ -105,7 +108,7 @@ def RunPipe(pipe_list, infile=None, outfile=None,
 
     if capture:
         result.stdout, result.stderr, result.combined = (
-                last_pipe.CommunicateFilter(None))
+                last_pipe.CommunicateFilter(output_func))
         if result.stdout and oneline:
             result.output = result.stdout.rstrip(b'\r\n')
         result.return_code = last_pipe.wait()
diff --git a/tools/patman/cros_subprocess.py b/tools/patman/cros_subprocess.py
index fdd51386856..88a4693feff 100644
--- a/tools/patman/cros_subprocess.py
+++ b/tools/patman/cros_subprocess.py
@@ -128,6 +128,9 @@ class Popen(subprocess.Popen):
                         sys.stdout or sys.stderr.
                 data: a string containing the data
 
+            Returns:
+                True to terminate the process
+
         Note: The data read is buffered in memory, so do not use this
         method if the data size is large or unlimited.
 
@@ -175,6 +178,7 @@ class Popen(subprocess.Popen):
             stderr = bytearray()
         combined = bytearray()
 
+        stop_now = False
         input_offset = 0
         while read_set or write_set:
             try:
@@ -212,7 +216,7 @@ class Popen(subprocess.Popen):
                     stdout += data
                     combined += data
                     if output:
-                        output(sys.stdout, data)
+                        stop_now = output(sys.stdout, data)
             if self.stderr in rlist:
                 data = b''
                 # We will get an error on read if the pty is closed
@@ -227,7 +231,9 @@ class Popen(subprocess.Popen):
                     stderr += data
                     combined += data
                     if output:
-                        output(sys.stderr, data)
+                        stop_now = output(sys.stderr, data)
+            if stop_now:
+                self.terminate()
 
         # All data exchanged.    Translate lists into strings.
         stdout = self.ConvertData(stdout)
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH 2/3] buildman: Detect Kconfig loops

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On Tue, Oct 19, 2021 at 09:43:24PM -0600, Simon Glass wrote:

> Hex and int Kconfig options are supposed to have defaults. This is so we
> can configure U-Boot without having to enter particular values for the
> items that don't have specific values in the board's defconfig file.

That's not true.  All symbols that we can make reasonable defaults for
get them.  It's just that for boolean n is a reasonable default.
int/hex often just need to be entered, period.

Everything else is fine however, thanks for digging in to this.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] buildman: Detect Kconfig loops

[Simon Glass]

Hi Tom,

On Thu, 21 Oct 2021 at 06:13, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Oct 19, 2021 at 09:43:24PM -0600, Simon Glass wrote:
>
> > Hex and int Kconfig options are supposed to have defaults. This is so we
> > can configure U-Boot without having to enter particular values for the
> > items that don't have specific values in the board's defconfig file.
>
> That's not true.  All symbols that we can make reasonable defaults for
> get them.  It's just that for boolean n is a reasonable default.
> int/hex often just need to be entered, period.
>
> Everything else is fine however, thanks for digging in to this.

I got that from there and it made sense as to why we have this problem:

https://docs.zephyrproject.org/2.4.0/guides/kconfig/tips.html#redundant-defaults

But I think you are right if we should update the commit message here
to avoid confusion. Shall I send v2?

Regards,
Simon

Re: [PATCH 2/3] buildman: Detect Kconfig loops

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On Thu, Oct 21, 2021 at 10:09:56AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 21 Oct 2021 at 06:13, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Oct 19, 2021 at 09:43:24PM -0600, Simon Glass wrote:
> >
> > > Hex and int Kconfig options are supposed to have defaults. This is so we
> > > can configure U-Boot without having to enter particular values for the
> > > items that don't have specific values in the board's defconfig file.
> >
> > That's not true.  All symbols that we can make reasonable defaults for
> > get them.  It's just that for boolean n is a reasonable default.
> > int/hex often just need to be entered, period.
> >
> > Everything else is fine however, thanks for digging in to this.
> 
> I got that from there and it made sense as to why we have this problem:
> 
> https://docs.zephyrproject.org/2.4.0/guides/kconfig/tips.html#redundant-defaults

Yeah, we're just in the bad spot because of things not being fully
migrated.

> But I think you are right if we should update the commit message here
> to avoid confusion. Shall I send v2?

Yes please.

> 
> Regards,
> Simon

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] buildman: Detect Kconfig loops

[Stefano Babic]

Hi Simon,

On 21.10.21 18:15, Tom Rini wrote:
> On Thu, Oct 21, 2021 at 10:09:56AM -0600, Simon Glass wrote:
>> Hi Tom,
>>
>> On Thu, 21 Oct 2021 at 06:13, Tom Rini <trini@konsulko.com> wrote:
>>>
>>> On Tue, Oct 19, 2021 at 09:43:24PM -0600, Simon Glass wrote:
>>>
>>>> Hex and int Kconfig options are supposed to have defaults. This is so we
>>>> can configure U-Boot without having to enter particular values for the
>>>> items that don't have specific values in the board's defconfig file.
>>>
>>> That's not true.  All symbols that we can make reasonable defaults for
>>> get them.  It's just that for boolean n is a reasonable default.
>>> int/hex often just need to be entered, period.
>>>
>>> Everything else is fine however, thanks for digging in to this.
>>
>> I got that from there and it made sense as to why we have this problem:
>>
>> https://docs.zephyrproject.org/2.4.0/guides/kconfig/tips.html#redundant-defaults
> 
> Yeah, we're just in the bad spot because of things not being fully
> migrated.
> 
>> But I think you are right if we should update the commit message here
>> to avoid confusion. Shall I send v2?
> 
> Yes please.

As delta...

I applied 1-2 to u-boot-imx (else I was stuck), and they are already 
merged as part of my PR. Your patches are already on -master.

Regards,
Stefano



-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================

Re: [PATCH 2/3] buildman: Detect Kconfig loops

[Simon Glass]

Hi Stefano,

On Thu, 21 Oct 2021 at 10:35, Stefano Babic <sbabic@denx.de> wrote:
>
> Hi Simon,
>
> On 21.10.21 18:15, Tom Rini wrote:
> > On Thu, Oct 21, 2021 at 10:09:56AM -0600, Simon Glass wrote:
> >> Hi Tom,
> >>
> >> On Thu, 21 Oct 2021 at 06:13, Tom Rini <trini@konsulko.com> wrote:
> >>>
> >>> On Tue, Oct 19, 2021 at 09:43:24PM -0600, Simon Glass wrote:
> >>>
> >>>> Hex and int Kconfig options are supposed to have defaults. This is so we
> >>>> can configure U-Boot without having to enter particular values for the
> >>>> items that don't have specific values in the board's defconfig file.
> >>>
> >>> That's not true.  All symbols that we can make reasonable defaults for
> >>> get them.  It's just that for boolean n is a reasonable default.
> >>> int/hex often just need to be entered, period.
> >>>
> >>> Everything else is fine however, thanks for digging in to this.
> >>
> >> I got that from there and it made sense as to why we have this problem:
> >>
> >> https://docs.zephyrproject.org/2.4.0/guides/kconfig/tips.html#redundant-defaults
> >
> > Yeah, we're just in the bad spot because of things not being fully
> > migrated.
> >
> >> But I think you are right if we should update the commit message here
> >> to avoid confusion. Shall I send v2?
> >
> > Yes please.
>
> As delta...

Oh I can't do that as it was the commit message that needed to change.

> I applied 1-2 to u-boot-imx (else I was stuck), and they are already
> merged as part of my PR. Your patches are already on -master.

It's fine, just an error that I doubt people will not notice / get confused by.

Regards,
Simon