[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Steinhilber, Markus]

Until now barebox images are copied to the output/images directory with their default name. This is a problem if the barebox and barebox-aux images have the same name. Also, you need to rename the images in a post-build script if you need certain file names.

With this patch there is a new option in the barebox and barebox-aux config, which allows you to copy the image files with target names of your choice. See the help text of the option for a more detailed explanation.

Signed-off-by: Markus Steinhilber <markus.steinhilber@erbe-med.com>
---
 boot/barebox/barebox-aux/Config.in | 18 ++++++++++++++++++
 boot/barebox/barebox.mk            | 23 ++++++++++++++++++++---
 boot/barebox/barebox/Config.in     | 18 ++++++++++++++++++
 3 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/boot/barebox/barebox-aux/Config.in b/boot/barebox/barebox-aux/Config.in
index d39d24f763..11e595241a 100644
--- a/boot/barebox/barebox-aux/Config.in
+++ b/boot/barebox/barebox-aux/Config.in
@@ -39,6 +39,24 @@ config BR2_TARGET_BAREBOX_AUX_IMAGE_FILE
          - barebox.bin for barebox versions older than 2012.10.
          - barebox-flash-image for later versions.

+config BR2_TARGET_BAREBOX_AUX_IMAGE_FILE_TARGETS
+       string "Image file copy target names"
+       help
+         Space-separated list of target file names used when copying
+         the image files from BR2_TARGET_BAREBOX_IMAGE_FILE to the
+         images directory.
+
+         The target names are applied in their order in the list. So the
+         first file in BR2_TARGET_BAREBOX_IMAGE_FILE is named after the
+         first entry in this list and so on.
+
+         If left empty or the number of names is smaller than the number
+         of files in BR2_TARGET_BAREBOX_IMAGE_FILE the original file
+         name is kept.
+
+         If BR2_TARGET_BAREBOX_IMAGE_FILE is left empty, the default file
+         is named after the first entry in this list.
+
 config BR2_TARGET_BAREBOX_AUX_CUSTOM_ENV
        bool "Generate an environment image"
        help
diff --git a/boot/barebox/barebox.mk b/boot/barebox/barebox.mk index 9e8a9f67b5..2fb547c58d 100644
--- a/boot/barebox/barebox.mk
+++ b/boot/barebox/barebox.mk
@@ -110,14 +110,31 @@ define $(1)_BUILD_CMDS  endef

 $(1)_IMAGE_FILES = $$(call qstrip,$$(BR2_TARGET_$(1)_IMAGE_FILE))
+$(1)_IMAGE_FILES_TARGETS = $$(call
+qstrip,$$(BR2_TARGET_$(1)_IMAGE_FILE_TARGETS))

 define $(1)_INSTALL_IMAGES_CMDS
+       image_files_array=($$($(1)_IMAGE_FILES)); \
+       image_targets_array=($$($(1)_IMAGE_FILES_TARGETS)); \
        if test -n "$$($(1)_IMAGE_FILES)"; then \
-               cp -L $$(foreach image,$$($(1)_IMAGE_FILES),$$(@D)/$$(image)) $$(BINARIES_DIR) ; \
+               if test -n "$$($(1)_IMAGE_FILES_TARGETS)"; then \
+                       for index in $$$${!image_files_array[*]}; do \
+                               cp -L $$(@D)/$$$${image_files_array[$$$$index]} $$(BINARIES_DIR)/$$$${image_targets_array[$$$$index]}; \
+                       done; \
+               else \
+                       cp -L $$(foreach image,$$($(1)_IMAGE_FILES),$$(@D)/$$(image)) $$(BINARIES_DIR) ; \
+               fi \
        elif test -h $$(@D)/barebox-flash-image ; then \
-               cp -L $$(@D)/barebox-flash-image $$(BINARIES_DIR)/barebox.bin ; \
+               if test -n "$$($(1)_IMAGE_FILES_TARGETS)"; then \
+                       cp -L $$(@D)/barebox-flash-image $$(BINARIES_DIR)/$$$${image_targets_array[0]} ; \
+               else \
+                       cp -L $$(@D)/barebox-flash-image $$(BINARIES_DIR)/barebox.bin ; \
+               fi \
        else \
-               cp $$(@D)/barebox.bin $$(BINARIES_DIR);\
+               if test -n "$$($(1)_IMAGE_FILES_TARGETS)"; then \
+                       cp $$(@D)/barebox.bin $$(BINARIES_DIR)/$$$${image_targets_array[0]};\
+               else \
+                       cp $$(@D)/barebox.bin $$(BINARIES_DIR);\
+               fi \
        fi
        $$($(1)_INSTALL_CUSTOM_ENV)
 endef
diff --git a/boot/barebox/barebox/Config.in b/boot/barebox/barebox/Config.in index 958e294e40..0096695ea5 100644
--- a/boot/barebox/barebox/Config.in
+++ b/boot/barebox/barebox/Config.in
@@ -39,6 +39,24 @@ config BR2_TARGET_BAREBOX_IMAGE_FILE
          - barebox.bin for barebox versions older than 2012.10.
          - barebox-flash-image for later versions.

+config BR2_TARGET_BAREBOX_IMAGE_FILE_TARGETS
+       string "Image file copy target names"
+       help
+         Space-separated list of target file names used when copying
+         the image files from BR2_TARGET_BAREBOX_IMAGE_FILE to the
+         images directory.
+
+         The target names are applied in their order in the list. So the
+         first file in BR2_TARGET_BAREBOX_IMAGE_FILE is named after the
+         first entry in this list and so on.
+
+         If left empty or the number of names is smaller than the number
+         of files in BR2_TARGET_BAREBOX_IMAGE_FILE the original file
+         name is kept.
+
+         If BR2_TARGET_BAREBOX_IMAGE_FILE is left empty, the default file
+         is named after the first entry in this list.
+
 config BR2_TARGET_BAREBOX_BAREBOXENV
        bool "bareboxenv tool in target"
        help
--
2.11.0

________________________________

Erbe Elektromedizin GmbH Firmensitz: 72072 Tuebingen Geschaeftsfuehrer: Christian O. Erbe, Reiner Thede Registergericht: Stuttgart HRB 380137

[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Thomas Petazzoni]

Hello Markus,

On Mon, 18 Feb 2019 13:59:42 +0000
"Steinhilber, Markus" <Markus.Steinhilber@erbe-med.com> wrote:

> Until now barebox images are copied to the output/images directory
> with their default name. This is a problem if the barebox and
> barebox-aux images have the same name.

Could you describe a specific case/situation where this would happen ?
The main reason we have barebox vs. barebox-aux is because you
sometimes need to build a full-blown Barebox, and a smaller Barebox
that serves a first stage bootloader. From what I remember the
generated images had different names, don't they ?

> Also, you need to rename the images in a post-build script if you need
> certain file names.

That is a pretty normal thing in Buildroot. We just install things as
they are installed by the upstream build system, and leave it up to
custom post-build script to further rename/move/adjust to match the
specific requirements of the project/user.

> +config BR2_TARGET_BAREBOX_AUX_IMAGE_FILE_TARGETS
> +       string "Image file copy target names"
> +       help
> +         Space-separated list of target file names used when copying
> +         the image files from BR2_TARGET_BAREBOX_IMAGE_FILE to the
> +         images directory.
> +
> +         The target names are applied in their order in the list. So the
> +         first file in BR2_TARGET_BAREBOX_IMAGE_FILE is named after the
> +         first entry in this list and so on.

I don't think there is any other option in Buildroot where we have two
space-separated lists where there is a mapping between each item in one
list with the corresponding item in the other list. It's a pretty weird
semantic.

Perhaps a less weird semantic (which would also simplify the
implementation I believe) would be to have:

	name1:target-name1 name2:target-name2

in the existing BR2_TARGET_BAREBOX_IMAGE_FILE option.

Then you can do:

	$(foreach f,$(BR2_TARGET_BAREBOX_IMAGE_FILE),
		$(if $(findstring,:,$(f),
			cp $(@D)/$(word 0,$(subst :,$(space),$(f)) $(BINARIES_DIR)/$(word 1,$(subst :,$(space),$(f),
			cp $(@D)/$(f) $(BINARIES_DIR)/$(f)
		)
	)

Well, it's not that simple and of course completely untested :) And
perhaps it doesn't fit well with the existing shell code in barebox.mk.

>  define $(1)_INSTALL_IMAGES_CMDS
> +       image_files_array=($$($(1)_IMAGE_FILES)); \
> +       image_targets_array=($$($(1)_IMAGE_FILES_TARGETS)); \

We don't use bash arrays anywhere else in the make code in Buildroot,
so this would also be a precedent.

Best regardsn

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Steinhilber, Markus]

Hello Thomas,

>> Until now barebox images are copied to the output/images directory
>> with their default name. This is a problem if the barebox and
>> barebox-aux images have the same name.
> Could you describe a specific case/situation where this would happen ?
> The main reason we have barebox vs. barebox-aux is because you sometimes need to build a full-blown Barebox, and a smaller Barebox that serves a first stage bootloader. From what I remember the generated images had different names, don't they ?

In our case it's happening, because we are building a barebox to be used with the device normally and use the barebox-aux to build a barebox which we use with the serial downloader mode of our board to do initial setup and programming. Those images have the same name and the second file overwrites the first without this patch. I understand that the usual case for barebox-aux may be to build a first stage bootloader, but I think it should not be limited to that case. The description of the barebox-aux package is " Build barebox with an auxiliary configuration" and for this, you can't be sure that the image files have different names. As soon as you are building 2 barebox images of the same type(SPL/TPL), but different config (which I think is quite useful) you will have this problem. I don't know if you may also have it in some cases when the type is different. Also, you can leave the setting empty and it will work just like before.

>> Also, you need to rename the images in a post-build script if you need
>> certain file names.
> That is a pretty normal thing in Buildroot. We just install things as they are installed by the upstream build system, and leave it up to custom post-build script to further rename/move/adjust to match the specific requirements of the project/user.

I understand. In this case I see it as some kind of nice-to-have function you can achieve. The main reason is the one above.

Thanks,
Markus
________________________________

Erbe Elektromedizin GmbH Firmensitz: 72072 Tuebingen Geschaeftsfuehrer: Christian O. Erbe, Reiner Thede Registergericht: Stuttgart HRB 380137

[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Thomas Petazzoni]

Hello Markus,

On Tue, 19 Feb 2019 11:03:52 +0000
"Steinhilber, Markus" <Markus.Steinhilber@erbe-med.com> wrote:

> >> Until now barebox images are copied to the output/images directory
> >> with their default name. This is a problem if the barebox and
> >> barebox-aux images have the same name.  
> > Could you describe a specific case/situation where this would happen ?
> > The main reason we have barebox vs. barebox-aux is because you sometimes need to build a full-blown Barebox, and a smaller Barebox that serves a first stage bootloader. From what I remember the generated images had different names, don't they ?  
> 
> In our case it's happening, because we are building a barebox to be
> used with the device normally and use the barebox-aux to build a
> barebox which we use with the serial downloader mode of our board to
> do initial setup and programming. Those images have the same name and
> the second file overwrites the first without this patch.

OK. It kind of works "by luck", because if you had been using U-Boot as
a bootloader, we don't support building two different U-Boot
configurations, and you would have had to use two separate Buildroot
configurations.

> I understand that the usual case for barebox-aux may be to build a
> first stage bootloader, but I think it should not be limited to that
> case. The description of the barebox-aux package is " Build barebox
> with an auxiliary configuration" and for this, you can't be sure that
> the image files have different names. As soon as you are building 2
> barebox images of the same type(SPL/TPL), but different config (which
> I think is quite useful) you will have this problem. I don't know if
> you may also have it in some cases when the type is different. Also,
> you can leave the setting empty and it will work just like before.
> 
> >> Also, you need to rename the images in a post-build script if you
> >> need certain file names.  
> > That is a pretty normal thing in Buildroot. We just install things
> > as they are installed by the upstream build system, and leave it up
> > to custom post-build script to further rename/move/adjust to match
> > the specific requirements of the project/user.  
> 
> I understand. In this case I see it as some kind of nice-to-have
> function you can achieve. The main reason is the one above.

Then perhaps the more Buildroot-ish way of doing things would be to
install the images build by the barebox package in a different location
than the one installed by the barebox-aux package, like
$(BINARIES_DIR)/barebox/ and $(BINARIES_DIR)/barebox-aux/. The
downsides of this are:

 - We don't do this for other packages

 - It breaks backward compatibility.

Arnout, Peter, what do you think ? (See Markus original patch for the
start of the discussion)

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Arnout Vandecappelle]

On 22/02/2019 09:40, Thomas Petazzoni wrote:
> Hello Markus,
> 
> On Tue, 19 Feb 2019 11:03:52 +0000
> "Steinhilber, Markus" <Markus.Steinhilber@erbe-med.com> wrote:

[snip]
>>>> Also, you need to rename the images in a post-build script if you
>>>> need certain file names.  
>>> That is a pretty normal thing in Buildroot. We just install things
>>> as they are installed by the upstream build system, and leave it up
>>> to custom post-build script to further rename/move/adjust to match
>>> the specific requirements of the project/user.  
>>
>> I understand. In this case I see it as some kind of nice-to-have
>> function you can achieve. The main reason is the one above.
> 
> Then perhaps the more Buildroot-ish way of doing things would be to
> install the images build by the barebox package in a different location
> than the one installed by the barebox-aux package, like
> $(BINARIES_DIR)/barebox/ and $(BINARIES_DIR)/barebox-aux/. The
> downsides of this are:
> 
>  - We don't do this for other packages
> 
>  - It breaks backward compatibility.
> 
> Arnout, Peter, what do you think ? (See Markus original patch for the
> start of the discussion)

 I feel that the original patch is way too complicated for such a corner case.

 Indeed, this use case happens to (almost) work by chance because barebox and
barebox-aux are two different packages rather than one like for U-Boot. But if
you anyway have the luck of having such an option, I don't mind adding a little
bit extra to be able to make better use of it.

 Perhaps a possibility is to add a config option that prefixes the output name
with something? You'd still need to rename it in the post-build script (or, more
likely, handle that in the genimage.cfg file). But it does solve the
identical-name problem, it looks pretty simple, and it doesn't break anything.

 Regards,
 Arnout

[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Thomas Petazzoni]

Hello,

On Fri, 22 Feb 2019 09:53:18 +0100
Arnout Vandecappelle <arnout@mind.be> wrote:

>  Indeed, this use case happens to (almost) work by chance because barebox and
> barebox-aux are two different packages rather than one like for U-Boot. But if
> you anyway have the luck of having such an option, I don't mind adding a little
> bit extra to be able to make better use of it.
> 
>  Perhaps a possibility is to add a config option that prefixes the output name
> with something? You'd still need to rename it in the post-build script (or, more
> likely, handle that in the genimage.cfg file). But it does solve the
> identical-name problem, it looks pretty simple, and it doesn't break anything.

Good idea! It is simple (just one string option), and it preserves
backward compatibility (the default value of the option would be empty).

Markus, could you look at implementing this instead ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Steinhilber, Markus]

Hi,

> -----Original Message-----
> From: Thomas Petazzoni [mailto:thomas.petazzoni at bootlin.com]
> Hello,
>
> On Fri, 22 Feb 2019 09:53:18 +0100
> Arnout Vandecappelle <arnout@mind.be> wrote:
>
> >  Indeed, this use case happens to (almost) work by chance because
> > barebox and barebox-aux are two different packages rather than one
> > like for U-Boot. But if you anyway have the luck of having such an
> > option, I don't mind adding a little bit extra to be able to make better use of it.
> >
> >  Perhaps a possibility is to add a config option that prefixes the
> > output name with something? You'd still need to rename it in the
> > post-build script (or, more likely, handle that in the genimage.cfg
> > file). But it does solve the identical-name problem, it looks pretty simple, and it doesn't break anything.
>
> Good idea! It is simple (just one string option), and it preserves backward compatibility (the default value of the option would be
> empty).

I have to say that I don't see a benefit in the proposed behavior over my patch. My patch adds also a single string option (ok, it can be a string list to be fair) and also doesn't break backward compatibility. Additionally it can be used as a rename option. With a default value of empty it behaves exactly as without the patch.

I also don't see it as too complex. It practically the original if-structure with an added if for each case to check if the value is set and use it or do the 'backward compatibility' way.

> Markus, could you look at implementing this instead ?

I think I could, but not right now as I'm busy with some other things.

Regards,
Markus
________________________________

Erbe Elektromedizin GmbH Firmensitz: 72072 Tuebingen Geschaeftsfuehrer: Christian O. Erbe, Reiner Thede Registergericht: Stuttgart HRB 380137

[Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy

[Thomas Petazzoni]

Hello Markus,

On Mon, 25 Feb 2019 10:00:37 +0000
"Steinhilber, Markus" <Markus.Steinhilber@erbe-med.com> wrote:

> > Good idea! It is simple (just one string option), and it preserves
> > backward compatibility (the default value of the option would be
> > empty).  
> 
> I have to say that I don't see a benefit in the proposed behavior
> over my patch. My patch adds also a single string option (ok, it can
> be a string list to be fair) and also doesn't break backward
> compatibility. Additionally it can be used as a rename option. With a
> default value of empty it behaves exactly as without the patch.
> 
> I also don't see it as too complex. It practically the original
> if-structure with an added if for each case to check if the value is
> set and use it or do the 'backward compatibility' way.

It's purely a matter of "design principle", i.e how Buildroot normally
does/implements/designs things. We try to be consistent and have the
same basic design principles applied across Buildroot, so that the way
package A does things is similar to how package B does things.

And nowhere in Buildroot we have this idea of two string options, where
the Nth entry of the first list "maps" to the Nth entry of the second
list. While it works, it's a bit of an awkward/strange semantic for
Config.in string options, hence our preference for a single option that
allows to specify a prefix or path where the Barebox binaries should be
installed.

So, there is nothing wrong per-se with your initial approach: it
functionally works. It is just that we (Arnout, me, as long-time
Buildroot developers/contributors) find that it doesn't fit very well
in the usual BR design principles, and there is a simple alternate
solution that works *and* fits better with the usual BR design
principles. It's as simple as that :-)

Hopefully this clarifies our suggestion on your contribution.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com