Re: [PATCH] image: Run copy_boot_files after rootfs postprocessing

[Harald Seiler <hws@denx.de>]

Hello Henning,

On Thu, 2020-06-25 at 21:27 +0200, Henning Schild wrote:
> Am Thu, 25 Jun 2020 21:23:54 +0200
> schrieb "[ext] Henning Schild" <henning.schild@siemens.com>:
> 
> > Am Thu, 25 Jun 2020 20:43:07 +0200
> > schrieb "[ext] Henning Schild" <henning.schild@siemens.com>:
> > 
> > > Am Thu, 25 Jun 2020 19:24:30 +0200
> > > schrieb Harald Seiler <hws@denx.de>:
> > >   
> > > > Hello Henning,
> > > > 
> > > > On Thu, 2020-06-25 at 19:02 +0200, Jan Kiszka wrote:    
> > > > > On 25.06.20 18:48, [ext] Henning Schild wrote:      
> > > > > > Hi Harald,
> > > > > > 
> > > > > > can you elaborate on those cases? The postprocessing is hacky,
> > > > > > if the problem is coming from your layer you should probably
> > > > > > keep this patch in you layer.      
> > > > > 
> > > > > Basically do_generate_image_uuid from 
> > > > > https://lore.kernel.org/cip-dev/20200625141015.31719-4-Quirin.Gylstorff@siemens.com/T/#u, 
> > > > > just modeled as post-processing hook, rather than a task.      
> > > > 
> > > > For reference, this is the exact code:
> > > > 
> > > >     ROOTFS_POSTPROCESS_COMMAND =+
> > > > "image_postprocess_generate_uuid"
> > > > image_postprocess_generate_uuid() { sudo sed -i
> > > > '/^IMAGE_UUID=.*/d' '${IMAGE_ROOTFS}/etc/os-release' echo
> > > > "IMAGE_UUID=\"${IMAGE_UUID}\"" | \ sudo tee -a
> > > > '${IMAGE_ROOTFS}/etc/os-release'
> > > > 
> > > >         sudo -E chroot '${ROOTFSDIR}' \
> > > >             update-initramfs -u
> > > >     }    
> > > 
> > > If /etc/os-release goes into the initrd we have the issue with
> > > image_postprocess_mark in isar, a valid reason for a merge.  

It does not.  The reason /etc/os-release and the initramfs become related
here is because the IMAGE_UUID from the rootfs needs to be added into the
initramfs so it can later identify the rootfs it belongs to (when multiple
copies exist).  This is done by a separate initramfs hook installed as
a package.

> > But that is not the case. You re-create the initrd in your layer so it
> > might just be your problem! How about we discuss the IMAGE_UUID
> > upstream together with the reordering?
> 
> Or do we need the "update-initramfs -u" for image_postprocess_mark?

No, as described above, this is only necessary for IMAGE_UUID because
there, a hook copies the IMAGE_UUID into the initramfs.  The rest of
/etc/os-release is irrelevant for initrd (AFAIK).

> If so that should be done as a patch in this queue, you will get your
> reordering for IMAGE_UUID in your layer if you decide to keep that
> downstream.

I don't know what the plan is; whether IMAGE_UUID should be upstreamed at
some point.  Jan, you're probably the person to ask here?  From my side,
I think this would be a useful feature to have.

> Henning
> 
> > Henning
> > 
> > > > > Jan
> > > > >       
> > > > > > Maybe you can point out an issue in isar itself, or explain
> > > > > > how you got into this situation? We can then see if your
> > > > > > change is generic enough for upstream. You could also provide
> > > > > > the error-case from your layer as an upstream feature, if
> > > > > > that is generic enough.      
> > > > 
> > > > I think this patch addresses an issue in isar itself.  There is no
> > > > reason for copy_boot_files() to run before the postprocessing
> > > > does. I've checked through the git history and the reason this
> > > > relationship was introduced was a bigger refactor of the task
> > > > dependency chain. It does not seem to be intentionally this way
> > > > from what I can tell.    
> > > 
> > > Ok, sounds fair.

In the other thread, Claudius has shed some light on the design decisions
that let to this.  But let's keep that discussion over there.

> > > > The other way around makes more sense, in my opinion.  As stated
> > > > in the commit message, postprocessing might do an update to the
> > > > initramfs (as seen above) and this change needs to be reflected in
> > > > the deployed initramfs as well, instead of silently only living in
> > > > the version that is part of the rootfs.
> > > > 
> > > > I also checked all existing postprocessing commands and did not
> > > > see any that assume to be run after the boot files have been
> > > > deployed.    
> > > 
> > > I just worried about people abusing postprocess for changes that
> > > should be packages instead. Thanks for going into detail!

If you have a better approach than what I came up with, please do tell!
I decided on postprocessing for a few reasons:

- The IMAGE_UUID is an 'image feature' as in, the image recipe should
  decide what its ID is (an image inheriting `image_uuid` can set the ID
  string).  So pushing that into a package is difficult.
- As this only adds a value to /etc/os-release instead of deploying the
  whole file, a package approach would need to work via postinst which
  seems like a hack to me.
- The previous approach from Quirin was to add a separate task for adding
  the UUID.  This task can then be placed in the dependency chain where
  I have now moved the postprocessing but this feels like duplicated work
  when a postprocessing task exists as a feature already.

-- 
Harald

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