[coreboot] filo kconfig patch of libpayload

[Patrick Georgi]

Am 13.07.2010 16:51, schrieb baiyin cai:
> 1) load libpayload kconfig while configuring filo.
> 2) build libpayload before filo building.
> it can be used by : $MAKE LIBCONFIG_PATH=/path/to/libpayload"
> Any suggestion will be welcome. thanks
Marc asked me to take a closer look, as I worked a lot on coreboot
related Makefiles in the past (though not on these) and he mentioned
that you plan to use that work for more libpayload-based payload.

If that is the case, consider pushing the complex and libpayload
specific parts into the libpayload tree, and then use "include" to load
it from the payloads' Makefiles.

But that can (and should) be done in a separate patch, to avoid that you
lose your work to mistakes.

> +ifneq ($(strip $(HAVE_FILO_CONFIG)),)
> +ifneq ($(strip $(HAVE_LIB_CONFIG)),)
>  xconfig: prepare $(objk)/qconf
> +	$(Q)printf "Libpayload config for FILO.\n"
> +	$(Q)mv $(FILO_CONFIG) $(FILO_CONFIG)."temp"
> +	$(Q)mv $(LIB_CONFIG) $(FILO_CONFIG)
> +	$(Q)$(objk)/qconf $(LIBCONFIG_PATH)/Config.in
> +	$(Q)mv $(FILO_CONFIG) $(LIB_CONFIG)
> +	$(Q)printf "Libpayload config done.\n"
> +	$(Q)mv $(FILO_CONFIG)."temp" $(FILO_CONFIG)
>  	$(Q)$(objk)/qconf $(Kconfig)
> +else
> +xconfig: prepare $(objk)/qconf
> +	$(Q)printf "Lost libpayload config file.\n"
> +	$(Q)rm -f $(FILO_CONFIG)
What's the intended behaviour if HAVE_LIB_CONFIG is not set?
Maybe it's better to just $(error ... ) before doing all these nested
ifs and elses? That would spare you the various copies below.

> -ifeq ($(strip $(HAVE_LIBPAYLOAD)),)
> -all:
> -	@printf "\nError: libpayload is not installed!\nexpected: $(LIBPAYLOAD).\n"
> +ifneq ($(strip $(HAVE_LIBPAYLOAD)),)
> +libpayload:
> +	@printf "Found Libpayload $(LIBPAYLOAD).\n"
>  else
> -all: prepare $(obj)/version.h $(TARGET)
> +libpayload: $(src)/$(LIB_CONFIG)
libpayload should be marked as a "phony" target, I think.
Alternatively, libpayload's install target could be changed to install a
certain file last, and then you could rely on its presence to determine
if there's a complete installation.

Also be careful with the actions of the libpayload target: libpayload is
referenced from multiple places, so if you're doing parallel builds
(make -j), make is free to execute this rule parallely multiple times
(not that this makes sense, but it can do so, and it does, sometimes).
Whatever you do here must be stable against races in such situations.


Apart from these little issues, I think your patch is a real improvement
for simplifying the build of a complete image. Thank you!

With the above taken into account, this is
Acked-by: Patrick Georgi <patrick.georgi at coresystems.de>

Patrick