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