[Buildroot] [PATCH] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Buildroot] [PATCH] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Sebastian Frias]

The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own
'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS
provided by (or updated with) 055e6162bba7 ("linux: don't build appended DTB
image in place and support multiple images") and thus is not operating
properly when APPENDED_DTB is used.

Indeed, it copies a single image, and does not copy the one with the DTB
appended.

This patch replaces the 'cp' command with $(LINUX_INSTALL_IMAGES_CMDS) which
handles APPENDED_DTB. It also makes the copy verbose so that the user can
see how many images were copied.

Fixes: 055e6162bba7 ("linux: don't build appended DTB image in place and
support multiple images")

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
 linux/linux.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux/linux.mk b/linux/linux.mk
index 317587f..1f93074 100644
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -330,7 +330,7 @@ ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
 # appended DTBs.
 define LINUX_INSTALL_IMAGE
 	mkdir -p $(1)
-	cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
+	cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
 endef
 else
 # Otherwise, just install the unique image generated by the kernel
@@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt:
$(LINUX_DIR)/.stamp_target_installed $(LI
 	# Build the kernel.
 	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)
$(LINUX_TARGET_NAME)
 	$(LINUX_APPEND_DTB)
-	# Copy the kernel image to its final destination
-	cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR)
+	# Copy the kernel image(s) to its(their) final destination
+	$(LINUX_INSTALL_IMAGES_CMDS)
 	# If there is a .ub file copy it to the final destination
 	test ! -f $(LINUX_IMAGE_PATH).ub || cp $(LINUX_IMAGE_PATH).ub
$(BINARIES_DIR)
 	$(Q)touch $@
-- 
1.7.11.2

[Buildroot] [PATCH] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Thomas Petazzoni]

Hello,

On Mon, 25 Apr 2016 13:58:59 +0200, Sebastian Frias wrote:

> The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own
> 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS
> provided by (or updated with) 055e6162bba7 ("linux: don't build appended DTB
> image in place and support multiple images") and thus is not operating
> properly when APPENDED_DTB is used.
> 
> Indeed, it copies a single image, and does not copy the one with the DTB
> appended.
> 
> This patch replaces the 'cp' command with $(LINUX_INSTALL_IMAGES_CMDS) which
> handles APPENDED_DTB. It also makes the copy verbose so that the user can
> see how many images were copied.
> 
> Fixes: 055e6162bba7 ("linux: don't build appended DTB image in place and
> support multiple images")

Thanks for reporting the problem, and thanks for your patch!

> diff --git a/linux/linux.mk b/linux/linux.mk
> index 317587f..1f93074 100644
> --- a/linux/linux.mk
> +++ b/linux/linux.mk
> @@ -330,7 +330,7 @@ ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
>  # appended DTBs.
>  define LINUX_INSTALL_IMAGE
>  	mkdir -p $(1)
> -	cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
> +	cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)

I think you did this change for debugging (which is OK), but it should
be kept in your patch.

>  endef
>  else
>  # Otherwise, just install the unique image generated by the kernel
> @@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt:
> $(LINUX_DIR)/.stamp_target_installed $(LI
>  	# Build the kernel.
>  	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)
> $(LINUX_TARGET_NAME)
>  	$(LINUX_APPEND_DTB)
> -	# Copy the kernel image to its final destination
> -	cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR)
> +	# Copy the kernel image(s) to its(their) final destination
> +	$(LINUX_INSTALL_IMAGES_CMDS)

I think I would prefer to have:

	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Thomas Petazzoni]

Hello,

On Mon, 25 Apr 2016 14:06:15 +0200, Thomas Petazzoni wrote:

> > -	cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
> > +	cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
> 
> I think you did this change for debugging (which is OK), but it should
> be kept in your patch.

Obviously: it should be *removed* from your patch. Sorry for the
mess :-/

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Sebastian Frias]

Hi Thomas,

On 04/25/2016 02:06 PM, Thomas Petazzoni wrote:
> Hello,
> 
> Thanks for reporting the problem, and thanks for your patch!

Sure! That's what Open Source is for! :-)

> 
>> diff --git a/linux/linux.mk b/linux/linux.mk
>> index 317587f..1f93074 100644
>> --- a/linux/linux.mk
>> +++ b/linux/linux.mk
>> @@ -330,7 +330,7 @@ ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
>>  # appended DTBs.
>>  define LINUX_INSTALL_IMAGE
>>  	mkdir -p $(1)
>> -	cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
>> +	cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
> 
> I think you did this change for debugging (which is OK), but it should
> be kept in your patch.

Ok, I will remove it.

> 
>>  endef
>>  else
>>  # Otherwise, just install the unique image generated by the kernel
>> @@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt:
>> $(LINUX_DIR)/.stamp_target_installed $(LI
>>  	# Build the kernel.
>>  	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)
>> $(LINUX_TARGET_NAME)

This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it?
LINUX_BUILD_CMDS does a couple of more things than what is done here,
like like running LINUX_BUILD_DTB or 'make modules', and maybe that is
not wanted?

>>  	$(LINUX_APPEND_DTB)
>> -	# Copy the kernel image to its final destination
>> -	cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR)
>> +	# Copy the kernel image(s) to its(their) final destination
>> +	$(LINUX_INSTALL_IMAGES_CMDS)
> 
> I think I would prefer to have:
> 
> 	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))
> 

Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also
handles the DTB copy when APPENDED_DTB is not set, right?

Best regards,

Sebastian

[Buildroot] [PATCH] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Thomas Petazzoni]

Hello,

On Mon, 25 Apr 2016 14:17:50 +0200, Sebastian Frias wrote:

> >>  	# Build the kernel.
> >>  	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)
> >> $(LINUX_TARGET_NAME)
> 
> This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it?
> LINUX_BUILD_CMDS does a couple of more things than what is done here,
> like like running LINUX_BUILD_DTB or 'make modules', and maybe that is
> not wanted?

We don't want to do "make modules" again, as this will change the root
filesystem (surely by reinstalling the same thing, but still), while we
are exactly in the process of adding the filesystem to the kernel image.

> > I think I would prefer to have:
> > 
> > 	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))
> 
> Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also
> handles the DTB copy when APPENDED_DTB is not set, right?

It also copies the DTB to $(BINARIES_DIR), which is useless here. And in
general, I don't like too much seeing the _CMDS variables called from
other variables.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Sebastian Frias]

Hi Thomas,

On 04/25/2016 02:23 PM, Thomas Petazzoni wrote:
> Hello,
> 
> On Mon, 25 Apr 2016 14:17:50 +0200, Sebastian Frias wrote:
> 
>>>>  	# Build the kernel.
>>>>  	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)
>>>> $(LINUX_TARGET_NAME)
>>
>> This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it?
>> LINUX_BUILD_CMDS does a couple of more things than what is done here,
>> like like running LINUX_BUILD_DTB or 'make modules', and maybe that is
>> not wanted?
> 
> We don't want to do "make modules" again, as this will change the root
> filesystem (surely by reinstalling the same thing, but still), while we
> are exactly in the process of adding the filesystem to the kernel image.

Ok, thanks for the explanation.

> 
>>> I think I would prefer to have:
>>>
>>> 	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))
>>
>> Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also
>> handles the DTB copy when APPENDED_DTB is not set, right?
> 
> It also copies the DTB to $(BINARIES_DIR), which is useless here. And in
> general, I don't like too much seeing the _CMDS variables called from
> other variables.

Ok, just one last question because I seem to be missing something here.

In your commit 055e6162bba7 ("linux: don't build appended DTB image in
place and support multiple images") you indicated:

-----
<snip>
    Some of the tested configuration:

     - Normal uImage with several DTBs

       BR2_LINUX_KERNEL_DEFCONFIG="mvebu_v7"
       BR2_LINUX_KERNEL_UIMAGE=y
       BR2_LINUX_KERNEL_UIMAGE_LOADADDR="0x200000"
       BR2_LINUX_KERNEL_DTS_SUPPORT=y
       BR2_LINUX_KERNEL_INTREE_DTS_NAME="armada-xp-matrix armada-xp-gp
armada-370-mirabox"

       Contents of output/images/:

       armada-370-mirabox.dtb  armada-xp-gp.dtb  armada-xp-matrix.dtb
uImage
<snip>
-----

Hence, the DTB images are intended to be copied to output/images/ under
some circumstances, right?
How would that work? I mean, there's no call to LINUX_INSTALL_DTB on
"$(LINUX_DIR)/.stamp_initramfs_rebuilt".
Or does that happens through LINUX_INSTALL_TARGET_CMDS and
package/pkg-generic.mk's "$(BUILD_DIR)/%/.stamp_target_installed" target?
In that case I think I don't understand why we enter in
"$(LINUX_DIR)/.stamp_initramfs_rebuilt" target, given there's a target
$(BUILD_DIR)/%/.stamp_images_installed that calls LINUX_INSTALL_IMAGES_CMDS.

Best regards,

Sebastian

[Buildroot] [PATCH v2] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Sebastian Frias]

The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own
'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS
provided by (or updated with) commit 055e6162bba7 ("linux: don't build
appended DTB image in place and support multiple images") and thus is
not operating properly when APPENDED_DTB is used.

Indeed, it copies a single image, and does not copy the one with the DTB
appended.

This patch replaces the 'cp' command with LINUX_INSTALL_IMAGE which
handles APPENDED_DTB.

Fixes: 055e6162bba7 ("linux: don't build appended DTB image in place and
support multiple images")

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
Changes v1 -> v2:
  - remove '-v' from cp (as suggested by thomas.petazzoni at free-electrons.com)
  - use LINUX_INSTALL_IMAGE instead of LINUX_INSTALL_IMAGES_CMDS (also suggested by thomas.petazzoni at free-electrons.com)
---
 linux/linux.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux/linux.mk b/linux/linux.mk
index 317587f..1f93074 100644
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt: $(LINUX_DIR)/.stamp_target_installed $(LI
 	# Build the kernel.
 	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) $(LINUX_TARGET_NAME)
 	$(LINUX_APPEND_DTB)
-	# Copy the kernel image to its final destination
-	cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR)
+	# Copy the kernel image(s) to its(their) final destination
+	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))
 	# If there is a .ub file copy it to the final destination
 	test ! -f $(LINUX_IMAGE_PATH).ub || cp $(LINUX_IMAGE_PATH).ub $(BINARIES_DIR)
 	$(Q)touch $@
-- 
1.7.11.2

[Buildroot] [PATCH v2] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Thomas Petazzoni]

Hello,

On Mon, 25 Apr 2016 16:52:17 +0200, Sebastian Frias wrote:
> The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own
> 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS
> provided by (or updated with) commit 055e6162bba7 ("linux: don't build
> appended DTB image in place and support multiple images") and thus is
> not operating properly when APPENDED_DTB is used.
> 
> Indeed, it copies a single image, and does not copy the one with the DTB
> appended.
> 
> This patch replaces the 'cp' command with LINUX_INSTALL_IMAGE which
> handles APPENDED_DTB.
> 
> Fixes: 055e6162bba7 ("linux: don't build appended DTB image in place and
> support multiple images")
> 
> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
> Changes v1 -> v2:
>   - remove '-v' from cp (as suggested by thomas.petazzoni at free-electrons.com)
>   - use LINUX_INSTALL_IMAGE instead of LINUX_INSTALL_IMAGES_CMDS (also suggested by thomas.petazzoni at free-electrons.com)
> ---
>  linux/linux.mk | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

The commit title was no longer matching what the commit was doing, so
I've tweaked it, and applied your patch. Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH v2] linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

[Sebastian Frias]

Hi Thomas,

On 04/25/2016 09:43 PM, Thomas Petazzoni wrote:
> Hello,
> 
> On Mon, 25 Apr 2016 16:52:17 +0200, Sebastian Frias wrote:
>> The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own
>> 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS
>> provided by (or updated with) commit 055e6162bba7 ("linux: don't build
>> appended DTB image in place and support multiple images") and thus is
>> not operating properly when APPENDED_DTB is used.
>>
>> Indeed, it copies a single image, and does not copy the one with the DTB
>> appended.
>>
>> This patch replaces the 'cp' command with LINUX_INSTALL_IMAGE which
>> handles APPENDED_DTB.
>>
>> Fixes: 055e6162bba7 ("linux: don't build appended DTB image in place and
>> support multiple images")
>>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>> ---
>> Changes v1 -> v2:
>>   - remove '-v' from cp (as suggested by thomas.petazzoni at free-electrons.com)
>>   - use LINUX_INSTALL_IMAGE instead of LINUX_INSTALL_IMAGES_CMDS (also suggested by thomas.petazzoni at free-electrons.com)
>> ---
>>  linux/linux.mk | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> The commit title was no longer matching what the commit was doing, so
> I've tweaked it, and applied your patch. Thanks!
> 

Oh, sorry about that (I had edited the patch manually), and thanks for fixing it! :-)

Best regards,

Sebastian