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

Fri Nov 15 15:26:56 CET 2024

Hi Tom,

On Wed, 13 Nov 2024 at 14:53, Tom Rini <trini at 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 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.

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

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