[Buildroot] [PATCHv2] package/uboot: detect missing user-supplied environment source files

[Cam Hutchison]

On 7 December 2017 at 20:13, Yann E. MORIN <yann.morin.1998 at free.fr> wrote:
> Cam, All,
>
> On 2017-12-07 19:18 +1100, Cam Hutchison spake thusly:
>> On 7 December 2017 at 09:58, Yann E. MORIN <yann.morin.1998 at free.fr> wrote:
>> > Since 0542bb79e8 (uboot: Support multiple environment source files),
>> > a missing user-supplied environment source files is no longer detected.
>> >
>> > This is because we cat them all, and feed the concatenation to the stdin
>> > of mkenvimage. So, if one source file is missing, the cat exits in error,
>> > but the compound command exits with the exit code of the last command,
>> > which is that of mkenvimage, which happens to be happy with whatever it
>> > is fed on its stdin, even is empty.
>> >
>> > We fix that by creating a temporary file, that we even leave afterward
>> > for the user to inspect.
>> >
>> > We also move it out of the _CMDS block and into a macro of its own, so
>> > that it is easier to write and maintain.
>> >
>> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998 at free.fr>
>> > Cc: Cam Hutchison <camh at xdna.net>
>> > Cc: Thomas Petazzoni <thomas.petazzoni at free-electrons.com>
>> >
>> > ---
>> > Changes v1 -> v2:
>> >   - move into its own macro  (Thomas)
>> > ---
>> >  boot/uboot/uboot.mk | 19 +++++++++++++------
>> >  1 file changed, 13 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
>> > index a1fac7dcae..abb89da69b 100644
>> > --- a/boot/uboot/uboot.mk
>> > +++ b/boot/uboot/uboot.mk
>> > @@ -238,6 +238,18 @@ define UBOOT_BUILD_OMAP_IFT
>> >                 -c $(call qstrip,$(BR2_TARGET_UBOOT_OMAP_IFT_CONFIG))
>> >  endef
>> >
>> > +ifneq ($(BR2_TARGET_UBOOT_ENVIMAGE),)
>> > +define UBOOT_GENERATE_ENV_IMAGE
>> > +       cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) \
>> > +               >$(BINARIES_DIR)/uboot-env.txt
>>
>> One (very minor) concern is using the name "uboot-env.txt". It is
>> possible that someone has
>> a post-image script that creates a file of that name in BINARIES_DIR,
>> so this will clobber
>> that file. As a tmp file, perhaps it should have a tmp file name?
>>
>> I have a post-image script that creates a file of this name, but it
>> goes at a higher-level in
>> a binaries-type dir - I have a multi-layered build script, and the
>> higher-level directory that
>> aggregates the layers protects my file from being clobbered, but I can
>> imagine that if I
>> made different decisions when writing my front-end script, I could
>> have easily placed
>> my uboot-env.txt file in buildroot's BINARIES_DIR.
>>
>> Perhaps _uboot-env.txt? Or go all the way and use mktemp?
>
> Or just generate uboot-env.txt in $(@D)/uboot-env.buildroot like so:
>
>     define UBOOT_GENERATE_ENV_IMAGE
>         cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) \
>             >$(@D)/uboot-env.buildroot
>         $(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
>             $(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
>             $(if $(filter BIG,$(BR2_ENDIAN)),-b) \
>             -o $(BINARIES_DIR)/uboot-env.bin \
>             $(@D)/uboot-env.buildroot
>     endef
>
> Thoughts?

I like this better.

LGTM


> Regards,
> Yann E. MORIN.
>
>> > +       $(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
>> > +               $(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
>> > +               $(if $(filter BIG,$(BR2_ENDIAN)),-b) \
>> > +               -o $(BINARIES_DIR)/uboot-env.bin \
>> > +               $(BINARIES_DIR)/uboot-env.txt
>> > +endef
>> > +endif
>> > +
>> >  define UBOOT_INSTALL_IMAGES_CMDS
>> >         $(foreach f,$(UBOOT_BINS), \
>> >                         cp -dpf $(@D)/$(f) $(BINARIES_DIR)/
>> > @@ -249,12 +261,7 @@ define UBOOT_INSTALL_IMAGES_CMDS
>> >                         cp -dpf $(@D)/$(f) $(BINARIES_DIR)/
>> >                 )
>> >         )
>> > -       $(if $(BR2_TARGET_UBOOT_ENVIMAGE),
>> > -               cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) | \
>> > -                       $(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
>> > -                       $(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
>> > -                       $(if $(filter BIG,$(BR2_ENDIAN)),-b) \
>> > -                       -o $(BINARIES_DIR)/uboot-env.bin -)
>> > +       $(UBOOT_GENERATE_ENV_IMAGE)
>> >         $(if $(BR2_TARGET_UBOOT_BOOT_SCRIPT),
>> >                 $(HOST_DIR)/bin/mkimage -C none -A $(MKIMAGE_ARCH) -T script \
>> >                         -d $(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)) \
>> > --
>> > 2.11.0
>> >
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'