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

["Moessbauer, Felix" <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.

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

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.

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" %