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

[Pieter Smith <pieter@boesman.nl>]

On Wed, Mar 02, 2016 at 07:12:57PM +0100, Arnout Vandecappelle wrote:
>  [Adding other core devs in CC, see the beginning and the end]
> 
> On 03/02/16 08:50, Pieter Smith wrote:
[snip]
> >> BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS)
> > 
> > This will not work. Both the BAREBOX_BUILD_CMDS and BAREBOX_INSTALL_IMAGES_CMDS
> > need parameterization. I can solve this with with:
> > 1. A scaled down version of the above barebox-package function to generate the
> >    boilerplate, or
> > 2. Parameterizable functions and duplicated boilerplate.
> > 
> > IMHO reducing duplication always improves clarity and maintenance, so I would
> > go for option 1.
> > 
> > What has your preference?
> 
>  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.

[snip]

> > I would like to:
> > 1. Ensure that all improvements to barebox propagate to barebox-2 in future.
> > 2. Avoid having something error-prone to resolve the above. IMHO using a review
> >    process to keep two pieces of code in sync is quite error-prone and an
> >    unnecessary cognitive burden on developers.
> 
>  There isn't really much to keep in sync between the two, only when you suddenly
> add _INSTALL_STAGING_CMDS for instance you'd have to update barebox-2.mk to copy
> that as well.
> 
>  There are two reasons why I prefer the copying of variables:
> 
> 1. I think that functions are a bit more difficult to understand.
> 2. I'm considering to add infrastructure that facilitates the pattern of copying
> variables, something like
> 
> $(eval $(call inherit-package,barebox))
>
> 
> which would expand to
> 
> BAREBOX_2_VERSION ?= BAREBOX_VERSION
> BAREBOX_2_SITE ?= BAREBOX_SITE
> ...
> BAREBOX_2_BUILD_CMDS ?= BAREBOX_BUILD_CMDS
> 
> so that in the end the barebox-2.mk does indeed become nothing more than a
> single eval line, and that the same approach can easily be used for other packages.

This would certainly be an elegant improvement! Do any of the other packages
need this? I can help with this if you don't have the time. Off-course I would
rather do this in a later patch-series.

>  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. ;-)

[snip]

- Pieter