[PATCH 0/9] buildman: Add initial support for config fragments
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(-)
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)
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
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)
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?
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=rM943hh9PQUhK9LJDgZYPxUsATYQe3w...
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_FILEdone
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=rM943hh9PQUhK9LJDgZYPxUsATYQe3w...
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?".
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_FILEdone
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_CPPFLAGSis 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=rM943hh9PQUhK9LJDgZYPxUsATYQe3w...
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
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.
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
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
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
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)
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 -------------------
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
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.
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
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?".
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
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-pa... 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.
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-pa... 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
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-pa... 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.
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"
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!
participants (3)
-
Heinrich Schuchardt -
Simon Glass -
Tom Rini