Re: [PATCH v9 03/11] tools: build mkeficapsule with tools-only_defconfig

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

On Wed, Jan 19, 2022 at 05:08:14PM +0100, Heinrich Schuchardt wrote:
> On 1/18/22 05:39, AKASHI Takahiro wrote:
> > Add CONFIG_TOOLS_MKEFICAPSULE. Then we want to always build mkeficapsule
> > if tools-only_defconfig is used.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > ---
> >   configs/tools-only_defconfig | 1 +
> >   tools/Kconfig                | 8 ++++++++
> >   tools/Makefile               | 3 +--
> >   3 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
> > index f482c9a1c1b0..5427797dd4c3 100644
> > --- a/configs/tools-only_defconfig
> > +++ b/configs/tools-only_defconfig
> > @@ -31,3 +31,4 @@ CONFIG_I2C_EDID=y
> >   # CONFIG_VIRTIO_MMIO is not set
> >   # CONFIG_VIRTIO_PCI is not set
> >   # CONFIG_VIRTIO_SANDBOX is not set
> > +CONFIG_TOOLS_MKEFICAPSULE=y
> > diff --git a/tools/Kconfig b/tools/Kconfig
> > index 91ce8ae3e516..117c921da3fe 100644
> > --- a/tools/Kconfig
> > +++ b/tools/Kconfig
> > @@ -90,4 +90,12 @@ config TOOLS_SHA512
> >   	help
> >   	  Enable SHA512 support in the tools builds
> > 
> > +config TOOLS_MKEFICAPSULE
> > +	bool "Build efimkcapsule command"
> > +	default y if EFI_CAPSULE_ON_DISK
> 
> We discussed this with Tom before. Building of tools should not depend
> on board options. Can we make this 'default y'?

No.
I think we have different opinions here.

I think that any of *board* configs should build only all the binaries
that are need to run U-Boot on that board.
For distros' case that you have mentioned before, we should
encourage them to use tools-only_defconfig for packaging U-Boot
host tools rather than using any particular *board* config.

# In either case, the resulting binary, as far as mkeficapsule is
concerned, is the exact same without any dependency of target configs.

> I wonder if a dependency are missing:
> 
> With CONFIG_FIT=n './mkeficapsule' shows the usage with:
> 
>    -f, --fit <fit image>       new FIT image file
> 
> I guess the tool should select:
> 
> CONFIG_FIT
> CONFIG_FIT_SIGNATURE
> 
> And #ifdef CONFIG_FIT_SIGNATURE should be removed in the code.

I'm not sure what your point is.

I believe that what you and Simon demand in building any host tool
is that the binary be the same whatever target configs are enabled
(or disabled).
So showing "-f, --fit <fit image>       new FIT image file" unconditionally
is a natural consequence.

It doesn't make sense that CONFIG_TOOLS_MKEFICAPSULE selects
any of target configs explicitly.

Furthermore, I don't have "#ifdef CONFIG_FIT_SIGNATURE" in mkeficapsule.c.
# there is some trick around CONFIG_FIT_SIGNATURE in tools/Kconfig, though.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +	help
> > +	  This command allows users to create a UEFI capsule file and,
> > +	  optionally sign that file. If you want to enable UEFI capsule
> > +	  update feature on your target, you certainly need this.
> > +
> >   endmenu
> > diff --git a/tools/Makefile b/tools/Makefile
> > index 1763f44cac43..766c0674f4a0 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -238,8 +238,7 @@ hostprogs-$(CONFIG_MIPS) += mips-relocs
> >   hostprogs-$(CONFIG_ASN1_COMPILER)	+= asn1_compiler
> >   HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
> > 
> > -mkeficapsule-objs	:= mkeficapsule.o $(LIBFDT_OBJS)
> > -hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += mkeficapsule
> > +hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
> > 
> >   # We build some files with extra pedantic flags to try to minimize things
> >   # that won't build on some weird host compiler -- though there are lots of
>