Re: [PATCH 1/3] wic: locate systemd-boot efi files in buildchroot

[Henning Schild <henning.schild@siemens.com>]

Am Tue, 30 Aug 2022 10:23:35 +0200
schrieb "Moessbauer, Felix (T CED SES-DE)"
<felix.moessbauer@siemens.com>:

> > -----Original Message-----
> > From: Schild, Henning (T CED SES-DE) <henning.schild@siemens.com>
> > Sent: Monday, August 22, 2022 1:48 PM
> > To: Moessbauer, Felix (T CED SES-DE) <felix.moessbauer@siemens.com>
> > Cc: isar-users@googlegroups.com; amikan@ilbers.de; Kiszka, Jan (T
> > CED) <jan.kiszka@siemens.com>
> > Subject: Re: [PATCH 1/3] wic: locate systemd-boot efi files in
> > buildchroot
> > 
> > Am Fri, 12 Aug 2022 08:50:37 +0200
> > schrieb Felix Moessbauer <felix.moessbauer@siemens.com>:
> >   
> > > This patch locates the systemd-boot efi files in the buildchroot
> > > and not the target chroot.
> > > By that, no imager-specifc dependencies have to be installed in
> > > the target rootfs.
> > >
> > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> > > ---
> > >  meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py | 6
> > > +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git
> > > a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
> > > b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py index
> > > a24e04f3..7a65a98d 100644 ---
> > > a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py +++
> > > b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py @@
> > > -415,9 +415,9 @@ class BootimgEFIPlugin(SourcePlugin): grub_cmd
> > > += "reiserfs regexp " + grub_modules exec_cmd(grub_cmd) elif
> > > source_params['loader'] == 'systemd-boot':
> > > -                kernel_dir =
> > > os.path.join(rootfs_dir['ROOTFS_DIR'],
> > > "usr/lib/systemd/boot/efi/")
> > > -                for mod in [x for x in os.listdir(kernel_dir) if
> > > x.startswith("systemd-")]:
> > > -                    cp_cmd = "cp %s/%s %s/EFI/BOOT/%s" %
> > > (kernel_dir, mod, hdddir, mod[8:])
> > > +                imager_efi_dir = "/usr/lib/systemd/boot/efi/"
> > > +                for mod in [x for x in
> > > os.listdir(imager_efi_dir) if x.startswith("systemd-")]:
> > > +                    cp_cmd = "cp %s/%s %s/EFI/BOOT/%s" %
> > > (imager_efi_dir, mod, hdddir, mod[8:]) exec_cmd(cp_cmd, True)  
> > 
> > Reject! Touching a forked file will need that fix upstream first.
> > 
> > Or (more likely) need to play with some variables to make that
> > upstream code work. Check the diff between the fork and the
> > original, and the commit history! The code looks a bit weird, the
> > idea is to only insert a few lines here and there to hopefully be
> > able to keep up with upstream more easily and avoid merge
> > conflicts.  
> 
> Before changing this, I checked the commit history of each line
> around that 😉
> 
> While your approach makes merging easier, changing things as you did
> before might break at runtime as variables are misused. To give an
> example, have a look at [1]. There, you overwrite kernel_dir which is
> not a local variable (re-used in [2]) and which then points to a
> folder where no kernel is in.

Valid finding that will bite someone who wants to use that startup.nsh
feature. Maybe set the variable back to what it was or overload it with
a local scoped one.

> Getting our customization into OE will also be hard as there the
> imager works differently.

I think we will not be able to get certain things there, otherwise i
would never have forked. I managed to revert a lot of the forking done
by others, but some things remain and do not look like we could make a
valid point for upstream.

> I really prefer to let the code reflect the semantics instead of
> trying to make the fork as minimal as possible. But I'm interested in
> others opinions as well. Maybe Anton and Jan can comment.

The problem is that authors do not know which bits are copied/forked
and how to deal with those. And maintainers often overlook that stuff
as well. Maybe because i did that work and have an allergy against
copies that get out of sync, while others are more relaxed on that.

At the moment i see no need to touch any of this again. How well it
will work is left to be seen when those upstream plugins see major
change and we have to merge.

I guess what would also be useful would be some sort of CI check where
we make sure that any forked file is at least as new as its source.
Either with a bit of git or with test -nt/-ot and git restore-mtime

Henning

> Felix
> 
> [1]
> https://github.com/ilbers/isar/blob/master/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py#L439
> [2]
> https://github.com/ilbers/isar/blob/master/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py#L449
> 
> > 
> > What you are proposing here is very much against that principle.
> > 
> > Henning
> >   
> > >              else:
> > >                  raise WicError("unrecognized bootimg-efi-isar
> > > loader: %s" %  
>