[PATCH 0/2] Fix of #56

[PATCH 0/2] Fix of #56

[claudius.heine.ext]

From: Claudius Heine <ch@denx.de>

Hi,

here is the fix for #56 without regression. Now its possible again to refer to
the kernel and initrd names at any point in the image creation process, even if
they were removed from the root fs in a cleanup step.

regards,
Claudius

Claudius Heine (2):
  image.bbclass: make KERNEL_IMAGE & INITRD_IMAGE variable fixed
  Revert "meta/classes/image: Call transform_template after
    rootfs_install"

 meta/classes/image.bbclass | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH 1/2] image.bbclass: make KERNEL_IMAGE & INITRD_IMAGE variable fixed

[claudius.heine.ext]

From: Claudius Heine <ch@denx.de>

Before the value of those variables where calculated at runtime with the
content of the target root file system. But if the root file system
does not contain the files yet or no longer, this variable will be
empty, even if those files are already copied to the deploy directory.

Setting this to fixed value allows to use them in every state of the
root file system.

Change from the last version is that KERNEL_IMAGE and INITRD_IMAGE
variables not contain the `${PF}`, so that they don't overwrite each
other.

Signed-off-by: Claudius Heine <ch@denx.de>
---
 meta/classes/image.bbclass | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 93d9bfc..398275f 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -13,8 +13,8 @@ IMAGE_INSTALL += "${@ ("linux-image-" + d.getVar("KERNEL_NAME", True)) if d.getV
 IMAGE_FULLNAME = "${PF}"
 
 # These variables are used by wic and start_vm
-KERNEL_IMAGE ?= "${@get_image_name(d, 'vmlinuz')[1]}"
-INITRD_IMAGE ?= "${@get_image_name(d, 'initrd.img')[1]}"
+KERNEL_IMAGE ?= "${PF}-vmlinuz"
+INITRD_IMAGE ?= "${PF}-initrd.img"
 
 # Useful variables for imager implementations:
 PP = "/home/builder/${PN}"
@@ -149,12 +149,10 @@ addtask rootfs_install before do_build after do_unpack
 
 do_copy_boot_files[dirs] = "${DEPLOY_DIR_IMAGE}"
 do_copy_boot_files() {
-    KERNEL_IMAGE=${@get_image_name(d, 'vmlinuz')[1]}
     if [ -n "${KERNEL_IMAGE}" ]; then
         cp -f ${IMAGE_ROOTFS}/boot/${@get_image_name(d, 'vmlinuz')[0]} ${DEPLOY_DIR_IMAGE}/${KERNEL_IMAGE}
     fi
 
-    INITRD_IMAGE=${@get_image_name(d, 'initrd.img')[1]}
     if [ -n "${INITRD_IMAGE}" ]; then
         sudo cp -f ${IMAGE_ROOTFS}/boot/${@get_image_name(d, 'initrd.img')[0]} ${DEPLOY_DIR_IMAGE}/${INITRD_IMAGE}
     fi
-- 
2.20.1

[PATCH 2/2] Revert "meta/classes/image: Call transform_template after rootfs_install"

[claudius.heine.ext]

From: Claudius Heine <ch@denx.de>

This 'fix' is no longer necessary.

This reverts commit 2df0b261610f38bc1d5472f7ee0e3d51243ed602.
---
 meta/classes/image.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 398275f..3eb261f 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -169,7 +169,7 @@ do_copy_boot_files() {
         cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/${DTB_FILE}"
     fi
 }
-addtask copy_boot_files before do_rootfs_postprocess do_transform_template after do_rootfs_install
+addtask copy_boot_files before do_rootfs_postprocess after do_rootfs_install
 
 python do_image_tools() {
     """Virtual task"""
-- 
2.20.1

Re: [PATCH 1/2] image.bbclass: make KERNEL_IMAGE & INITRD_IMAGE variable fixed

[Claudius Heine]

On 16/05/2019 14.50, [ext] claudius.heine.ext@siemens.com wrote:
> From: Claudius Heine <ch@denx.de>
> 
> Before the value of those variables where calculated at runtime with the
> content of the target root file system. But if the root file system
> does not contain the files yet or no longer, this variable will be
> empty, even if those files are already copied to the deploy directory.
> 
> Setting this to fixed value allows to use them in every state of the
> root file system.
> 
> Change from the last version is that KERNEL_IMAGE and INITRD_IMAGE
> variables not contain the `${PF}`, so that they don't overwrite each

not -> now

If that is the only mistake, then this could be changed on merge.

Claudius

> other.
> 
> Signed-off-by: Claudius Heine <ch@denx.de>
> ---
>   meta/classes/image.bbclass | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 93d9bfc..398275f 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -13,8 +13,8 @@ IMAGE_INSTALL += "${@ ("linux-image-" + d.getVar("KERNEL_NAME", True)) if d.getV
>   IMAGE_FULLNAME = "${PF}"
>   
>   # These variables are used by wic and start_vm
> -KERNEL_IMAGE ?= "${@get_image_name(d, 'vmlinuz')[1]}"
> -INITRD_IMAGE ?= "${@get_image_name(d, 'initrd.img')[1]}"
> +KERNEL_IMAGE ?= "${PF}-vmlinuz"
> +INITRD_IMAGE ?= "${PF}-initrd.img"
>   
>   # Useful variables for imager implementations:
>   PP = "/home/builder/${PN}"
> @@ -149,12 +149,10 @@ addtask rootfs_install before do_build after do_unpack
>   
>   do_copy_boot_files[dirs] = "${DEPLOY_DIR_IMAGE}"
>   do_copy_boot_files() {
> -    KERNEL_IMAGE=${@get_image_name(d, 'vmlinuz')[1]}
>       if [ -n "${KERNEL_IMAGE}" ]; then
>           cp -f ${IMAGE_ROOTFS}/boot/${@get_image_name(d, 'vmlinuz')[0]} ${DEPLOY_DIR_IMAGE}/${KERNEL_IMAGE}
>       fi
>   
> -    INITRD_IMAGE=${@get_image_name(d, 'initrd.img')[1]}
>       if [ -n "${INITRD_IMAGE}" ]; then
>           sudo cp -f ${IMAGE_ROOTFS}/boot/${@get_image_name(d, 'initrd.img')[0]} ${DEPLOY_DIR_IMAGE}/${INITRD_IMAGE}
>       fi
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de

Re: [PATCH 2/2] Revert "meta/classes/image: Call transform_template after rootfs_install"

[Maxim Yu. Osipov]

On 5/16/19 2:50 PM, claudius.heine.ext@siemens.com wrote:
> From: Claudius Heine <ch@denx.de>
> 
> This 'fix' is no longer necessary.

Why did you put fix in quotes?
This revert fixed the problem with do_fit_image if we want to keep old 
naming scheme for kernel/initrd image.

Maxim.

> This reverts commit 2df0b261610f38bc1d5472f7ee0e3d51243ed602.
> ---
>   meta/classes/image.bbclass | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 398275f..3eb261f 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -169,7 +169,7 @@ do_copy_boot_files() {
>           cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/${DTB_FILE}"
>       fi
>   }
> -addtask copy_boot_files before do_rootfs_postprocess do_transform_template after do_rootfs_install
> +addtask copy_boot_files before do_rootfs_postprocess after do_rootfs_install
>   
>   python do_image_tools() {
>       """Virtual task"""
> 


-- 
Maxim Osipov
ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn
Germany
+49 (151) 6517 6917
mosipov@ilbers.de
http://ilbers.de/
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

Re: [PATCH 0/2] Fix of #56

[Maxim Yu. Osipov]

On 5/16/19 2:50 PM, claudius.heine.ext@siemens.com wrote:
> From: Claudius Heine <ch@denx.de>
> 
> Hi,
> 
> here is the fix for #56 without regression. Now its possible again to refer to
> the kernel and initrd names at any point in the image creation process, even if
> they were removed from the root fs in a cleanup step.
> 
> regards,
> Claudius

Reworded and applied to the 'next'.

Maxim.

> Claudius Heine (2):
>    image.bbclass: make KERNEL_IMAGE & INITRD_IMAGE variable fixed
>    Revert "meta/classes/image: Call transform_template after
>      rootfs_install"
> 
>   meta/classes/image.bbclass | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 


-- 
Maxim Osipov
ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn
Germany
+49 (151) 6517 6917
mosipov@ilbers.de
http://ilbers.de/
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

Re: [PATCH 2/2] Revert "meta/classes/image: Call transform_template after rootfs_install"

[Claudius Heine]

Hi Maxim,

On 17/05/2019 12.58, Maxim Yu. Osipov wrote:
> On 5/16/19 2:50 PM, claudius.heine.ext@siemens.com wrote:
>> From: Claudius Heine <ch@denx.de>
>>
>> This 'fix' is no longer necessary.
> 
> Why did you put fix in quotes?

Because this change just painted over the underlying issue with the 
non-deterministic and volatile value of the KERNEL_IMAGE and 
INITRD_IMAGE variables. So I would not consider that a permanent fix but 
a quick band-aid.

> This revert fixed the problem with do_fit_image if we want to keep old 
> naming scheme for kernel/initrd image.

This is not just a naming scheme change. Any bitbake variable that could 
change in the middle of the build process is not deterministic an 
therefor a bug.

A good next refactoring step would in fact be to remove the 
'get_image_name' function, since it does not return a deterministic 
value, and instead implement that completely in the `do_copy_boot_files` 
task.

Claudius

> 
> Maxim.
> 
>> This reverts commit 2df0b261610f38bc1d5472f7ee0e3d51243ed602.
>> ---
>>   meta/classes/image.bbclass | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
>> index 398275f..3eb261f 100644
>> --- a/meta/classes/image.bbclass
>> +++ b/meta/classes/image.bbclass
>> @@ -169,7 +169,7 @@ do_copy_boot_files() {
>>           cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/${DTB_FILE}"
>>       fi
>>   }
>> -addtask copy_boot_files before do_rootfs_postprocess 
>> do_transform_template after do_rootfs_install
>> +addtask copy_boot_files before do_rootfs_postprocess after 
>> do_rootfs_install
>>   python do_image_tools() {
>>       """Virtual task"""
>>
> 
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de

Re: [PATCH 2/2] Revert "meta/classes/image: Call transform_template after rootfs_install"

[Baurzhan Ismagulov]

On Mon, May 20, 2019 at 09:19:41AM +0200, Claudius Heine wrote:
> Because this change just painted over the underlying issue with the
> non-deterministic and volatile value of the KERNEL_IMAGE and INITRD_IMAGE
> variables. So I would not consider that a permanent fix but a quick
> band-aid.

The function returns different values depending on whether the respective task
has finished. So, the implicit precondition is that the caller must depend on
the kernel deployment task. Given the precondition, the behavior is
deterministic and there could be other ways to fix them without sacrificing
user experience.

For my understanding, what was your user-level use case when you experienced
the get_image_name() issue?

To be clear, Maxim's patch only addresses CI and not this issue.

With kind regards,
Baurzhan.

Re: [PATCH 2/2] Revert "meta/classes/image: Call transform_template after rootfs_install"

[Claudius Heine]

Hi Baurzhan,

On 20/05/2019 21.47, Baurzhan Ismagulov wrote:
> On Mon, May 20, 2019 at 09:19:41AM +0200, Claudius Heine wrote:
>> Because this change just painted over the underlying issue with the
>> non-deterministic and volatile value of the KERNEL_IMAGE and INITRD_IMAGE
>> variables. So I would not consider that a permanent fix but a quick
>> band-aid.
> 
> The function returns different values depending on whether the respective task
> has finished. So, the implicit precondition is that the caller must depend on
> the kernel deployment task. Given the precondition, the behavior is
> deterministic and there could be other ways to fix them without sacrificing
> user experience.

?

I don't understand you here. From my perspective is this patchset now 
merged and the issue is fixed without sacrificing user experience.

Or do you have any issue with how it was fixed? If so, then a patch 
review would be nice.

> 
> For my understanding, what was your user-level use case when you experienced
> the get_image_name() issue?

As I already described previously, there is only a very specific time 
frame in the image build process where `get_image_name` is allowed to be 
called and that is after the kernel package is installed and before the 
kernel is removed from `/boot` in a possible post-install cleanup step.

After this patchset `get_image_name` is only called in one task 
(`do_copy_boot_files`), so it can be safely removed and integrated there 
to avoid any misuse of that function. I also think that is is much 
easier to implement there and thus removes complexity and improves code 
readability.

> To be clear, Maxim's patch only addresses CI and not this issue.

I know, and that was exactly what I commented on the patch. What the 
revert in Maxims patch set did was a zero sum improvement: one issue 
fixed and another caused.

I would have rather seen either to fix the issue (for instance the way I 
described in the review of Maxims patch set and this patch set does) or 
to disable the test in the CI until the issue is fixed correctly. That 
way either the issue in the code or that CI is 'red' would have been 
fixed and that would have been both a positive change instead no change.

A "revert" for me means that a patch does not do what it was meant to do 
or is useless. If it was found that a merged patch accidentally caused a 
regression, then instead of reverting that commit and knowingly causing 
further regressions, the regression of that patch should be fixed directly.

 From that conviction, Maxims revert patch just seems petty to me.

regards,
Claudius

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de