| Message ID | 1458513351-6556-2-git-send-email-pieter@boesman.nl |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Mar 20, 2016 at 11:35 PM, Pieter Smith <pieter@boesman.nl> wrote: > Support selection of the built image filename in a multi-image barebox build. > This makes it possible to specify which image to pick in a in a multi-image you seem to have double "in a" > barebox config such as the am335x_defconfig. > > The specified image can either be located in the barebox build directory or the > barebox build images directory. > > For compatibility with contemporary barbox builds, a sane default has been > chosen. For barebox versions older than 2012.10, the user will have to specify > 'barebox.bin'. > > Signed-off-by: Pieter Smith <pieter@boesman.nl> Reviewed-by: Yegor Yefremov <yegorslists@googlemail.com> > --- > boot/barebox/Config.in | 9 +++++++++ > boot/barebox/barebox.mk | 9 ++++++--- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/boot/barebox/Config.in b/boot/barebox/Config.in > index bbe6ae2..4f6872c 100644 > --- a/boot/barebox/Config.in > +++ b/boot/barebox/Config.in > @@ -97,6 +97,15 @@ config BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES > A space-separated list of configuration fragment files, > that will be merged to the main Barebox configuration file. > > +config BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE > + string "Built image filename" > + default "barebox-flash-image" > + help > + Name of the built barebox image filename in the barebox build or > + build images directory. > + > + Set to barebox.bin for barebox versions older than 2012.10. > + > config BR2_TARGET_BAREBOX_BAREBOXENV > bool "bareboxenv tool in target" > help > diff --git a/boot/barebox/barebox.mk b/boot/barebox/barebox.mk > index 7715daf..2a3b724 100644 > --- a/boot/barebox/barebox.mk > +++ b/boot/barebox/barebox.mk > @@ -92,10 +92,10 @@ define BAREBOX_BUILD_CMDS > endef > > define BAREBOX_INSTALL_IMAGES_CMDS > - if test -h $(@D)/barebox-flash-image ; then \ > - cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ > + if test -e $(@D)/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)); then \ > + cp -L $(@D)/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)) $(BINARIES_DIR)/barebox.bin ; \ > else \ > - cp $(@D)/barebox.bin $(BINARIES_DIR);\ > + cp $(@D)/images/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)) $(BINARIES_DIR)/barebox.bin ; \ > fi > $(BAREBOX_INSTALL_CUSTOM_ENV) > endef > @@ -115,6 +115,9 @@ ifeq ($(BR2_TARGET_BAREBOX)$(BR_BUILDING),yy) > ifeq ($(or $(BAREBOX_KCONFIG_FILE),$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))),) > $(error No Barebox config. Check your BR2_TARGET_BAREBOX_BOARD_DEFCONFIG or BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE settings) > endif > +ifndef BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE > +$(error No barebox built image filename specified. Check your BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE setting) > +endif > endif > > $(eval $(kconfig-package)) > -- > 2.5.0 >
Hello, First of all, thanks a lot for keeping up the work on this topic! On Sun, 20 Mar 2016 23:35:45 +0100, Pieter Smith wrote: > define BAREBOX_INSTALL_IMAGES_CMDS > - if test -h $(@D)/barebox-flash-image ; then \ > - cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ > + if test -e $(@D)/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)); then \ > + cp -L $(@D)/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)) $(BINARIES_DIR)/barebox.bin ; \ > else \ > - cp $(@D)/barebox.bin $(BINARIES_DIR);\ > + cp $(@D)/images/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)) $(BINARIES_DIR)/barebox.bin ; \ > fi > $(BAREBOX_INSTALL_CUSTOM_ENV) I think I don't like two things here: 1/ That it potentially breaks existing configurations, where barebox.bin was used. 2/ That is automatically searches in images/, which this could be made explicit rather than implicit. So, what about changing this to: 1/ Have an option that defaults to empty, and if it's empty, preserves the current behavior. 2/ Change the code in the .mk file to do: BAREBOX_IMAGE_FILE = $(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE) ifeq ($(BAREBOX_IMAGE_FILE),) define BARBEOX_INSTALL_IMAGE # old code here, using barebox-flash-image, falling back to barebox.bin endef else define BAREBOX_INSTALL_IMAGE # install BAREBOX_IMAGE_FILE endef endif and that's it? Alternatively, you could do that with shell conditionals only, testing is BAREBOX_IMAGE_FILE is empty or not. This may be even easier to read: if test -n $(BAREBOX_IMAGE) ; then install $(BAREBOX_IMAGE) elif test -h $(@D)/barebox-flash-image ; then cp -L $(@D)/barebox-flash-image ... else cp $(@D)/barebox.bin ... fi Thanks! Thomas
On 04/02/16 17:31, Thomas Petazzoni wrote: [snip] > Alternatively, you could do that with shell conditionals only, testing > is BAREBOX_IMAGE_FILE is empty or not. This may be even easier to read: > > if test -n $(BAREBOX_IMAGE) ; then > install $(BAREBOX_IMAGE) > elif test -h $(@D)/barebox-flash-image ; then > cp -L $(@D)/barebox-flash-image ... > else > cp $(@D)/barebox.bin ... > fi +1 to that. Regards, Arnout
On 03/20/16 23:35, Pieter Smith wrote: > Support selection of the built image filename in a multi-image barebox build. > This makes it possible to specify which image to pick in a in a multi-image > barebox config such as the am335x_defconfig. > > The specified image can either be located in the barebox build directory or the > barebox build images directory. > > For compatibility with contemporary barbox builds, a sane default has been > chosen. For barebox versions older than 2012.10, the user will have to specify > 'barebox.bin'. > > Signed-off-by: Pieter Smith <pieter@boesman.nl> > --- > boot/barebox/Config.in | 9 +++++++++ > boot/barebox/barebox.mk | 9 ++++++--- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/boot/barebox/Config.in b/boot/barebox/Config.in > index bbe6ae2..4f6872c 100644 > --- a/boot/barebox/Config.in > +++ b/boot/barebox/Config.in > @@ -97,6 +97,15 @@ config BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES > A space-separated list of configuration fragment files, > that will be merged to the main Barebox configuration file. > > +config BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE > + string "Built image filename" I think the 'built' part is redundant. For example, the kernel just has BR2_LINUX_KERNEL_IMAGE_NAME with prompt "Kernel image name", so I'd go for BR2_TARGET_BAREBOX_IMAGE_NAME and "Barebox image name". > + default "barebox-flash-image" > + help > + Name of the built barebox image filename in the barebox build or > + build images directory. > + > + Set to barebox.bin for barebox versions older than 2012.10. With Thomas's suggestion, the help text could become The filename of the built barebox image. For current barebox, this is usually barebox-flash-image; for barebox versions older than 2012.10, it is barebox.bin. Leave empty to try these two options. This option only needs to be set for multi-image barebox configs. This file will be copied to the images directory as 'barebox.bin'. > + > config BR2_TARGET_BAREBOX_BAREBOXENV > bool "bareboxenv tool in target" > help > diff --git a/boot/barebox/barebox.mk b/boot/barebox/barebox.mk > index 7715daf..2a3b724 100644 > --- a/boot/barebox/barebox.mk > +++ b/boot/barebox/barebox.mk > @@ -92,10 +92,10 @@ define BAREBOX_BUILD_CMDS > endef > > define BAREBOX_INSTALL_IMAGES_CMDS > - if test -h $(@D)/barebox-flash-image ; then \ > - cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ > + if test -e $(@D)/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)); then \ > + cp -L $(@D)/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)) $(BINARIES_DIR)/barebox.bin ; \ > else \ > - cp $(@D)/barebox.bin $(BINARIES_DIR);\ > + cp $(@D)/images/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)) $(BINARIES_DIR)/barebox.bin ; \ > fi > $(BAREBOX_INSTALL_CUSTOM_ENV) > endef > @@ -115,6 +115,9 @@ ifeq ($(BR2_TARGET_BAREBOX)$(BR_BUILDING),yy) > ifeq ($(or $(BAREBOX_KCONFIG_FILE),$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))),) > $(error No Barebox config. Check your BR2_TARGET_BAREBOX_BOARD_DEFCONFIG or BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE settings) > endif > +ifndef BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE > +$(error No barebox built image filename specified. Check your BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE setting) > +endif With Thomas's suggestion, this check should be removed. Regards, Arnout > endif > > $(eval $(kconfig-package)) >
On Thu, Mar 31, 2016 at 07:44:26AM +0200, Yegor Yefremov wrote: > On Sun, Mar 20, 2016 at 11:35 PM, Pieter Smith <pieter@boesman.nl> wrote: > > Support selection of the built image filename in a multi-image barebox build. > > This makes it possible to specify which image to pick in a in a multi-image > > you seem to have double "in a" ACK. Will be fixed in v5. [snip] - Pieter
diff --git a/boot/barebox/Config.in b/boot/barebox/Config.in index bbe6ae2..4f6872c 100644 --- a/boot/barebox/Config.in +++ b/boot/barebox/Config.in @@ -97,6 +97,15 @@ config BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES A space-separated list of configuration fragment files, that will be merged to the main Barebox configuration file. +config BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE + string "Built image filename" + default "barebox-flash-image" + help + Name of the built barebox image filename in the barebox build or + build images directory. + + Set to barebox.bin for barebox versions older than 2012.10. + config BR2_TARGET_BAREBOX_BAREBOXENV bool "bareboxenv tool in target" help diff --git a/boot/barebox/barebox.mk b/boot/barebox/barebox.mk index 7715daf..2a3b724 100644 --- a/boot/barebox/barebox.mk +++ b/boot/barebox/barebox.mk @@ -92,10 +92,10 @@ define BAREBOX_BUILD_CMDS endef define BAREBOX_INSTALL_IMAGES_CMDS - if test -h $(@D)/barebox-flash-image ; then \ - cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ + if test -e $(@D)/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)); then \ + cp -L $(@D)/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)) $(BINARIES_DIR)/barebox.bin ; \ else \ - cp $(@D)/barebox.bin $(BINARIES_DIR);\ + cp $(@D)/images/$(call qstrip,$(BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE)) $(BINARIES_DIR)/barebox.bin ; \ fi $(BAREBOX_INSTALL_CUSTOM_ENV) endef @@ -115,6 +115,9 @@ ifeq ($(BR2_TARGET_BAREBOX)$(BR_BUILDING),yy) ifeq ($(or $(BAREBOX_KCONFIG_FILE),$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))),) $(error No Barebox config. Check your BR2_TARGET_BAREBOX_BOARD_DEFCONFIG or BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE settings) endif +ifndef BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE +$(error No barebox built image filename specified. Check your BR2_TARGET_BAREBOX_BUILT_IMAGE_FILE setting) +endif endif $(eval $(kconfig-package))
Support selection of the built image filename in a multi-image barebox build. This makes it possible to specify which image to pick in a in a multi-image barebox config such as the am335x_defconfig. The specified image can either be located in the barebox build directory or the barebox build images directory. For compatibility with contemporary barbox builds, a sane default has been chosen. For barebox versions older than 2012.10, the user will have to specify 'barebox.bin'. Signed-off-by: Pieter Smith <pieter@boesman.nl> --- boot/barebox/Config.in | 9 +++++++++ boot/barebox/barebox.mk | 9 ++++++--- 2 files changed, 15 insertions(+), 3 deletions(-)