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

Wed Nov 13 22:53:24 CET 2024

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

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241113/b8c18aa8/attachment.sig>