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

[Simon Glass]

Hi Tom,

On Tue, 12 Nov 2024 at 19:40, Tom Rini <trini at 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 at 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/