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

[Claudius Heine <claudius.heine.ext@siemens.com>]

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