[PATCH 0/3] systemd-boot: Locate imager deps in buildchroot

[PATCH 0/3] systemd-boot: Locate imager deps in buildchroot

[Felix Moessbauer]

This patch reworks the logic how we create images with systemd-boot:
Instead of installing the systemd-boot dependency into the target
image, we install it in the buildchroot, leveraging the IMAGER_INSTALL
infrastructure.

This has the following advantages:

- smaller images: The systemd-boot package is no longer installed into the image
- unified imager logic with OE

Best regards,
Felix

Felix Moessbauer (3):
  wic: locate systemd-boot efi files in buildchroot
  Rework imager support for systemd-boot
  Install systemd-boot into buildchroot for qemuarm

 meta-isar/conf/multiconfig/qemuarm-bookworm.conf        | 2 +-
 meta/conf/distro/debian-common.conf                     | 6 +++---
 meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.30.2

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

[Felix Moessbauer]

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)
             else:
                 raise WicError("unrecognized bootimg-efi-isar loader: %s" %
-- 
2.30.2

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

[Henning Schild]

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.

What you are proposing here is very much against that principle.

Henning

>              else:
>                  raise WicError("unrecognized bootimg-efi-isar
> loader: %s" %

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

[Moessbauer, Felix]

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

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

[Henning Schild]

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

[PATCH 2/3] Rework imager support for systemd-boot

[Felix Moessbauer]

This is an addendum to a1e441.

As the imager now reads the systemd-boot files from the
buildchroot, we install the systemd-boot deps there (instead of
installing into the target image).
By that, use SYSTEMD_BOOTLOADER_INSTALL together with
IMAGER_INSTALL in configs.

Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 meta/conf/distro/debian-common.conf | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/meta/conf/distro/debian-common.conf b/meta/conf/distro/debian-common.conf
index 9203b34b..a2c23cb4 100644
--- a/meta/conf/distro/debian-common.conf
+++ b/meta/conf/distro/debian-common.conf
@@ -28,9 +28,9 @@ GRUB_BOOTLOADER_INSTALL_arm64 = "grub-efi-arm64-bin"
 
 SYSLINUX_BOOTLOADER_INSTALL = "syslinux syslinux-common"
 
-SYSTEMD_BOOTLOADER_PREINSTALL = "systemd"
-SYSTEMD_BOOTLOADER_PREINSTALL_debian-bookworm = "systemd-boot"
-SYSTEMD_BOOTLOADER_PREINSTALL_debian-sid-ports = "systemd-boot"
+SYSTEMD_BOOTLOADER_INSTALL = "systemd"
+SYSTEMD_BOOTLOADER_INSTALL_debian-bookworm = "systemd-boot"
+SYSTEMD_BOOTLOADER_INSTALL_debian-sid-ports = "systemd-boot"
 
 COMPAT_DISTRO_ARCH_amd64 = "i386"
 COMPAT_DISTRO_ARCH_arm64 = "armhf"
-- 
2.30.2

[PATCH 3/3] Install systemd-boot into buildchroot for qemuarm

[Felix Moessbauer]

This patch reworks the qemuarm target and installs
the systemd-boot dependency into the buildchroot
rather than the target chroot.

Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 meta-isar/conf/multiconfig/qemuarm-bookworm.conf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta-isar/conf/multiconfig/qemuarm-bookworm.conf b/meta-isar/conf/multiconfig/qemuarm-bookworm.conf
index c82abf30..126935b0 100644
--- a/meta-isar/conf/multiconfig/qemuarm-bookworm.conf
+++ b/meta-isar/conf/multiconfig/qemuarm-bookworm.conf
@@ -6,4 +6,4 @@ DISTRO ?= "debian-bookworm"
 IMAGE_FSTYPES_append = " wic"
 WKS_FILE ?= "sdimage-efi-sd"
 
-IMAGE_PREINSTALL += "${SYSTEMD_BOOTLOADER_PREINSTALL}"
+IMAGER_INSTALL += "${SYSTEMD_BOOTLOADER_INSTALL}"
-- 
2.30.2

Re: [PATCH 0/3] systemd-boot: Locate imager deps in buildchroot

[Bezdeka, Florian]

On Fri, 2022-08-12 at 08:50 +0200, Felix Moessbauer wrote:
> This patch reworks the logic how we create images with systemd-boot:
> Instead of installing the systemd-boot dependency into the target
> image, we install it in the buildchroot, leveraging the IMAGER_INSTALL
> infrastructure.

For some reasion 2/3 didn't make it into my inbox but it's on the list.
Just in case someone else is missing a patch.

IMHO patches 2 and 3 should be squashed together to stay bisectable.

I hope that was tested with Debian testing as well. Testing was the
reason for the change last time.

Florian

> 
> This has the following advantages:
> 
> - smaller images: The systemd-boot package is no longer installed into the image
> - unified imager logic with OE
> 
> Best regards,
> Felix
> 
> Felix Moessbauer (3):
>   wic: locate systemd-boot efi files in buildchroot
>   Rework imager support for systemd-boot
>   Install systemd-boot into buildchroot for qemuarm
> 
>  meta-isar/conf/multiconfig/qemuarm-bookworm.conf        | 2 +-
>  meta/conf/distro/debian-common.conf                     | 6 +++---
>  meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py | 6 +++---
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> -- 
> 2.30.2
> 

RE: [PATCH 0/3] systemd-boot: Locate imager deps in buildchroot

[Moessbauer, Felix]

> -----Original Message-----
> From: Bezdeka, Florian (T CED SES-DE) <florian.bezdeka@siemens.com>
> Sent: Friday, August 12, 2022 9:08 AM
> To: isar-users@googlegroups.com; Moessbauer, Felix (T CED SES-DE)
> <felix.moessbauer@siemens.com>
> Cc: amikan@ilbers.de; Kiszka, Jan (T CED) <jan.kiszka@siemens.com>; Schild,
> Henning (T CED SES-DE) <henning.schild@siemens.com>
> Subject: Re: [PATCH 0/3] systemd-boot: Locate imager deps in buildchroot
> 
> On Fri, 2022-08-12 at 08:50 +0200, Felix Moessbauer wrote:
> > This patch reworks the logic how we create images with systemd-boot:
> > Instead of installing the systemd-boot dependency into the target
> > image, we install it in the buildchroot, leveraging the IMAGER_INSTALL
> > infrastructure.
> 
> For some reasion 2/3 didn't make it into my inbox but it's on the list.
> Just in case someone else is missing a patch.
> 
> IMHO patches 2 and 3 should be squashed together to stay bisectable.

Both patches have a one-to-one relation to another patch from [1].
That's why I separated them, but I have no problem with squashing.

> 
> I hope that was tested with Debian testing as well. Testing was the reason for
> the change last time.

This has been tested on Debian testing, as well as a Debian bullseye layer where all system-* packages come from backports.
I can also confirm that the systemd-boot package is no longer installed in the image.

But I would be happy if more people could give this series a test.

Felix

[1] https://groups.google.com/g/isar-users/c/AM9L31ugDco/m/A9UVRL1cAgAJ

> 
> Florian
> 
> >
> > This has the following advantages:
> >
> > - smaller images: The systemd-boot package is no longer installed into
> > the image
> > - unified imager logic with OE
> >
> > Best regards,
> > Felix
> >
> > Felix Moessbauer (3):
> >   wic: locate systemd-boot efi files in buildchroot
> >   Rework imager support for systemd-boot
> >   Install systemd-boot into buildchroot for qemuarm
> >
> >  meta-isar/conf/multiconfig/qemuarm-bookworm.conf        | 2 +-
> >  meta/conf/distro/debian-common.conf                     | 6 +++---
> >  meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py | 6 +++---
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > --
> > 2.30.2
> >

Re: [PATCH 0/3] systemd-boot: Locate imager deps in buildchroot

[Henning Schild]

Am Fri, 12 Aug 2022 08:50:36 +0200
schrieb Felix Moessbauer <felix.moessbauer@siemens.com>:

> This patch reworks the logic how we create images with systemd-boot:
> Instead of installing the systemd-boot dependency into the target
> image, we install it in the buildchroot, leveraging the IMAGER_INSTALL
> infrastructure.
> 
> This has the following advantages:
> 
> - smaller images: The systemd-boot package is no longer installed
> into the image
> - unified imager logic with OE

Good idea. You could also look at my last series of bumping wic and
reforking all the bootloader bits, enabling systemd-boot in the first
place.

That bootloader used to stick out because it was the only one to install
"from withing inside the target rootfs", as opposed to "from outside".

systemd itself is very likely to be installed there and the bootloader
used to be a part of that main package. But with >11 it has been split
out into its own package, so indeed even if the target runs systemd we
might not need to install that package. In fact we could probably even
use systemd-boot for a system using openrc or sysvinit.

I do not remember all the details.

But i do remember that i actually wanted to eventually move all
bootloaders to "inside" and maybe even drop support for "outside". That
would be to enable kernel updates in isar generated images.
And if one wanted boot loader updates the bootloaders would also have
to become part of the target install set.

regards,
Henning

> Best regards,
> Felix
> 
> Felix Moessbauer (3):
>   wic: locate systemd-boot efi files in buildchroot
>   Rework imager support for systemd-boot
>   Install systemd-boot into buildchroot for qemuarm
> 
>  meta-isar/conf/multiconfig/qemuarm-bookworm.conf        | 2 +-
>  meta/conf/distro/debian-common.conf                     | 6 +++---
>  meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py | 6 +++---
>  3 files changed, 7 insertions(+), 7 deletions(-)
>