[PATCH 0/9] buildman: Add initial support for config fragments

[PATCH 0/9] buildman: Add initial support for config fragments

[Simon Glass]

This series updates buildman to process #include lines in defconfig
files. With this, it is no-longer necessary to duplicate lines certain
lines from the include-file in the defconfig, e.g. CONFIG_ARM and
CONFIG_SOC_...


Simon Glass (9):
  buildman: Add a lower-level test for KconfigScanner
  buildman: Set up the tout library
  buildman: Support #include files in defconfigs
  buildman: Correct the indentation in the setting-up section
  buildman: Document the toolchain-prefix section
  buildman: Correct logic for adding a toolchain
  buildman: Support a tilde to represent the home directory
  buildman: Propose a format for extra boards
  RFC: Show building with #include in defconfig

 configs/j721e_sk_a72_defconfig |   5 --
 tools/buildman/boards.py       |  25 +++++++-
 tools/buildman/buildman.rst    | 102 ++++++++++++++++++++++++++++-----
 tools/buildman/func_test.py    |  85 +++++++++++++++++++++++++++
 tools/buildman/main.py         |   9 ++-
 tools/buildman/test.py         |  50 ++++++++++++++++
 tools/buildman/toolchain.py    |  47 ++++++++-------
 7 files changed, 281 insertions(+), 42 deletions(-)

-- 
2.34.1

[PATCH 1/9] buildman: Add a lower-level test for KconfigScanner

[Simon Glass]

This code is tested by test_scan_defconfigs() but it is useful to have
some specific tests for the KconfigScanner's operation in U-Boot. Add
a test which checks that the values are obtained correctly.

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

 tools/buildman/func_test.py | 51 +++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index 0ac9fc7e44f..db3c9d63ec4 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -1067,3 +1067,54 @@ endif
             result = self._RunControl('--print-arch', 'board0')
         self.assertEqual('arm\n', stdout.getvalue())
         self.assertEqual('', stderr.getvalue())
+
+    def test_kconfig_scanner(self):
+        """Test using the kconfig scanner to determine important values
+
+        Note that there is already a test_scan_defconfigs() which checks the
+        higher-level scan_defconfigs() function. This test checks just the
+        scanner itself
+        """
+        src = self._git_dir
+        scanner = boards.KconfigScanner(src)
+
+        # First do a simple sanity check
+        norm = os.path.join(src, 'board0_defconfig')
+        tools.write_file(norm, 'CONFIG_TARGET_BOARD0=y', False)
+        res = scanner.scan(norm, True)
+        self.assertEqual(({
+            'arch': 'arm',
+            'cpu': 'armv7',
+            'soc': '-',
+            'vendor': 'Tester',
+            'board': 'ARM Board 0',
+            'config': 'config0',
+            'target': 'board0'}, []), res)
+
+        # Check that the SoC cannot be changed and the filename does not affect
+        # the resulting board
+        tools.write_file(norm, '''CONFIG_TARGET_BOARD2=y
+CONFIG_SOC="fred"
+''', False)
+        res = scanner.scan(norm, True)
+        self.assertEqual(({
+            'arch': 'powerpc',
+            'cpu': 'ppc',
+            'soc': 'mpc85xx',
+            'vendor': 'Tester',
+            'board': 'PowerPC board 1',
+            'config': 'config2',
+            'target': 'board0'}, []), res)
+
+        # Check handling of missing information
+        tools.write_file(norm, '', False)
+        res = scanner.scan(norm, True)
+        self.assertEqual(({
+            'arch': '-',
+            'cpu': '-',
+            'soc': '-',
+            'vendor': '-',
+            'board': '-',
+            'config': '-',
+            'target': 'board0'},
+            ['WARNING: board0_defconfig: No TARGET_BOARD0 enabled']), res)
-- 
2.34.1

[PATCH 2/9] buildman: Set up the tout library

[Simon Glass]

Use this to control info and debugging messages. Enable them if -v is
given.

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

 tools/buildman/main.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/buildman/main.py b/tools/buildman/main.py
index 3cf877e5e68..a948f36d9c0 100755
--- a/tools/buildman/main.py
+++ b/tools/buildman/main.py
@@ -25,6 +25,7 @@ from buildman import cmdline
 from buildman import control
 from u_boot_pylib import test_util
 from u_boot_pylib import tools
+from u_boot_pylib import tout
 
 def run_tests(skip_net_tests, debug, verbose, args):
     """Run the buildman tests
@@ -93,8 +94,12 @@ def run_buildman():
 
     # Build selected commits for selected boards
     else:
-        bsettings.setup(args.config_file)
-        ret_code = control.do_buildman(args)
+        try:
+            tout.init(tout.INFO if args.verbose else tout.WARNING)
+            bsettings.setup(args.config_file)
+            ret_code = control.do_buildman(args)
+        finally:
+            tout.uninit()
         return ret_code
 
 
-- 
2.34.1

[PATCH 3/9] buildman: Support #include files in defconfigs

[Simon Glass]

This is used by some boards in U-Boot and is a convenient way to deal
with common settings where using a Kconfig files is not desirable.

Detect #include files and process them as if they were part of the
original file.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/30
---

 tools/buildman/boards.py    | 25 ++++++++++++++++++++++++-
 tools/buildman/buildman.rst | 24 ++++++++++++++++++++++++
 tools/buildman/func_test.py | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/tools/buildman/boards.py b/tools/buildman/boards.py
index 3c2822715f3..9e7b486656b 100644
--- a/tools/buildman/boards.py
+++ b/tools/buildman/boards.py
@@ -19,7 +19,10 @@ import time
 from buildman import board
 from buildman import kconfiglib
 
+from u_boot_pylib import command
 from u_boot_pylib.terminal import print_clear, tprint
+from u_boot_pylib import tools
+from u_boot_pylib import tout
 
 ### constant variables ###
 OUTPUT_FILE = 'boards.cfg'
@@ -202,6 +205,7 @@ class KconfigScanner:
         os.environ['KCONFIG_OBJDIR'] = ''
         self._tmpfile = None
         self._conf = kconfiglib.Kconfig(warn=False)
+        self._srctree = srctree
 
     def __del__(self):
         """Delete a leftover temporary file before exit.
@@ -239,7 +243,26 @@ class KconfigScanner:
         expect_target, match, rear = leaf.partition('_defconfig')
         assert match and not rear, f'{leaf} : invalid defconfig'
 
-        self._conf.load_config(defconfig)
+        temp = None
+        if b'#include' in tools.read_file(defconfig):
+            cmd = [
+                os.getenv('CPP', 'cpp'),
+                '-nostdinc', '-P',
+                '-I', self._srctree,
+                '-undef',
+                '-x', 'assembler-with-cpp',
+                defconfig]
+            result = command.run_pipe([cmd], capture=True, capture_stderr=True)
+            temp = tempfile.NamedTemporaryFile(prefix='buildman-')
+            tools.write_file(temp.name, result.stdout, False)
+            fname = temp.name
+            tout.info(f'Processing #include to produce {defconfig}')
+        else:
+            fname = defconfig
+
+        self._conf.load_config(fname)
+        if temp:
+            del temp
         self._tmpfile = None
 
         params = {}
diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
index e873611e596..8acf236f2b3 100644
--- a/tools/buildman/buildman.rst
+++ b/tools/buildman/buildman.rst
@@ -1112,6 +1112,30 @@ The -U option uses the u-boot.env files which are produced by a build.
 Internally, buildman writes out an out-env file into the build directory for
 later comparison.
 
+defconfig fragments
+-------------------
+
+Buildman provides some initial support for configuration fragments. It can scan
+these when present in defconfig files and handle the resuiting Kconfig
+correctly. Thus it is possible to build a board which has a ``#include`` in the
+defconfig file.
+
+For now, Buildman simply includes the files to produce a single output file,
+using the C preprocessor. It does not call the ``merge_config.sh`` script. The
+redefined/redundant logic in that script could fairly easily be repeated in
+Buildman, to detect potential problems. For now it is not clear that this is
+useful.
+
+To specify the C preprocessor to use, set the ``CPP`` environment variable. The
+default is ``cpp``.
+
+Note that Buildman does not support adding fragments to existing boards, e.g.
+like::
+
+    make qemu_riscv64_defconfig acpi.config
+
+This is partly because there is no way for Buildman to know which fragments are
+valid on which boards.
 
 Building with clang
 -------------------
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index db3c9d63ec4..4e12c671a3d 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -2,8 +2,10 @@
 # Copyright (c) 2014 Google, Inc
 #
 
+import io
 import os
 from pathlib import Path
+import re
 import shutil
 import sys
 import tempfile
@@ -373,6 +375,22 @@ class TestFunctional(unittest.TestCase):
     def _HandleCommandSize(self, args):
         return command.CommandResult(return_code=0)
 
+    def _HandleCommandCpp(self, args):
+        # args ['-nostdinc', '-P', '-I', '/tmp/tmp7f17xk_o/src', '-undef',
+        # '-x', 'assembler-with-cpp', fname]
+        fname = args[7]
+        buf = io.StringIO()
+        for line in tools.read_file(fname, False).splitlines():
+            if line.startswith('#include'):
+                # Example: #include <configs/renesas_rcar2.config>
+                m_incfname = re.match('#include <(.*)>', line)
+                data = tools.read_file(m_incfname.group(1), False)
+                for line in data.splitlines():
+                    print(line, file=buf)
+            else:
+                print(line, file=buf)
+        return command.CommandResult(stdout=buf.getvalue(), return_code=0)
+
     def _HandleCommand(self, **kwargs):
         """Handle a command execution.
 
@@ -406,6 +424,8 @@ class TestFunctional(unittest.TestCase):
             return self._HandleCommandObjcopy(args)
         elif cmd.endswith( 'size'):
             return self._HandleCommandSize(args)
+        elif cmd.endswith( 'cpp'):
+            return self._HandleCommandCpp(args)
 
         if not result:
             # Not handled, so abort
@@ -1118,3 +1138,17 @@ CONFIG_SOC="fred"
             'config': '-',
             'target': 'board0'},
             ['WARNING: board0_defconfig: No TARGET_BOARD0 enabled']), res)
+
+        # check handling of #include files; see _HandleCommandCpp()
+        inc = os.path.join(src, 'common')
+        tools.write_file(inc, b'CONFIG_TARGET_BOARD0=y\n')
+        tools.write_file(norm, f'#include <{inc}>', False)
+        res = scanner.scan(norm, True)
+        self.assertEqual(({
+            'arch': 'arm',
+            'cpu': 'armv7',
+            'soc': '-',
+            'vendor': 'Tester',
+            'board': 'ARM Board 0',
+            'config': 'config0',
+            'target': 'board0'}, []), res)
-- 
2.34.1

Re: [PATCH 3/9] buildman: Support #include files in defconfigs

[Tom Rini]

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

On Fri, Nov 08, 2024 at 08:23:44AM -0700, Simon Glass wrote:

> This is used by some boards in U-Boot and is a convenient way to deal
> with common settings where using a Kconfig files is not desirable.
> 
> Detect #include files and process them as if they were part of the
> original file.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/30
[snip]
> +defconfig fragments
> +-------------------
> +
> +Buildman provides some initial support for configuration fragments. It can scan
> +these when present in defconfig files and handle the resuiting Kconfig
> +correctly. Thus it is possible to build a board which has a ``#include`` in the
> +defconfig file.
> +
> +For now, Buildman simply includes the files to produce a single output file,
> +using the C preprocessor. It does not call the ``merge_config.sh`` script. The
> +redefined/redundant logic in that script could fairly easily be repeated in
> +Buildman, to detect potential problems. For now it is not clear that this is
> +useful.

I don't like this logic because the whole point of merge_config.sh is
that it IS the canonical way to handle Kconfig config files + fragments
and provides handy feedback like "You expected CONFIG_FOO=y but you
ended up with '# CONFIG_FOO is not set'". It's frankly an at least small
problem of our current cpp rule, but calling that for every defconfig
would be a performance nightmare too.

> +To specify the C preprocessor to use, set the ``CPP`` environment variable. The
> +default is ``cpp``.

Uh, I was hoping it would get the correct CPP and flags from the
Makefile? Otherwise this is going to fall down in some corner cases such
as I expect clang.

> +Note that Buildman does not support adding fragments to existing boards, e.g.
> +like::
> +
> +    make qemu_riscv64_defconfig acpi.config
> +
> +This is partly because there is no way for Buildman to know which fragments are
> +valid on which boards.

That seems like a really weird deficiency and non-sequitur. I don't know
why buildman would be attempting any sort of validation beyond syntax
validation. It's more that we don't have any way to pass additional
arguments to the "make defconfig" part of the build, yes? And then in
turn because buildman reads the defconfig itself too, prior to that
stage?

-- 
Tom

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

Re: [PATCH 3/9] buildman: Support #include files in defconfigs

[Simon Glass]

Hi Tom,

On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Nov 08, 2024 at 08:23:44AM -0700, Simon Glass wrote:
>
> > This is used by some boards in U-Boot and is a convenient way to deal
> > with common settings where using a Kconfig files is not desirable.
> >
> > Detect #include files and process them as if they were part of the
> > original file.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/30
> [snip]
> > +defconfig fragments
> > +-------------------
> > +
> > +Buildman provides some initial support for configuration fragments. It can scan
> > +these when present in defconfig files and handle the resuiting Kconfig
> > +correctly. Thus it is possible to build a board which has a ``#include`` in the
> > +defconfig file.
> > +
> > +For now, Buildman simply includes the files to produce a single output file,
> > +using the C preprocessor. It does not call the ``merge_config.sh`` script. The
> > +redefined/redundant logic in that script could fairly easily be repeated in
> > +Buildman, to detect potential problems. For now it is not clear that this is
> > +useful.
>
> I don't like this logic because the whole point of merge_config.sh is
> that it IS the canonical way to handle Kconfig config files + fragments
> and provides handy feedback like "You expected CONFIG_FOO=y but you
> ended up with '# CONFIG_FOO is not set'". It's frankly an at least small
> problem of our current cpp rule, but calling that for every defconfig
> would be a performance nightmare too.

Here is the part of scripts/kconfig/merge_config.sh which actually
does something:

# Merge files, printing warnings on overridden values
for MERGE_FILE in $MERGE_LIST ; do
        echo "Merging $MERGE_FILE"
        if [ ! -r "$MERGE_FILE" ]; then
                echo "The merge file '$MERGE_FILE' does not exist.  Exit." >&2
                exit 1
        fi
        CFG_LIST=$(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2"
$MERGE_FILE)

        for CFG in $CFG_LIST ; do
                grep -q -w $CFG $TMP_FILE || continue
                PREV_VAL=$(grep -w $CFG $TMP_FILE)
                NEW_VAL=$(grep -w $CFG $MERGE_FILE)
                if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
                        echo Value of $CFG is redefined by fragment $MERGE_FILE:
                        echo Previous  value: $PREV_VAL
                        echo New value:       $NEW_VAL
                        echo
                elif [ "$WARNREDUN" = "true" ]; then
                        echo Value of $CFG is redundant by fragment $MERGE_FILE:
                fi
                sed -i "/$CFG[ =]/d" $TMP_FILE
        done
        cat $MERGE_FILE >> $TMP_FILE
done

So for every line in the fragment it greps the file to check for
redefines/redundancies. I feel we can do this in a much more efficient
way in Python.

>
> > +To specify the C preprocessor to use, set the ``CPP`` environment variable. The
> > +default is ``cpp``.
>
> Uh, I was hoping it would get the correct CPP and flags from the
> Makefile? Otherwise this is going to fall down in some corner cases such
> as I expect clang.

I don't want to parse Makefile, if that's what you're suggesting.

It shouldn't really matter which cpp we use. U-Boot seems to use
'$(CC) -E' so we could do the same in buildman, perhaps.

But actually, having thought about this patch a bit, I think the
better thing is to process the #include in buildman, rather than
running the CPP. That way we can avoid people adding #defines in
there, etc. It locks down the format a bit better.

>
> > +Note that Buildman does not support adding fragments to existing boards, e.g.
> > +like::
> > +
> > +    make qemu_riscv64_defconfig acpi.config
> > +
> > +This is partly because there is no way for Buildman to know which fragments are
> > +valid on which boards.
>
> That seems like a really weird deficiency and non-sequitur. I don't know
> why buildman would be attempting any sort of validation beyond syntax
> validation. It's more that we don't have any way to pass additional
> arguments to the "make defconfig" part of the build, yes? And then in
> turn because buildman reads the defconfig itself too, prior to that
> stage?

Yes, everything is seeming weird at the moment for me too.

Buildman needs to know the architecture, so it can select the
toolchain. So it generates a boards.cfg file. It does this using
Kconfiglib, which processes the defconfig along with the Kconfig files
in U-Boot, to produce a final .config - see KconfigScanner

Anyway, this problem[1] is exactly why I complained about fragments a
year or so back[2]. I'm not sure what the disconnect here is. IMO
unless we build boards with the fragments in CI they may stop working
at any time. Therefore IMO we need a file which collects the defconfig
and its fragments, so we can build the valid combination in CI.

What am I missing?

Regards,
Simon

[1] https://lore.kernel.org/u-boot/4d55f212-ee39-4886-9246-3596fc4268f7@gmx.de/
[2] https://lore.kernel.org/u-boot/CAPnjgZ14CUVt=rM943hh9PQUhK9LJDgZYPxUsATYQe3wwOUYqQ@mail.gmail.com/

Re: [PATCH 3/9] buildman: Support #include files in defconfigs

[Tom Rini]

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

On Wed, Nov 13, 2024 at 07:39:37AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Nov 08, 2024 at 08:23:44AM -0700, Simon Glass wrote:
> >
> > > This is used by some boards in U-Boot and is a convenient way to deal
> > > with common settings where using a Kconfig files is not desirable.
> > >
> > > Detect #include files and process them as if they were part of the
> > > original file.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/30
> > [snip]
> > > +defconfig fragments
> > > +-------------------
> > > +
> > > +Buildman provides some initial support for configuration fragments. It can scan
> > > +these when present in defconfig files and handle the resuiting Kconfig
> > > +correctly. Thus it is possible to build a board which has a ``#include`` in the
> > > +defconfig file.
> > > +
> > > +For now, Buildman simply includes the files to produce a single output file,
> > > +using the C preprocessor. It does not call the ``merge_config.sh`` script. The
> > > +redefined/redundant logic in that script could fairly easily be repeated in
> > > +Buildman, to detect potential problems. For now it is not clear that this is
> > > +useful.
> >
> > I don't like this logic because the whole point of merge_config.sh is
> > that it IS the canonical way to handle Kconfig config files + fragments
> > and provides handy feedback like "You expected CONFIG_FOO=y but you
> > ended up with '# CONFIG_FOO is not set'". It's frankly an at least small
> > problem of our current cpp rule, but calling that for every defconfig
> > would be a performance nightmare too.
> 
> Here is the part of scripts/kconfig/merge_config.sh which actually
> does something:
> 
> # Merge files, printing warnings on overridden values
> for MERGE_FILE in $MERGE_LIST ; do
>         echo "Merging $MERGE_FILE"
>         if [ ! -r "$MERGE_FILE" ]; then
>                 echo "The merge file '$MERGE_FILE' does not exist.  Exit." >&2
>                 exit 1
>         fi
>         CFG_LIST=$(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2"
> $MERGE_FILE)
> 
>         for CFG in $CFG_LIST ; do
>                 grep -q -w $CFG $TMP_FILE || continue
>                 PREV_VAL=$(grep -w $CFG $TMP_FILE)
>                 NEW_VAL=$(grep -w $CFG $MERGE_FILE)
>                 if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
>                         echo Value of $CFG is redefined by fragment $MERGE_FILE:
>                         echo Previous  value: $PREV_VAL
>                         echo New value:       $NEW_VAL
>                         echo
>                 elif [ "$WARNREDUN" = "true" ]; then
>                         echo Value of $CFG is redundant by fragment $MERGE_FILE:
>                 fi
>                 sed -i "/$CFG[ =]/d" $TMP_FILE
>         done
>         cat $MERGE_FILE >> $TMP_FILE
> done
> 
> So for every line in the fragment it greps the file to check for
> redefines/redundancies. I feel we can do this in a much more efficient
> way in Python.

I was too literal then. Yes, we should do the above, but in a more
python way.

> > > +To specify the C preprocessor to use, set the ``CPP`` environment variable. The
> > > +default is ``cpp``.
> >
> > Uh, I was hoping it would get the correct CPP and flags from the
> > Makefile? Otherwise this is going to fall down in some corner cases such
> > as I expect clang.
> 
> I don't want to parse Makefile, if that's what you're suggesting.
> 
> It shouldn't really matter which cpp we use. U-Boot seems to use
> '$(CC) -E' so we could do the same in buildman, perhaps.

It doesn't matter up until it matters. In the Linux Kernel:
commit feb843a469fb0ab00d2d23cfb9bcc379791011bb
Author: Masahiro Yamada <masahiroy@kernel.org>
Date:   Sun Apr 9 23:53:57 2023 +0900

    kbuild: add $(CLANG_FLAGS) to KBUILD_CPPFLAGS

is what I was remembering. But it's likely not going to be a problem for
just handling Kconfig fragments (the above failed on parsing out linker
scripts, iirc)

> But actually, having thought about this patch a bit, I think the
> better thing is to process the #include in buildman, rather than
> running the CPP. That way we can avoid people adding #defines in
> there, etc. It locks down the format a bit better.

It's a defined format, and "make foo_defconfig bar.config" must work.

> > > +Note that Buildman does not support adding fragments to existing boards, e.g.
> > > +like::
> > > +
> > > +    make qemu_riscv64_defconfig acpi.config
> > > +
> > > +This is partly because there is no way for Buildman to know which fragments are
> > > +valid on which boards.
> >
> > That seems like a really weird deficiency and non-sequitur. I don't know
> > why buildman would be attempting any sort of validation beyond syntax
> > validation. It's more that we don't have any way to pass additional
> > arguments to the "make defconfig" part of the build, yes? And then in
> > turn because buildman reads the defconfig itself too, prior to that
> > stage?
> 
> Yes, everything is seeming weird at the moment for me too.
> 
> Buildman needs to know the architecture, so it can select the
> toolchain. So it generates a boards.cfg file. It does this using
> Kconfiglib, which processes the defconfig along with the Kconfig files
> in U-Boot, to produce a final .config - see KconfigScanner

Right. And this won't have any idea about the contents of #include
because that's not a normal feature. The Linux Kernel only supports
merging fragments via that script and then dealing with a complete
".config". That's the behavior we need to emulate in order for something
like configs/am68_sk_a72_defconfig work without duplicating entries from
configs/j721s2_evm_a72_defconfig.

> Anyway, this problem[1] is exactly why I complained about fragments a
> year or so back[2]. I'm not sure what the disconnect here is. IMO
> unless we build boards with the fragments in CI they may stop working
> at any time. Therefore IMO we need a file which collects the defconfig
> and its fragments, so we can build the valid combination in CI.
> 
> What am I missing?
> 
> Regards,
> Simon
> 
> [1] https://lore.kernel.org/u-boot/4d55f212-ee39-4886-9246-3596fc4268f7@gmx.de/
> [2] https://lore.kernel.org/u-boot/CAPnjgZ14CUVt=rM943hh9PQUhK9LJDgZYPxUsATYQe3wwOUYqQ@mail.gmail.com/

What you're missing is that the solution is that only
configs/*defconfigs are valid config files for CI to test, and buildman
doesn't support handling #include files correctly yet. It's clunky to
use in that we have to have duplicate information in the combined
defconfig file and so it's non-obvious to fix and I believe discouraging
to more developers.

A CI-only feature of "buildman knows how to combine foo_defconfig +
feature.config" is duplicating the work of "make foo_defconfig
feature.config" that regular developers will use or "make
foo_feature_defconfig" like we have numerous examples of now. Telling
developers they need to update some file to validate their new defconfig
is likely to result in a new and differently perplexing CI error along
the lines of "Why did the test for building every board not count my
board?".

-- 
Tom

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

Re: [PATCH 3/9] buildman: Support #include files in defconfigs

[Simon Glass]

Hi Tom,

On Wed, 13 Nov 2024 at 14:53, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Nov 13, 2024 at 07:39:37AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Nov 08, 2024 at 08:23:44AM -0700, Simon Glass wrote:
> > >
> > > > This is used by some boards in U-Boot and is a convenient way to deal
> > > > with common settings where using a Kconfig files is not desirable.
> > > >
> > > > Detect #include files and process them as if they were part of the
> > > > original file.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/30
> > > [snip]
> > > > +defconfig fragments
> > > > +-------------------
> > > > +
> > > > +Buildman provides some initial support for configuration fragments. It can scan
> > > > +these when present in defconfig files and handle the resuiting Kconfig
> > > > +correctly. Thus it is possible to build a board which has a ``#include`` in the
> > > > +defconfig file.
> > > > +
> > > > +For now, Buildman simply includes the files to produce a single output file,
> > > > +using the C preprocessor. It does not call the ``merge_config.sh`` script. The
> > > > +redefined/redundant logic in that script could fairly easily be repeated in
> > > > +Buildman, to detect potential problems. For now it is not clear that this is
> > > > +useful.
> > >
> > > I don't like this logic because the whole point of merge_config.sh is
> > > that it IS the canonical way to handle Kconfig config files + fragments
> > > and provides handy feedback like "You expected CONFIG_FOO=y but you
> > > ended up with '# CONFIG_FOO is not set'". It's frankly an at least small
> > > problem of our current cpp rule, but calling that for every defconfig
> > > would be a performance nightmare too.
> >
> > Here is the part of scripts/kconfig/merge_config.sh which actually
> > does something:
> >
> > # Merge files, printing warnings on overridden values
> > for MERGE_FILE in $MERGE_LIST ; do
> >         echo "Merging $MERGE_FILE"
> >         if [ ! -r "$MERGE_FILE" ]; then
> >                 echo "The merge file '$MERGE_FILE' does not exist.  Exit." >&2
> >                 exit 1
> >         fi
> >         CFG_LIST=$(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2"
> > $MERGE_FILE)
> >
> >         for CFG in $CFG_LIST ; do
> >                 grep -q -w $CFG $TMP_FILE || continue
> >                 PREV_VAL=$(grep -w $CFG $TMP_FILE)
> >                 NEW_VAL=$(grep -w $CFG $MERGE_FILE)
> >                 if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> >                         echo Value of $CFG is redefined by fragment $MERGE_FILE:
> >                         echo Previous  value: $PREV_VAL
> >                         echo New value:       $NEW_VAL
> >                         echo
> >                 elif [ "$WARNREDUN" = "true" ]; then
> >                         echo Value of $CFG is redundant by fragment $MERGE_FILE:
> >                 fi
> >                 sed -i "/$CFG[ =]/d" $TMP_FILE
> >         done
> >         cat $MERGE_FILE >> $TMP_FILE
> > done
> >
> > So for every line in the fragment it greps the file to check for
> > redefines/redundancies. I feel we can do this in a much more efficient
> > way in Python.
>
> I was too literal then. Yes, we should do the above, but in a more
> python way.

OK good

>
> > > > +To specify the C preprocessor to use, set the ``CPP`` environment variable. The
> > > > +default is ``cpp``.
> > >
> > > Uh, I was hoping it would get the correct CPP and flags from the
> > > Makefile? Otherwise this is going to fall down in some corner cases such
> > > as I expect clang.
> >
> > I don't want to parse Makefile, if that's what you're suggesting.
> >
> > It shouldn't really matter which cpp we use. U-Boot seems to use
> > '$(CC) -E' so we could do the same in buildman, perhaps.
>
> It doesn't matter up until it matters. In the Linux Kernel:
> commit feb843a469fb0ab00d2d23cfb9bcc379791011bb
> Author: Masahiro Yamada <masahiroy@kernel.org>
> Date:   Sun Apr 9 23:53:57 2023 +0900
>
>     kbuild: add $(CLANG_FLAGS) to KBUILD_CPPFLAGS
>
> is what I was remembering. But it's likely not going to be a problem for
> just handling Kconfig fragments (the above failed on parsing out linker
> scripts, iirc)
>
> > But actually, having thought about this patch a bit, I think the
> > better thing is to process the #include in buildman, rather than
> > running the CPP. That way we can avoid people adding #defines in
> > there, etc. It locks down the format a bit better.
>
> It's a defined format, and "make foo_defconfig bar.config" must work.
>
> > > > +Note that Buildman does not support adding fragments to existing boards, e.g.
> > > > +like::
> > > > +
> > > > +    make qemu_riscv64_defconfig acpi.config
> > > > +
> > > > +This is partly because there is no way for Buildman to know which fragments are
> > > > +valid on which boards.
> > >
> > > That seems like a really weird deficiency and non-sequitur. I don't know
> > > why buildman would be attempting any sort of validation beyond syntax
> > > validation. It's more that we don't have any way to pass additional
> > > arguments to the "make defconfig" part of the build, yes? And then in
> > > turn because buildman reads the defconfig itself too, prior to that
> > > stage?
> >
> > Yes, everything is seeming weird at the moment for me too.
> >
> > Buildman needs to know the architecture, so it can select the
> > toolchain. So it generates a boards.cfg file. It does this using
> > Kconfiglib, which processes the defconfig along with the Kconfig files
> > in U-Boot, to produce a final .config - see KconfigScanner
>
> Right. And this won't have any idea about the contents of #include
> because that's not a normal feature. The Linux Kernel only supports
> merging fragments via that script and then dealing with a complete
> ".config". That's the behavior we need to emulate in order for something
> like configs/am68_sk_a72_defconfig work without duplicating entries from
> configs/j721s2_evm_a72_defconfig.
>
> > Anyway, this problem[1] is exactly why I complained about fragments a
> > year or so back[2]. I'm not sure what the disconnect here is. IMO
> > unless we build boards with the fragments in CI they may stop working
> > at any time. Therefore IMO we need a file which collects the defconfig
> > and its fragments, so we can build the valid combination in CI.
> >
> > What am I missing?
> >
> > Regards,
> > Simon
> >
> > [1] https://lore.kernel.org/u-boot/4d55f212-ee39-4886-9246-3596fc4268f7@gmx.de/
> > [2] https://lore.kernel.org/u-boot/CAPnjgZ14CUVt=rM943hh9PQUhK9LJDgZYPxUsATYQe3wwOUYqQ@mail.gmail.com/
>
> What you're missing is that the solution is that only
> configs/*defconfigs are valid config files for CI to test, and buildman
> doesn't support handling #include files correctly yet. It's clunky to
> use in that we have to have duplicate information in the combined
> defconfig file and so it's non-obvious to fix and I believe discouraging
> to more developers.
>
> A CI-only feature of "buildman knows how to combine foo_defconfig +
> feature.config" is duplicating the work of "make foo_defconfig
> feature.config" that regular developers will use or "make
> foo_feature_defconfig" like we have numerous examples of now. Telling
> developers they need to update some file to validate their new defconfig
> is likely to result in a new and differently perplexing CI error along
> the lines of "Why did the test for building every board not count my
> board?".

This patch fixes the 'buildman doesn't support handling #include files
correctly yet' problem. My other patch proposes a way to allow
buildman to build things with fragments. I'm sure there are other
options, but since buildman can potentially build all the boards in
U-Boot, it needs some way to know whether to apply a fragment.

For building a single board (e.g. with --board), buildman could
perhaps allow fragments to be specified?

It seems you are asking people to create boards containing the
required combinations? I see with TI there are two possible fragments.
How do we keep those working in CI? If you are wanting people to
create boards for each (which is fine by me), can we produce an error
when a fragment is not used by any board?

Regards,
Simon

Re: [PATCH 3/9] buildman: Support #include files in defconfigs

[Tom Rini]

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

On Fri, Nov 15, 2024 at 07:26:56AM -0700, Simon Glass wrote:

[snip]
> This patch fixes the 'buildman doesn't support handling #include files
> correctly yet' problem.

This is good, and what I wanted to see. And the final patch in this
series shows that we can indeed trim down many of the current #include
users to be fewer lines, and that things like:
$ cat configs/qemu_arm64_acpi_defconfig
#include <configs/qemu_arm64_defconfig>
#include <board/emulation/configs/acpi.config>

Now work with buildman, too.

> My other patch proposes a way to allow
> buildman to build things with fragments. I'm sure there are other
> options, but since buildman can potentially build all the boards in
> U-Boot, it needs some way to know whether to apply a fragment.

I keep going back to, why does it need this? If it's important enough
for CI, it's now a 2 line (or so) defconfig file. Done. No new code to
maintain. No new jobs to run. And _every_ config doesn't need to be done
in CI, either. We have things like
board/asus/transformer-t20/configs/tf101g.config which just change the
device tree to be used. And once that's upstream, there's no value in CI
building that (there's no value today in CI building that, we don't do
anything with device tree warnings, etc).

> For building a single board (e.g. with --board), buildman could
> perhaps allow fragments to be specified?

On the one hand, yes, it would let me think about changing how I do CI
on hardware to use buildman instead, but on the other hand I don't think
it would make anything easier, so I probably wouldn't.

> It seems you are asking people to create boards containing the
> required combinations?

Yes.

> I see with TI there are two possible fragments.

More than that, if you count cases like
configs/am62x_evm_a53_ethboot_defconfig and
configs/am68_sk_a72_defconfig where the latter is just changing device
tree BUT the desire is for end users to trivially and non-confusingly
know how to build the board, so "am68_sk_a72_defconfig" and not
"j721s2_evm_a72_defconfig am68_sk_a72.config".

> How do we keep those working in CI? If you are wanting people to
> create boards for each (which is fine by me), can we produce an error
> when a fragment is not used by any board?

Not all fragments must be in CI. Not all fragments are "obvious" either.
It's not always clear when a fragment is or is not valid with another
defconfig. Solving these kind of problems seems like it's a high effort
and low return on engineering time compared with "make a defconfig".
Especially now that we can lower the effort bar on making the defconfig
as one tested with "make" will now work with buildman too.

-- 
Tom

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

[PATCH 4/9] buildman: Correct the indentation in the setting-up section

[Simon Glass]

The example settings file should be indented so that it shows correctly
in the output. Fix it.

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

 tools/buildman/buildman.rst | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
index 8acf236f2b3..5eda01fd5e1 100644
--- a/tools/buildman/buildman.rst
+++ b/tools/buildman/buildman.rst
@@ -186,23 +186,22 @@ Setting up
 #. Create ~/.buildman to tell buildman where to find tool chains (see
    buildman_settings_ for details). As an example::
 
-   # Buildman settings file
+      # Buildman settings file
 
-   [toolchain]
-   root: /
-   rest: /toolchains/*
-   eldk: /opt/eldk-4.2
-   arm: /opt/linaro/gcc-linaro-arm-linux-gnueabihf-4.8-2013.08_linux
-   aarch64: /opt/linaro/gcc-linaro-aarch64-none-elf-4.8-2013.10_linux
+      [toolchain]
+      root: /
+      rest: /toolchains/*
+      eldk: /opt/eldk-4.2
+      arm: /opt/linaro/gcc-linaro-arm-linux-gnueabihf-4.8-2013.08_linux
+      aarch64: /opt/linaro/gcc-linaro-aarch64-none-elf-4.8-2013.10_linux
 
-   [toolchain-prefix]
-   arc = /opt/arc/arc_gnu_2021.03_prebuilt_elf32_le_linux_install/bin/arc-elf32-
-
-   [toolchain-alias]
-   riscv = riscv32
-   sh = sh4
-   x86: i386
+      [toolchain-prefix]
+      arc = /opt/arc/arc_gnu_2021.03_prebuilt_elf32_le_linux_install/bin/arc-elf32-
 
+      [toolchain-alias]
+      riscv = riscv32
+      sh = sh4
+      x86: i386
 
    This selects the available toolchain paths. Add the base directory for
    each of your toolchains here. Buildman will search inside these directories
-- 
2.34.1

[PATCH 5/9] buildman: Document the toolchain-prefix section

[Simon Glass]

This is mentioned in examples but should have its own mention in the
'settings' documentation. Add it.

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

 tools/buildman/buildman.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
index 5eda01fd5e1..73a13134a4d 100644
--- a/tools/buildman/buildman.rst
+++ b/tools/buildman/buildman.rst
@@ -933,6 +933,12 @@ a set of (tag, value) pairs.
     For example powerpc-linux-gcc will be noted as a toolchain for 'powerpc'
     and CROSS_COMPILE will be set to powerpc-linux- when using it.
 
+'[toolchain-prefix]' section
+    This can be used to provide the full toolchain-prefix for one or more
+    architectures. The full CROSS_COMPILE prefix must be provided. These
+    typically have a higher priority than matches in the '[toolchain]', due to
+    this prefix.
+
 '[toolchain-alias]' section
     This converts toolchain architecture names to U-Boot names. For example,
     if an x86 toolchains is called i386-linux-gcc it will not normally be
-- 
2.34.1

[PATCH 6/9] buildman: Correct logic for adding a toolchain

[Simon Glass]

If the toolchain is bad, it should not be added. Correct the logic for
this.

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

 tools/buildman/toolchain.py | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py
index 0c8a4fa16eb..dab350fec31 100644
--- a/tools/buildman/toolchain.py
+++ b/tools/buildman/toolchain.py
@@ -327,16 +327,17 @@ class Toolchains:
         toolchain = Toolchain(fname, test, verbose, priority, arch,
                               self.override_toolchain)
         add_it = toolchain.ok
-        if toolchain.arch in self.toolchains:
-            add_it = (toolchain.priority <
-                        self.toolchains[toolchain.arch].priority)
         if add_it:
-            self.toolchains[toolchain.arch] = toolchain
-        elif verbose:
-            print(("Toolchain '%s' at priority %d will be ignored because "
-                   "another toolchain for arch '%s' has priority %d" %
-                   (toolchain.gcc, toolchain.priority, toolchain.arch,
-                    self.toolchains[toolchain.arch].priority)))
+            if toolchain.arch in self.toolchains:
+                add_it = (toolchain.priority <
+                          self.toolchains[toolchain.arch].priority)
+            if add_it:
+                self.toolchains[toolchain.arch] = toolchain
+            elif verbose:
+                print(("Toolchain '%s' at priority %d will be ignored because "
+                       "another toolchain for arch '%s' has priority %d" %
+                       (toolchain.gcc, toolchain.priority, toolchain.arch,
+                        self.toolchains[toolchain.arch].priority)))
 
     def ScanPath(self, path, verbose):
         """Scan a path for a valid toolchain
-- 
2.34.1

[PATCH 7/9] buildman: Support a tilde to represent the home directory

[Simon Glass]

It is convenient to use ~ to represent the home directory in the
settings file. Add support for this.

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

 tools/buildman/buildman.rst |  6 +++++
 tools/buildman/test.py      | 50 +++++++++++++++++++++++++++++++++++++
 tools/buildman/toolchain.py | 28 +++++++++++++--------
 3 files changed, 73 insertions(+), 11 deletions(-)

diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
index 73a13134a4d..924564b5700 100644
--- a/tools/buildman/buildman.rst
+++ b/tools/buildman/buildman.rst
@@ -933,12 +933,18 @@ a set of (tag, value) pairs.
     For example powerpc-linux-gcc will be noted as a toolchain for 'powerpc'
     and CROSS_COMPILE will be set to powerpc-linux- when using it.
 
+    The tilde character ``~`` is supported in paths, to represent the home
+    directory.
+
 '[toolchain-prefix]' section
     This can be used to provide the full toolchain-prefix for one or more
     architectures. The full CROSS_COMPILE prefix must be provided. These
     typically have a higher priority than matches in the '[toolchain]', due to
     this prefix.
 
+    The tilde character ``~`` is supported in paths, to represent the home
+    directory.
+
 '[toolchain-alias]' section
     This converts toolchain architecture names to U-Boot names. For example,
     if an x86 toolchains is called i386-linux-gcc it will not normally be
diff --git a/tools/buildman/test.py b/tools/buildman/test.py
index 15801f6097f..385a34e5254 100644
--- a/tools/buildman/test.py
+++ b/tools/buildman/test.py
@@ -46,6 +46,16 @@ main: /usr/sbin
 wrapper = ccache
 '''
 
+settings_data_homedir = '''
+# Buildman settings file
+
+[toolchain]
+main = ~/mypath
+
+[toolchain-prefix]
+x86 = ~/mypath-x86-
+'''
+
 migration = '''===================== WARNING ======================
 This board does not use CONFIG_DM. CONFIG_DM will be
 compulsory starting with the v2020.01 release.
@@ -1030,6 +1040,46 @@ class TestBuild(unittest.TestCase):
         finally:
             os.environ['PATH'] = old_path
 
+    def testHomedir(self):
+        """Test using ~ in a toolchain or toolchain-prefix section"""
+        # Add some test settings
+        bsettings.setup(None)
+        bsettings.add_file(settings_data_homedir)
+
+        # Set up the toolchains
+        home = os.path.expanduser('~')
+        toolchains = toolchain.Toolchains()
+        toolchains.GetSettings()
+        self.assertEqual([f'{home}/mypath'], toolchains.paths)
+
+        # Check scanning
+        with test_util.capture_sys_output() as (stdout, _):
+            toolchains.Scan(verbose=True, raise_on_error=False)
+        lines = iter(stdout.getvalue().splitlines() + ['##done'])
+        self.assertEqual('Scanning for tool chains', next(lines))
+        self.assertEqual(f"   - scanning prefix '{home}/mypath-x86-'",
+                         next(lines))
+        self.assertEqual(
+            f"Error: No tool chain found for prefix '{home}/mypath-x86-gcc'",
+            next(lines))
+        self.assertEqual(f"   - scanning path '{home}/mypath'", next(lines))
+        self.assertEqual(f"      - looking in '{home}/mypath/.'", next(lines))
+        self.assertEqual(f"      - looking in '{home}/mypath/bin'", next(lines))
+        self.assertEqual(f"      - looking in '{home}/mypath/usr/bin'",
+                         next(lines))
+        self.assertEqual('##done', next(lines))
+
+        # Check adding a toolchain
+        with test_util.capture_sys_output() as (stdout, _):
+            toolchains.Add('~/aarch64-linux-gcc', test=True, verbose=True)
+        lines = iter(stdout.getvalue().splitlines() + ['##done'])
+        self.assertEqual('Tool chain test:  BAD', next(lines))
+        self.assertEqual(f'Command: {home}/aarch64-linux-gcc --version',
+                         next(lines))
+        self.assertEqual('', next(lines))
+        self.assertEqual('', next(lines))
+        self.assertEqual('##done', next(lines))
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py
index dab350fec31..958f36f9f61 100644
--- a/tools/buildman/toolchain.py
+++ b/tools/buildman/toolchain.py
@@ -65,12 +65,13 @@ class Toolchain:
         """Create a new toolchain object.
 
         Args:
-            fname: Filename of the gcc component
+            fname: Filename of the gcc component, possibly with ~ or $HOME in it
             test: True to run the toolchain to test it
             verbose: True to print out the information
             priority: Priority to use for this toolchain, or PRIORITY_CALC to
                 calculate it
         """
+        fname = os.path.expanduser(fname)
         self.gcc = fname
         self.path = os.path.dirname(fname)
         self.override_toolchain = override_toolchain
@@ -109,7 +110,7 @@ class Toolchain:
                                                           self.priority))
                 else:
                     print('BAD')
-                    print('Command: ', cmd)
+                    print(f"Command: {' '.join(cmd)}")
                     print(result.stdout)
                     print(result.stderr)
         else:
@@ -296,10 +297,11 @@ class Toolchains:
 
         paths = []
         for name, value in toolchains:
+            fname = os.path.expanduser(value)
             if '*' in value:
-                paths += glob.glob(value)
+                paths += glob.glob(fname)
             else:
-                paths.append(value)
+                paths.append(fname)
         return paths
 
     def GetSettings(self, show_warning=True):
@@ -373,7 +375,7 @@ class Toolchains:
                 pathname_list.append(pathname)
         return pathname_list
 
-    def Scan(self, verbose):
+    def Scan(self, verbose, raise_on_error=True):
         """Scan for available toolchains and select the best for each arch.
 
         We look for all the toolchains we can file, figure out the
@@ -385,11 +387,12 @@ class Toolchains:
         """
         if verbose: print('Scanning for tool chains')
         for name, value in self.prefixes:
-            if verbose: print("   - scanning prefix '%s'" % value)
-            if os.path.exists(value):
-                self.Add(value, True, verbose, PRIORITY_FULL_PREFIX, name)
+            fname = os.path.expanduser(value)
+            if verbose: print("   - scanning prefix '%s'" % fname)
+            if os.path.exists(fname):
+                self.Add(fname, True, verbose, PRIORITY_FULL_PREFIX, name)
                 continue
-            fname = value + 'gcc'
+            fname += 'gcc'
             if os.path.exists(fname):
                 self.Add(fname, True, verbose, PRIORITY_PREFIX_GCC, name)
                 continue
@@ -397,8 +400,11 @@ class Toolchains:
             for f in fname_list:
                 self.Add(f, True, verbose, PRIORITY_PREFIX_GCC_PATH, name)
             if not fname_list:
-                raise ValueError("No tool chain found for prefix '%s'" %
-                                   value)
+                msg = f"No tool chain found for prefix '{fname}'"
+                if raise_on_error:
+                    raise ValueError(msg)
+                else:
+                    print(f'Error: {msg}')
         for path in self.paths:
             if verbose: print("   - scanning path '%s'" % path)
             fnames = self.ScanPath(path, verbose)
-- 
2.34.1

[PATCH 8/9] buildman: Propose a format for extra boards

[Simon Glass]

It has become more common to use config fragments to extend or adjust
the functionality of boards in U-Boot.

Propose a format for how to deal with this. It is not implemented as
yet.

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

 tools/buildman/buildman.rst | 39 +++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
index 924564b5700..48705d0e49e 100644
--- a/tools/buildman/buildman.rst
+++ b/tools/buildman/buildman.rst
@@ -1148,6 +1148,45 @@ like::
 This is partly because there is no way for Buildman to know which fragments are
 valid on which boards.
 
+Specifying the build matrix with fragments
+------------------------------------------
+
+In order to build boards which can use fragments, Buildman needs to know which
+fragments are valid with which boards. The following scheme is proposed, but not
+currently implemented.
+
+In ``defconfig/``, files with a '.buildman' suffix are used to effectively
+create new boards for Buildman to build. All such files are processed, but it
+might be best to put all the information in a single file for now, e.g.
+``extended.buildman``.
+
+The syntax consists of a number of sections, each introduced by a name. For each
+section the fragment file is named (without the implied ``.config`` suffix),
+then the targets which can accept that fragment are specified, either by their
+board name, with wildcards, or a set of ``CONFIG`` options to check. All
+``CONFIG`` options must match for a board to be included in the set. To specify
+multiple fragments to be included, add them in the order which they should be
+applied, one per line.
+
+For example::
+
+   # Build RISC-V QEMU builds with ACPI
+   name: ACPI with supporting boards
+   fragment: acpi
+   targets:
+     qemu_riscv*
+
+   # Build Android variant of 'k3' boards, with DFU
+   name USB DFU for am62x boards
+   fragment: am62x_r5_usbdfu
+   fragment: am62x_a53_android
+   targets:
+     CONFIG_SYS_SOC="k3"
+
+Buildman normally ignores these files. To request that Buildman process these
+extended new 'boards', use the ``-X / --extend`` option. Note that this may
+significantly increase the number of boards which Buildman builds.
+
 Building with clang
 -------------------
 
-- 
2.34.1

Re: [PATCH 8/9] buildman: Propose a format for extra boards

[Heinrich Schuchardt]

On 08.11.24 16:23, Simon Glass wrote:
> It has become more common to use config fragments to extend or adjust
> the functionality of boards in U-Boot.
> 
> Propose a format for how to deal with this. It is not implemented as
> yet.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>   tools/buildman/buildman.rst | 39 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
> index 924564b5700..48705d0e49e 100644
> --- a/tools/buildman/buildman.rst
> +++ b/tools/buildman/buildman.rst
> @@ -1148,6 +1148,45 @@ like::
>   This is partly because there is no way for Buildman to know which fragments are
>   valid on which boards.
>   
> +Specifying the build matrix with fragments
> +------------------------------------------
> +
> +In order to build boards which can use fragments, Buildman needs to know which
> +fragments are valid with which boards. The following scheme is proposed, but not
> +currently implemented.
> +
> +In ``defconfig/``, files with a '.buildman' suffix are used to effectively
> +create new boards for Buildman to build. All such files are processed, but it
> +might be best to put all the information in a single file for now, e.g.
> +``extended.buildman``.
> +
> +The syntax consists of a number of sections, each introduced by a name. For each
> +section the fragment file is named (without the implied ``.config`` suffix),
> +then the targets which can accept that fragment are specified, either by their
> +board name, with wildcards, or a set of ``CONFIG`` options to check. All
> +``CONFIG`` options must match for a board to be included in the set. To specify
> +multiple fragments to be included, add them in the order which they should be
> +applied, one per line.
> +
> +For example::
> +
> +   # Build RISC-V QEMU builds with ACPI
> +   name: ACPI with supporting boards
> +   fragment: acpi
> +   targets:
> +     qemu_riscv*
> +
> +   # Build Android variant of 'k3' boards, with DFU
> +   name USB DFU for am62x boards
> +   fragment: am62x_r5_usbdfu
> +   fragment: am62x_a53_android
> +   targets:
> +     CONFIG_SYS_SOC="k3"

Thank you for looking into this. I don't think that we will have to 
build every board with each fragment. But for every fragment there 
should be at least one build.

qemu-riscv64_smode_defconfig + acpi.config and qemu_arm_defconfig + 
acpi.config would be enough for acpi.config. x86 anyway uses ACPI.

Best regards

Heinrich

> +
> +Buildman normally ignores these files. To request that Buildman process these
> +extended new 'boards', use the ``-X / --extend`` option. Note that this may
> +significantly increase the number of boards which Buildman builds.
> +
>   Building with clang
>   -------------------
>   

Re: [PATCH 8/9] buildman: Propose a format for extra boards

[Tom Rini]

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

On Fri, Nov 08, 2024 at 08:23:49AM -0700, Simon Glass wrote:

> It has become more common to use config fragments to extend or adjust
> the functionality of boards in U-Boot.
> 
> Propose a format for how to deal with this. It is not implemented as
> yet.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

I think that the first problem is that this patch series is an
inappropriate method and place to start the discussion.

I also think this gets things backwards as the common case is "make",
not "buildman". We need more defconfig's that are just base +
fragment(s) if they're important enough to be a combination that needs
to be tested and work. A board is not a time-expensive part of CI. A
pytest run is, a new job itself is.

-- 
Tom

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

Re: [PATCH 8/9] buildman: Propose a format for extra boards

[Simon Glass]

Hi Tom,

On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Nov 08, 2024 at 08:23:49AM -0700, Simon Glass wrote:
>
> > It has become more common to use config fragments to extend or adjust
> > the functionality of boards in U-Boot.
> >
> > Propose a format for how to deal with this. It is not implemented as
> > yet.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
>
> I think that the first problem is that this patch series is an
> inappropriate method and place to start the discussion.

We had a discussion a year ago but it trailed off.

>
> I also think this gets things backwards as the common case is "make",
> not "buildman". We need more defconfig's that are just base +
> fragment(s) if they're important enough to be a combination that needs
> to be tested and work. A board is not a time-expensive part of CI. A
> pytest run is, a new job itself is.

OK, that would work too. It would also avoid the problem of combinatorial
explosion. But I am not seeing people actually doing that, with rare
exceptions.

Regards,
Simon

Re: [PATCH 8/9] buildman: Propose a format for extra boards

[Tom Rini]

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

On Wed, Nov 13, 2024 at 09:03:35AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Nov 08, 2024 at 08:23:49AM -0700, Simon Glass wrote:
> >
> > > It has become more common to use config fragments to extend or adjust
> > > the functionality of boards in U-Boot.
> > >
> > > Propose a format for how to deal with this. It is not implemented as
> > > yet.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > I think that the first problem is that this patch series is an
> > inappropriate method and place to start the discussion.
> 
> We had a discussion a year ago but it trailed off.

OK. Still an inappropriate place to resurrect it.

> > I also think this gets things backwards as the common case is "make",
> > not "buildman". We need more defconfig's that are just base +
> > fragment(s) if they're important enough to be a combination that needs
> > to be tested and work. A board is not a time-expensive part of CI. A
> > pytest run is, a new job itself is.
> 
> OK, that would work too. It would also avoid the problem of combinatorial
> explosion. But I am not seeing people actually doing that, with rare
> exceptions.

I think it's the exception, not the rule, where config fragments are not
being put to use in a defconfig. And that's possibly because not a lot
of people seem to know about the #include option, and then when I
explain it exists to people the next problem is "Oh, I have to do what
so that buildman also works?".

-- 
Tom

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

Re: [PATCH 8/9] buildman: Propose a format for extra boards

[Simon Glass]

Hi Tom,

On Wed, 13 Nov 2024 at 15:20, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Nov 13, 2024 at 09:03:35AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Nov 08, 2024 at 08:23:49AM -0700, Simon Glass wrote:
> > >
> > > > It has become more common to use config fragments to extend or adjust
> > > > the functionality of boards in U-Boot.
> > > >
> > > > Propose a format for how to deal with this. It is not implemented as
> > > > yet.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > >
> > > I think that the first problem is that this patch series is an
> > > inappropriate method and place to start the discussion.
> >
> > We had a discussion a year ago but it trailed off.
>
> OK. Still an inappropriate place to resurrect it.

What do you suggest? I am trying to solve the problem, not just talk
about it, so patches are often best for that.

>
> > > I also think this gets things backwards as the common case is "make",
> > > not "buildman". We need more defconfig's that are just base +
> > > fragment(s) if they're important enough to be a combination that needs
> > > to be tested and work. A board is not a time-expensive part of CI. A
> > > pytest run is, a new job itself is.
> >
> > OK, that would work too. It would also avoid the problem of combinatorial
> > explosion. But I am not seeing people actually doing that, with rare
> > exceptions.
>
> I think it's the exception, not the rule, where config fragments are not
> being put to use in a defconfig. And that's possibly because not a lot
> of people seem to know about the #include option, and then when I
> explain it exists to people the next problem is "Oh, I have to do what
> so that buildman also works?".

Are you looking for any feature in Buildman for this? This series is
intended to fix [1]

Regards,
Simon

[1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/30

Re: [PATCH 8/9] buildman: Propose a format for extra boards

[Tom Rini]

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

On Fri, Nov 15, 2024 at 06:37:18AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 13 Nov 2024 at 15:20, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Nov 13, 2024 at 09:03:35AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Nov 08, 2024 at 08:23:49AM -0700, Simon Glass wrote:
> > > >
> > > > > It has become more common to use config fragments to extend or adjust
> > > > > the functionality of boards in U-Boot.
> > > > >
> > > > > Propose a format for how to deal with this. It is not implemented as
> > > > > yet.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > I think that the first problem is that this patch series is an
> > > > inappropriate method and place to start the discussion.
> > >
> > > We had a discussion a year ago but it trailed off.
> >
> > OK. Still an inappropriate place to resurrect it.
> 
> What do you suggest? I am trying to solve the problem, not just talk
> about it, so patches are often best for that.

Well, in that there's a problem to be solved, it's that buildman makes
#include more painful that it should be to use.

Or, we validate config fragments by building them. A requires-buildman
solution is not appropriate. That:
https://patchwork.ozlabs.org/project/uboot/patch/20241114115049.2070780-2-patrick.rudolph@9elements.com/
doesn't use the acpi.config fragment is because of the parsing issue
buildman has.

> > > > I also think this gets things backwards as the common case is "make",
> > > > not "buildman". We need more defconfig's that are just base +
> > > > fragment(s) if they're important enough to be a combination that needs
> > > > to be tested and work. A board is not a time-expensive part of CI. A
> > > > pytest run is, a new job itself is.
> > >
> > > OK, that would work too. It would also avoid the problem of combinatorial
> > > explosion. But I am not seeing people actually doing that, with rare
> > > exceptions.
> >
> > I think it's the exception, not the rule, where config fragments are not
> > being put to use in a defconfig. And that's possibly because not a lot
> > of people seem to know about the #include option, and then when I
> > explain it exists to people the next problem is "Oh, I have to do what
> > so that buildman also works?".
> 
> Are you looking for any feature in Buildman for this? This series is
> intended to fix [1]

Yes, the part of this series that, if I read it right, causes buildman
to read the preprocessed config file, rather than the raw defconfig, is
what should fix that, as the final patch in the series confirmed.

-- 
Tom

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

Re: [PATCH 8/9] buildman: Propose a format for extra boards

[Simon Glass]

Hi Tom,

On Fri, 15 Nov 2024 at 07:44, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Nov 15, 2024 at 06:37:18AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 13 Nov 2024 at 15:20, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Nov 13, 2024 at 09:03:35AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Fri, Nov 08, 2024 at 08:23:49AM -0700, Simon Glass wrote:
> > > > >
> > > > > > It has become more common to use config fragments to extend or adjust
> > > > > > the functionality of boards in U-Boot.
> > > > > >
> > > > > > Propose a format for how to deal with this. It is not implemented as
> > > > > > yet.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > I think that the first problem is that this patch series is an
> > > > > inappropriate method and place to start the discussion.
> > > >
> > > > We had a discussion a year ago but it trailed off.
> > >
> > > OK. Still an inappropriate place to resurrect it.
> >
> > What do you suggest? I am trying to solve the problem, not just talk
> > about it, so patches are often best for that.
>
> Well, in that there's a problem to be solved, it's that buildman makes
> #include more painful that it should be to use.
>
> Or, we validate config fragments by building them. A requires-buildman
> solution is not appropriate. That:
> https://patchwork.ozlabs.org/project/uboot/patch/20241114115049.2070780-2-patrick.rudolph@9elements.com/
> doesn't use the acpi.config fragment is because of the parsing issue
> buildman has.
>
> > > > > I also think this gets things backwards as the common case is "make",
> > > > > not "buildman". We need more defconfig's that are just base +
> > > > > fragment(s) if they're important enough to be a combination that needs
> > > > > to be tested and work. A board is not a time-expensive part of CI. A
> > > > > pytest run is, a new job itself is.
> > > >
> > > > OK, that would work too. It would also avoid the problem of combinatorial
> > > > explosion. But I am not seeing people actually doing that, with rare
> > > > exceptions.
> > >
> > > I think it's the exception, not the rule, where config fragments are not
> > > being put to use in a defconfig. And that's possibly because not a lot
> > > of people seem to know about the #include option, and then when I
> > > explain it exists to people the next problem is "Oh, I have to do what
> > > so that buildman also works?".
> >
> > Are you looking for any feature in Buildman for this? This series is
> > intended to fix [1]
>
> Yes, the part of this series that, if I read it right, causes buildman
> to read the preprocessed config file, rather than the raw defconfig, is
> what should fix that, as the final patch in the series confirmed.

Are you saying you want a validation solution that doesn't use
buildman. If so, why? CI relies entirely on buildman for building
things.

The first 7 patches of this series resolve an issue that you filed.
Above you say 'because of the parsing issue that buildman has'.
So...should we fix it?

I don't like having fragments in the tree where we don't (at least
programmatically) know what board they relate to. At least I think it
should be an error to add a fragment with no in-tree defconfig users.

In any case, I think the file format I have come up with is somewhat
reasonable. My main concern with it is that it might greatly increase
the CI load.

Regards,
Simon

Re: [PATCH 8/9] buildman: Propose a format for extra boards

[Tom Rini]

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

On Sun, Nov 17, 2024 at 12:48:14PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 15 Nov 2024 at 07:44, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Nov 15, 2024 at 06:37:18AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 13 Nov 2024 at 15:20, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Nov 13, 2024 at 09:03:35AM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Fri, Nov 08, 2024 at 08:23:49AM -0700, Simon Glass wrote:
> > > > > >
> > > > > > > It has become more common to use config fragments to extend or adjust
> > > > > > > the functionality of boards in U-Boot.
> > > > > > >
> > > > > > > Propose a format for how to deal with this. It is not implemented as
> > > > > > > yet.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > >
> > > > > > I think that the first problem is that this patch series is an
> > > > > > inappropriate method and place to start the discussion.
> > > > >
> > > > > We had a discussion a year ago but it trailed off.
> > > >
> > > > OK. Still an inappropriate place to resurrect it.
> > >
> > > What do you suggest? I am trying to solve the problem, not just talk
> > > about it, so patches are often best for that.
> >
> > Well, in that there's a problem to be solved, it's that buildman makes
> > #include more painful that it should be to use.
> >
> > Or, we validate config fragments by building them. A requires-buildman
> > solution is not appropriate. That:
> > https://patchwork.ozlabs.org/project/uboot/patch/20241114115049.2070780-2-patrick.rudolph@9elements.com/
> > doesn't use the acpi.config fragment is because of the parsing issue
> > buildman has.
> >
> > > > > > I also think this gets things backwards as the common case is "make",
> > > > > > not "buildman". We need more defconfig's that are just base +
> > > > > > fragment(s) if they're important enough to be a combination that needs
> > > > > > to be tested and work. A board is not a time-expensive part of CI. A
> > > > > > pytest run is, a new job itself is.
> > > > >
> > > > > OK, that would work too. It would also avoid the problem of combinatorial
> > > > > explosion. But I am not seeing people actually doing that, with rare
> > > > > exceptions.
> > > >
> > > > I think it's the exception, not the rule, where config fragments are not
> > > > being put to use in a defconfig. And that's possibly because not a lot
> > > > of people seem to know about the #include option, and then when I
> > > > explain it exists to people the next problem is "Oh, I have to do what
> > > > so that buildman also works?".
> > >
> > > Are you looking for any feature in Buildman for this? This series is
> > > intended to fix [1]
> >
> > Yes, the part of this series that, if I read it right, causes buildman
> > to read the preprocessed config file, rather than the raw defconfig, is
> > what should fix that, as the final patch in the series confirmed.
> 
> Are you saying you want a validation solution that doesn't use
> buildman. If so, why? CI relies entirely on buildman for building
> things.

Because (a) fragments should be used as a general rule and (b) it will
introduce a new failure mode for new platforms, platform added with
fragments, whatever-table not updated.

> The first 7 patches of this series resolve an issue that you filed.
> Above you say 'because of the parsing issue that buildman has'.
> So...should we fix it?

Yes, patches 1-7 are good and I'm leaning towards picking them up for
-next soon. They haven't quite had the general 2 week waiting period for
others to comment on.

> I don't like having fragments in the tree where we don't (at least
> programmatically) know what board they relate to. At least I think it
> should be an error to add a fragment with no in-tree defconfig users.

Fragments don't have specific relationships to boards. That's what I
keep saying. And we *intentionall* have fragments without in-tree
defconfig users too, as I spelled out in another part of this thread.

> In any case, I think the file format I have come up with is somewhat
> reasonable. My main concern with it is that it might greatly increase
> the CI load.

Yes, but I don't want the thing you're proposing and don't think it will
have value and that it will make life harder not easier because of
introducing a new CI-only failure mode for submissions.

-- 
Tom

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

[PATCH 9/9] RFC: Show building with #include in defconfig

[Simon Glass]

Do not apply.

Update one board to drop the now-unnecessary settings.

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

 configs/j721e_sk_a72_defconfig | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/configs/j721e_sk_a72_defconfig b/configs/j721e_sk_a72_defconfig
index 80e3e90cafd..f7ddb405ee1 100644
--- a/configs/j721e_sk_a72_defconfig
+++ b/configs/j721e_sk_a72_defconfig
@@ -1,9 +1,4 @@
 #include <configs/j721e_evm_a72_defconfig>
 
-CONFIG_ARM=y
-CONFIG_ARCH_K3=y
-CONFIG_SOC_K3_J721E=y
-CONFIG_TARGET_J721E_A72_EVM=y
-
 CONFIG_DEFAULT_DEVICE_TREE="ti/k3-j721e-sk"
 CONFIG_OF_LIST="ti/k3-j721e-sk"
-- 
2.34.1

Re: (subset) [PATCH 0/9] buildman: Add initial support for config fragments

[Tom Rini]

On Fri, 08 Nov 2024 08:23:41 -0700, Simon Glass wrote:

> This series updates buildman to process #include lines in defconfig
> files. With this, it is no-longer necessary to duplicate lines certain
> lines from the include-file in the defconfig, e.g. CONFIG_ARM and
> CONFIG_SOC_...
> 
> 
> Simon Glass (9):
>   buildman: Add a lower-level test for KconfigScanner
>   buildman: Set up the tout library
>   buildman: Support #include files in defconfigs
>   buildman: Correct the indentation in the setting-up section
>   buildman: Document the toolchain-prefix section
>   buildman: Correct logic for adding a toolchain
>   buildman: Support a tilde to represent the home directory
>   buildman: Propose a format for extra boards
>   RFC: Show building with #include in defconfig
> 
> [...]

Applied to u-boot/next, thanks!

-- 
Tom