[Buildroot] [v3, 1/4] barebox: prepare for secondary config build

[Arnout Vandecappelle <arnout@mind.be>]

On 03/05/16 14:16, Pieter Smith wrote:
> On Wed, Mar 02, 2016 at 10:32:56PM +0100, Pieter Smith wrote:
>> On Wed, Mar 02, 2016 at 07:12:57PM +0100, Arnout Vandecappelle wrote:
> 
> [snip]
> 
>>>  The BUILD_CMDS can be parameterized without using functions, by using
>>> $($(PKG)_...) variables instead of $(BAREBOX_2_...). That's what I tried to show
>>> in my first reply:
>>
>> Off course, silly me... I've been making use of this since forever. It works
>> because of the eval in $(eval $(kconfig-package)). I'll do it this way.
>>  
>>> define BAREBOX_BUILD_CMDS
>>> 	$($(PKG)_BUILD_BAREBOXENV_CMDS)
>>> 	$(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D)
>>> 	$($(PKG)_BUILD_CUSTOM_ENV)
>>> endef
>>> BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS)
>>>
>>> BAREBOX_2_BUILD_BAREBOXENV_CMDS and BAREBOX_2_BUILD_CUSTOM_ENV will not be set,
>>> so those parts are removed. The rest should be identical for barebox-2, but if
>>> you do need something else you can add something like $($(PKG)_EXTRA_FLAGS).
>>
>> I agree with not needing the additional BAREBOX_2_BUILD_BAREBOXENV_CMDS and
>> BAREBOX_2_INSTALL_BAREBOXENV_CMDS (why would you need to install bareboxenv to
>> the rootfs twice), but I would like to keep the BAREBOX_2_BUILD_CUSTOM_ENV and
>> the built-in variant in Yegor's pending patch-set. They allow customization of
>> the barebox environment, and therefore the boot behavior.
> 
> I am having some trouble with this. I am not able to handle ifdef-space
> diversity in this way. Ifdef-space diversity is used to determine how barebox
> should be configured, whether / how the barebox environment should be built,
> and to print user-friendly config errors. The ifdef logic is not all trivial
> (almost half of the makefile logic), so I would like to avoid duplicating it
> between barebox and barebox-2. The only way I know of to avoid this duplication
> is by extracting the logic info a definition and using $(eval), which is
> exactly what you want to avoid.

 Ah yes, I didn't realize that. Indeed, for those things there is no way to
handle them with $(PKG).

 Actually I would like those things to move to kconfig-package, but that would
be dragging things a bit too far for this series.

 So yes, your idea of $(call barebox-package) looks good. One small remark that
you can already fix before submitting: in the patch that adds the
barebox-package function, say explicitly in the commit message if anything has
changed except for $ -> $$ and BAREBOX -> $(1). That'll make review easier.

 Regards,
 Arnout

> 
> Also, for two of the ifdef-space diversity sections, $(PKG) seems to be
> undefined at the time of evaluation. Getting to the bottom of this is proving
> quite taxing. I can get around this by doing an `$(eval $(call ...,BAREBOX))`,
> which looks a lot like my initial suggestion.
> 
> I am getting to the point where I am just going to duplicate the ifdef sections
> and leave the consistency burden with future reviewers. I understand your
> concern that some developers do not understanding $(eval) all that well, but
> this is like trying to solve the problem with my shoelaces tied.
> 
> [snip]
>  
>>>  However, this is just my opinion, other developers may see it differently,
>>> hence the CC.
>>
>> Off course. Your opinion is held in rather high regard, so I would like to have
>> at least one of the regular barebox developers on board. ;-)
> 
> Is there anyone else we can prod to weigh in on this? Please have a look at the
> templated proposal and confirm your opinion on $(eval):
> https://github.com/smipi1/bbb_buildroot/tree/barebox_2nd_config_build-v4-templated.
> 
> [snip]
> 
> - Pieter
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF