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 <tr...@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 <s...@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 <masahi...@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-3596fc426...@gmx.de/
> [2]
> https://lore.kernel.org/u-boot/CAPnjgZ14CUVt=rm943hh9pquhk9ljdgzypxusatyqe3wwou...@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
signature.asc
Description: PGP signature
