hi patrick,<br>   thanks for your kindly suggestions. That's will be much helpful for me. I would take<br>that into consideration.<br><br>Signed-off-by: Cai Bai Yin <<a href="mailto:caibaiyin.pku@gmail.com">caibaiyin.pku@gmail.com</a>><br clear="all">
<font color="#888888"><br></font><div class="gmail_quote">2010/7/14 Patrick Georgi <span dir="ltr"><<a href="mailto:patrick@georgi-clan.de">patrick@georgi-clan.de</a>></span><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Am 13.07.2010 16:51, schrieb baiyin cai:<br>
<div class="im">> 1) load libpayload kconfig while configuring filo.<br>
> 2) build libpayload before filo building.<br>
> it can be used by : $MAKE LIBCONFIG_PATH=/path/to/libpayload"<br>
> Any suggestion will be welcome. thanks<br>
</div>Marc asked me to take a closer look, as I worked a lot on coreboot<br>
related Makefiles in the past (though not on these) and he mentioned<br>
that you plan to use that work for more libpayload-based payload.<br>
<br>
If that is the case, consider pushing the complex and libpayload<br>
specific parts into the libpayload tree, and then use "include" to load<br>
it from the payloads' Makefiles.<br>
<br>
But that can (and should) be done in a separate patch, to avoid that you<br>
lose your work to mistakes.<br>
<br>
> +ifneq ($(strip $(HAVE_FILO_CONFIG)),)<br>
> +ifneq ($(strip $(HAVE_LIB_CONFIG)),)<br>
>  xconfig: prepare $(objk)/qconf<br>
> +     $(Q)printf "Libpayload config for FILO.\n"<br>
> +     $(Q)mv $(FILO_CONFIG) $(FILO_CONFIG)."temp"<br>
> +     $(Q)mv $(LIB_CONFIG) $(FILO_CONFIG)<br>
> +     $(Q)$(objk)/qconf $(LIBCONFIG_PATH)/Config.in<br>
> +     $(Q)mv $(FILO_CONFIG) $(LIB_CONFIG)<br>
> +     $(Q)printf "Libpayload config done.\n"<br>
> +     $(Q)mv $(FILO_CONFIG)."temp" $(FILO_CONFIG)<br>
>       $(Q)$(objk)/qconf $(Kconfig)<br>
> +else<br>
> +xconfig: prepare $(objk)/qconf<br>
> +     $(Q)printf "Lost libpayload config file.\n"<br>
> +     $(Q)rm -f $(FILO_CONFIG)<br>
What's the intended behaviour if HAVE_LIB_CONFIG is not set?<br>
Maybe it's better to just $(error ... ) before doing all these nested<br>
ifs and elses? That would spare you the various copies below.<br>
<br>
> -ifeq ($(strip $(HAVE_LIBPAYLOAD)),)<br>
> -all:<br>
> -     @printf "\nError: libpayload is not installed!\nexpected: $(LIBPAYLOAD).\n"<br>
> +ifneq ($(strip $(HAVE_LIBPAYLOAD)),)<br>
> +libpayload:<br>
> +     @printf "Found Libpayload $(LIBPAYLOAD).\n"<br>
>  else<br>
> -all: prepare $(obj)/version.h $(TARGET)<br>
> +libpayload: $(src)/$(LIB_CONFIG)<br>
libpayload should be marked as a "phony" target, I think.<br>
Alternatively, libpayload's install target could be changed to install a<br>
certain file last, and then you could rely on its presence to determine<br>
if there's a complete installation.<br>
<br>
Also be careful with the actions of the libpayload target: libpayload is<br>
referenced from multiple places, so if you're doing parallel builds<br>
(make -j), make is free to execute this rule parallely multiple times<br>
(not that this makes sense, but it can do so, and it does, sometimes).<br>
Whatever you do here must be stable against races in such situations.<br>
<br>
<br>
Apart from these little issues, I think your patch is a real improvement<br>
for simplifying the build of a complete image. Thank you!<br>
<br>
With the above taken into account, this is<br>
Acked-by: Patrick Georgi <<a href="mailto:patrick.georgi@coresystems.de">patrick.georgi@coresystems.de</a>><br>
<font color="#888888"><br>
Patrick<br>
</font></blockquote></div><br>