[Buildroot] [PATCH 1/2] package/uboot-tools: resolve uboot env source file error

[Buildroot] [PATCH 1/2] package/uboot-tools: resolve uboot env source file error

[Matt Weber]

From: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>

If Target u-boot is not available, the host build of uboot-tools
requires user to provide u-boot environment source file.
This change resolves a missing parentheses and updates the comment
for the same.

Signed-off-by: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
---
 package/uboot-tools/uboot-tools.mk | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk
index 1313107e0e..10cbd1cdd9 100644
--- a/package/uboot-tools/uboot-tools.mk
+++ b/package/uboot-tools/uboot-tools.mk
@@ -139,11 +139,11 @@ ifeq ($(BR_BUILDING),y)
 ifeq ($(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE)),)
 $(error Please provide U-Boot environment size (BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE setting))
 endif
-# If U-Boot is available, ENVIMAGE_SOURCE is optional because the default can
-# be taken from U-Boot.
+# If U-Boot is not available, ENVIMAGE_SOURCE must be provided by user,
+# otherwise it is optional because the default can be taken from U-Boot
 ifeq ($(BR2_TARGET_UBOOT),)
-ifeq ($(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE),)
-$(error Please provide U-Boot environment file BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE setting))
+ifeq ($(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE)),)
+$(error Please provide U-Boot environment file (BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE setting))
 endif
 endif #BR2_TARGET_UBOOT
 endif #BR_BUILDING
-- 
2.17.1

[Buildroot] [PATCH 2/2] package/uboot-tools: env/script generation need BINARIES_DIR

[Matt Weber]

From: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>

The host build of uboot-tools can occur early in the build process and may
require the creation of BINARIES_DIR before generation of an enabled envimage
and/or boot script binary.

Signed-off-by: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
---
 package/uboot-tools/uboot-tools.mk | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk
index 10cbd1cdd9..f9ff170266 100644
--- a/package/uboot-tools/uboot-tools.mk
+++ b/package/uboot-tools/uboot-tools.mk
@@ -128,6 +128,7 @@ endif
 
 define HOST_UBOOT_TOOLS_GENERATE_ENV_IMAGE
 	$(HOST_UBOOT_TOOLS_GENERATE_ENV_DEFAULTS)
+	mkdir -p $(BINARIES_DIR)
 	$(HOST_DIR)/bin/mkenvimage -s $(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE) \
 		$(if $(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_REDUNDANT),-r) \
 		$(if $(filter "BIG",$(BR2_ENDIAN)),-b) \
@@ -164,6 +165,7 @@ define HOST_UBOOT_TOOLS_INSTALL_CMDS
 	$(INSTALL) -m 0755 -D $(@D)/tools/dumpimage $(HOST_DIR)/bin/dumpimage
 	$(HOST_UBOOT_TOOLS_GENERATE_ENV_IMAGE)
 	$(if $(BR2_PACKAGE_HOST_UBOOT_TOOLS_BOOT_SCRIPT),
+		mkdir -p $(BINARIES_DIR); \
 		$(MKIMAGE) -C none -A $(MKIMAGE_ARCH) -T script \
 			-d $(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_BOOT_SCRIPT_SOURCE)) \
 			$(BINARIES_DIR)/boot.scr)
-- 
2.17.1

[Buildroot] [PATCH 1/2] package/uboot-tools: resolve uboot env source file error

[Thomas Petazzoni]

On Mon, 25 Jan 2021 08:57:41 -0600
Matt Weber <matthew.weber@rockwellcollins.com> wrote:

> From: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> 
> If Target u-boot is not available, the host build of uboot-tools
> requires user to provide u-boot environment source file.
> This change resolves a missing parentheses and updates the comment
> for the same.
> 
> Signed-off-by: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
> ---
>  package/uboot-tools/uboot-tools.mk | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied to master, thanks. I think this bug really shows we need some
unit tests for this uboot-tools package functionality, in
supporting/testing/.

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

[Buildroot] [PATCH 2/2] package/uboot-tools: env/script generation need BINARIES_DIR

[Thomas Petazzoni]

On Mon, 25 Jan 2021 08:57:42 -0600
Matt Weber <matthew.weber@rockwellcollins.com> wrote:

> From: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> 
> The host build of uboot-tools can occur early in the build process and may
> require the creation of BINARIES_DIR before generation of an enabled envimage
> and/or boot script binary.
> 
> Signed-off-by: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>

Hum, while this works, I'm in fact not too happy with the proposed
solution. I would prefer that we move the mkenvimage/$(MKIMAGE)
invocations to a BUILD_CMDS step, that produces its results in $(@D),
and then INSTALL_CMDS does a $(INSTALL) ... of that file, which with
the -D option would create $(BINARIES_DIR).

I know it's a bit bike-shedding, but it feels a little bit better. What
do you think ?

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

[Buildroot] [PATCH 2/2] package/uboot-tools: env/script generation need BINARIES_DIR

[Matthew Weber]

Thomas/Yann,


On Mon, Jan 25, 2021 at 3:18 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Mon, 25 Jan 2021 08:57:42 -0600
> Matt Weber <matthew.weber@rockwellcollins.com> wrote:
>
> > From: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> >
> > The host build of uboot-tools can occur early in the build process and may
> > require the creation of BINARIES_DIR before generation of an enabled envimage
> > and/or boot script binary.
> >
> > Signed-off-by: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> > Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
>
> Hum, while this works, I'm in fact not too happy with the proposed
> solution. I would prefer that we move the mkenvimage/$(MKIMAGE)
> invocations to a BUILD_CMDS step, that produces its results in $(@D),
> and then INSTALL_CMDS does a $(INSTALL) ... of that file, which with
> the -D option would create $(BINARIES_DIR).
>
> I know it's a bit bike-shedding, but it feels a little bit better. What
> do you think ?

I've added Yann to the discussion as he suggested the mkdir :-)

Whatever is better to maintain.

-Matt

[Buildroot] [PATCH 1/2] package/uboot-tools: resolve uboot env source file error

[Matthew Weber]

Thomas,

On Mon, Jan 25, 2021 at 3:16 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Mon, 25 Jan 2021 08:57:41 -0600
> Matt Weber <matthew.weber@rockwellcollins.com> wrote:
>
> > From: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> >
> > If Target u-boot is not available, the host build of uboot-tools
> > requires user to provide u-boot environment source file.
> > This change resolves a missing parentheses and updates the comment
> > for the same.
> >
> > Signed-off-by: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> > Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
> > ---
> >  package/uboot-tools/uboot-tools.mk | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
>
> Applied to master, thanks. I think this bug really shows we need some
> unit tests for this uboot-tools package functionality, in
> supporting/testing/.

I did add autobuilder testing of this feature but it didn't do a case
for when the file isn't provided....
https://github.com/buildroot/buildroot/blob/master/utils/genrandconfig#L304

Matt

[Buildroot] [PATCH 1/2] package/uboot-tools: resolve uboot env source file error

[Thomas Petazzoni]

On Mon, 25 Jan 2021 15:30:42 -0600
Matthew Weber <matthew.weber@collins.com> wrote:

> > Applied to master, thanks. I think this bug really shows we need some
> > unit tests for this uboot-tools package functionality, in
> > supporting/testing/.  
> 
> I did add autobuilder testing of this feature but it didn't do a case
> for when the file isn't provided....
> https://github.com/buildroot/buildroot/blob/master/utils/genrandconfig#L304

I think this sort of feature gets better tested by specific targeted
tests in support/testing/.

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

[Buildroot] [PATCH 2/2] package/uboot-tools: env/script generation need BINARIES_DIR

[Yann E. MORIN]

Matthew, Thomas, All,

On 2021-01-25 15:22 -0600, Matthew Weber via buildroot spake thusly:
> On Mon, Jan 25, 2021 at 3:18 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
> > On Mon, 25 Jan 2021 08:57:42 -0600
> > Matt Weber <matthew.weber@rockwellcollins.com> wrote:
> > > From: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> > > The host build of uboot-tools can occur early in the build process and may
> > > require the creation of BINARIES_DIR before generation of an enabled envimage
> > > and/or boot script binary.
> > >
> > > Signed-off-by: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
> > > Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
> >
> > Hum, while this works, I'm in fact not too happy with the proposed
> > solution. I would prefer that we move the mkenvimage/$(MKIMAGE)
> > invocations to a BUILD_CMDS step, that produces its results in $(@D),
> > and then INSTALL_CMDS does a $(INSTALL) ... of that file, which with
> > the -D option would create $(BINARIES_DIR).
> >
> > I know it's a bit bike-shedding, but it feels a little bit better. What
> > do you think ?
> 
> I've added Yann to the discussion as he suggested the mkdir :-)

On principle, I do agree with Thomas.

However, I would point out that this is way wider than just the script
image. Indeed, we also have HOST_UBOOT_TOOLS_GENERATE_ENV_IMAGE which
is only called at install time.

If we wanted to mandate clean, separate build and install steps for the
script image generation, so would we want for the environment image too.

Which is a wider endeavour than just catering for the script image.

Of course, if one would want to actually tackle the issue and actually
separate the build and install steps, that would be awesome.

Otherwise, I am fine with just the mkdir.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/2] package/uboot-tools: env/script generation need BINARIES_DIR

[Thomas Petazzoni]

On Mon, 25 Jan 2021 22:42:55 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> On principle, I do agree with Thomas.
> 
> However, I would point out that this is way wider than just the script
> image. Indeed, we also have HOST_UBOOT_TOOLS_GENERATE_ENV_IMAGE which
> is only called at install time.

Well, the patch is also adding a mkdir inside the code generating the
environment image.

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

[Buildroot] [External] Re: [PATCH 2/2] package/uboot-tools: env/script generation need BINARIES_DIR

[Matthew Weber]

All,

On Mon, Jan 25, 2021 at 4:23 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Mon, 25 Jan 2021 22:42:55 +0100
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
>
> > On principle, I do agree with Thomas.
> >
> > However, I would point out that this is way wider than just the script
> > image. Indeed, we also have HOST_UBOOT_TOOLS_GENERATE_ENV_IMAGE which
> > is only called at install time.
>
> Well, the patch is also adding a mkdir inside the code generating the
> environment image.

v2 will be out in a bit, i'll mark this one as superseded.

Regards,
Matt

[Buildroot] [PATCH 1/2] package/uboot-tools: resolve uboot env source file error

[Peter Korsgaard]

>>>>> "Matt" == Matt Weber <matthew.weber@rockwellcollins.com> writes:

 > From: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
 > If Target u-boot is not available, the host build of uboot-tools
 > requires user to provide u-boot environment source file.
 > This change resolves a missing parentheses and updates the comment
 > for the same.

 > Signed-off-by: Kalpesh Panchal <kalpesh.panchal@rockwellcollins.com>
 > Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>

Committed to 2020.11.x, thanks.

-- 
Bye, Peter Korsgaard