ilbers / isar

Integration System for Automated Root filesystem generation
Other
177 stars 72 forks source link

`BootimgPcbiosIsarPlugin` wic plugin files fails for lack of syslinux #69

Open ydirson opened 2 years ago

ydirson commented 2 years ago

I can generate an UEFI image with the following settings:

MACHINE ??= "qemuamd64"
DISTRO ??= "debian-buster"

To get a legacy/pcbios image, I try WKS_FILE = "directdisk-isar.wks" or directdisk-gpt.wks ... which fail looking for syslinux, which is apparently not pulled. Some missing dependency ?

ERROR: mc:qemuamd64-buster:isar-image-base-1.0-r0 do_wic_image: Error executing a python function in exec_python_func() autogenerated:

The stack trace of python calls that resulted in this exception/failure was:
File: 'exec_python_func() autogenerated', lineno: 2, function: <module>
     0001:
 *** 0002:do_wic_image(d)
     0003:
File: '/home/user/isar/meta/classes/wic-img.bbclass', lineno: 141, function: do_wic_image
     0137:}
     0138:
     0139:do_wic_image[file-checksums] += "${WKS_FILE_CHECKSUM}"
     0140:python do_wic_image() {
 *** 0141:    bb.build.exec_func("generate_wic_image", d)
     0142:    bb.build.exec_func("check_for_wic_warnings", d)
     0143:}
     0144:addtask wic_image before do_image after do_image_tools
     0145:
File: '/home/user/isar/bitbake/lib/bb/build.py', lineno: 251, function: exec_func
     0247:    with bb.utils.fileslocked(lockfiles):
     0248:        if ispython:
     0249:            exec_func_python(func, d, runfile, cwd=adir)
     0250:        else:
 *** 0251:            exec_func_shell(func, d, runfile, cwd=adir)
     0252:
     0253:    try:
     0254:        curcwd = os.getcwd()
     0255:    except:
File: '/home/user/isar/bitbake/lib/bb/build.py', lineno: 452, function: exec_func_shell
     0448:    with open(fifopath, 'r+b', buffering=0) as fifo:
     0449:        try:
     0450:            bb.debug(2, "Executing shell function %s" % func)
     0451:            with open(os.devnull, 'r+') as stdin, logfile:
 *** 0452:                bb.process.run(cmd, shell=False, stdin=stdin, log=logfile, extrafiles=[(fifo,readfifo)])
     0453:        finally:
     0454:            os.unlink(fifopath)
     0455:
     0456:    bb.debug(2, "Shell function %s finished" % func)
File: '/home/user/isar/bitbake/lib/bb/process.py', lineno: 182, function: run
     0178:        if not stderr is None:
     0179:            stderr = stderr.decode("utf-8")
     0180:
     0181:    if pipe.returncode != 0:
 *** 0182:        raise ExecutionError(cmd, pipe.returncode, stdout, stderr)
     0183:    return stdout, stderr
Exception: bb.process.ExecutionError: Execution of '/home/user/build/tmp/work/debian-buster-amd64/isar-image-base-qemuamd64-wic-img/1.0-r0/temp/run.generate_wic_image.26597' failed with exit code 1:
INFO: Creating image(s)...

ERROR: Couldn't find syslinux directory, exiting

WARNING: exit code 1 from a shell command.

ERROR: Logfile of failure stored in: /home/user/build/tmp/work/debian-buster-amd64/isar-image-base-qemuamd64-wic-img/1.0-r0/temp/log.do_wic_image.26597
ERROR: Task (mc:qemuamd64-buster:/home/user/isar/meta-isar/recipes-core/images/isar-image-base.bb:do_wic_image) failed with exit code '1'

Diggig a bit I see that BootimgPcbiosIsarPlugin is heavily depending in syslinux. However, it does not look like there is any recipe to get the latter into STAGING_DATADIR.

ismagulb commented 2 years ago

I've tried qemuamd64.conf with WKS_FILE = "directdisk-isar.wks" but get a different error message from run.generate_wic_image. Did you do some other steps / how can I reproduce your problem?

ydirson commented 2 years ago

This is in a Debian10 VM, the following steps reproduce the error:

user@isar-build:~/isar$ git clean -fdx
user@isar-build:~/isar$ git describe --dirty
v0.7-436-g03124cc
user@isar-build:~/isar$ . isar-init-build-env ../build2
...
user@isar-build:~/build2$ cat >> conf/local.conf 
MACHINE = "qemuamd64"
DISTRO = "debian-buster"
DISTRO_ARCH = "amd64"
WKS_FILE = "directdisk-isar.wks"
user@isar-build:~/build2$ bitbake mc:qemuamd64-buster:isar-image-base
henning-schild commented 2 years ago

IMAGER_INSTALL += "${SYSLINUX_BOOTLOADER_INSTALL}"

ismagulb commented 2 years ago

Thanks Henning, worked for me.

ydirson commented 2 years ago

I confirm, adding this to eg. local.conf does the trick. Getting it to be selected automatically seems more tricky, though.

henning-schild commented 2 years ago

Well it can be kind of found in the examples. IMAGER_INSTALL adds stuff to the buildchroot that is needed for the do_image task. What that will be depends on which image class you choose. In the case of wic it can even differ depending on which plugins you use in your WKS_FILE. When choosing "pcbios" it will be "syslinux", for "efi" it can be "grub" (but can also be empty when using systemd-boot).

Not sure how really to automate the choice. In fact we could always add "syslinux" and "grub" as potentially needed runtime deps of wic. Adding a few too many packages to buildchroot does not hurt, will not grow the the target image in terms of size or manifest. But doing so would have to take into account that some packages are not available in all arches or change their names.

i.e. "syslinux" is amd64/i386-only while "grub" gets called many names

meta/conf/distro/debian-common.conf

21:GRUB_BOOTLOADER_INSTALL_amd64 = "grub-efi-amd64-bin"
22:GRUB_BOOTLOADER_INSTALL_i386 = "grub-efi-ia32-bin"
23:GRUB_BOOTLOADER_INSTALL_armhf = "grub-efi-arm-bin"
24:GRUB_BOOTLOADER_INSTALL_arm64 = "grub-efi-arm64-bin"

My guess is that the hint i gave is enough to close this issue. Anyways you should not be using pcbios unless you really really have to deal with such legacy.

henning-schild commented 2 years ago

@ydirson your git describe looks like "master" branch ... in Isar "next" is usually better and not rebasing while "master" is often lacking behind and might have bugs already fixed in "next"

henning-schild commented 2 years ago

And note that i only found this issue because of a cross-post to the Isar mailing-list. Issues on github or not the best channel to get in touch, mailing-list posts would be better because they have a wider reach into the "small" community.

ydirson commented 2 years ago

Note(s) taken, thanks!

ismagulb commented 2 years ago

@ydirson, if this worked for you, can we close this issue?

henning-schild commented 2 years ago

@ismagulb what works better is is not too rude is to close saying "please re-open if issue persists"

ydirson commented 2 years ago

I understand the difficulty of making this transparent to the user, and since the user will have to take care of this, wouldn't it be better to make this more visible in the doc ?

ismagulb commented 2 years ago

Good point, let me check what we can do. If you have a specific idea, a patch would be great.

henning-schild commented 2 years ago

Concluding that this issue is solved, like already confirmed in https://github.com/ilbers/isar/issues/69#issuecomment-923083323

Possibly needed doc improvements might deserve a new issue.

ydirson commented 2 years ago

Well it can be kind of found in the examples. IMAGER_INSTALL adds stuff to the buildchroot that is needed for the do_image task. What that will be depends on which image class you choose. In the case of wic it can even differ depending on which plugins you use in your WKS_FILE. When choosing "pcbios" it will be "syslinux", for "efi" it can be "grub" (but can also be empty when using systemd-boot).

Thinking twice about that, we can expand variables inside .wks files. If we use this to specify plugins from a bitbake variable, we could probably use the same variable to trigger the addition of the proper dependencies to IMAGER_INSTALL ?

jan-kiszka commented 2 years ago

Simplifications are always welcome, but I cannot follow yours yet. Can you make a concrete example?

ydirson commented 2 years ago

Here is a PoC for just directdisk-isar, with only definitions for the pcbios case. Major problem would probably be that all users would have to change .wks to .wks.in, so maybe both could have to be kept for compatibility (which kinda sucks).

Maybe wic-img.bbclass would be a better fit than image.bbclass, BTW.

diff --git a/meta-isar/scripts/lib/wic/canned-wks/common-isar.wks.inc b/meta-isar/scripts/lib/wic/canned-wks/common-isar.wks.inc
index a65e646..2607edb 100644
--- a/meta-isar/scripts/lib/wic/canned-wks/common-isar.wks.inc
+++ b/meta-isar/scripts/lib/wic/canned-wks/common-isar.wks.inc
@@ -1,3 +1,3 @@
 # This file is included into 3 canned wks files from this directory
-part /boot --source bootimg-pcbios-isar --ondisk sda --label boot --active --align 1024
+part /boot --source ${BOOTIMG_PLUGIN} --ondisk sda --label boot --active --align 1024
 part / --source rootfs --ondisk sda --fstype=ext4 --mkfs-extraopts "-T default" --label platform --align 1024 --exclude-path=boot
diff --git a/meta-isar/scripts/lib/wic/canned-wks/directdisk-isar.wks b/meta-isar/scripts/lib/wic/canned-wks/directdisk-isar.wks.in
similarity index 100%
rename from meta-isar/scripts/lib/wic/canned-wks/directdisk-isar.wks
rename to meta-isar/scripts/lib/wic/canned-wks/directdisk-isar.wks.in
diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 5a0f32e..021c860 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -15,6 +15,14 @@ IMAGE_INSTALL += "${@ ("linux-image-" + d.getVar("KERNEL_NAME", True)) if d.getV
 # Name of the image including distro&machine names
 IMAGE_FULLNAME = "${PN}-${DISTRO}-${MACHINE}"

+# These variables are used to keep in sync wic plugin and matching dependencies
+BOOTIMG_TYPE ?= "pcbios"
+BOOTIMG_PLUGIN_pcbios = "bootimg-pcbios-isar"
+BOOTIMG_INSTALL_pcbios = "${SYSLINUX_BOOTLOADER_INSTALL}"
+BOOTIMG_PLUGIN = "${BOOTIMG_PLUGIN_${BOOTIMG_TYPE}}"
+IMAGER_INSTALL += "${BOOTIMG_INSTALL_${BOOTIMG_TYPE}}"
+WICVARS_append = " BOOTIMG_PLUGIN"
+
 # These variables are used by wic and start_vm
 KERNEL_IMAGE ?= "${IMAGE_FULLNAME}-${KERNEL_FILE}"
 INITRD_IMAGE ?= "${IMAGE_FULLNAME}-initrd.img"
jan-kiszka commented 2 years ago

And you would BOOTIMG_TYPE basically where you also set WKS_FILE

Yeah, this would be the approach from a technical perspective. From a user perspective, selecting bootimg-* in the wks file should simple do The Right Thing, automatically. Maybe we should parse the selected WKS_FILE for bootimg entries and extend IMAGER_INSTALL automatically? Somewhere in wic-img.bbclass.

ydirson commented 2 years ago

I'm not sure it is more "right" to select a particular plugin in the .wks, than to specifiy the intent at a higher level, especially as it would also open the possibility to have a single .wks for both efi and pcbios boot (lifting --source-params into a bb var too). In yocto, as a user I've often felt the .wks as an implementation detail: only when the user wants to tune the partitions is it necessary to dive into wic. I feel it would improve the learning curve to have the parameters most in need of tuning all easily set in local.conf.

And you would BOOTIMG_TYPE basically where you also set WKS_FILE

Yes, and it could also maybe be better named like WKS_FILE_BOOTIMG_TYPE to stress the link. Although I'm not completely sure here: this parameter could also be used in other places, eg. to build a qemu commandline, or even to select alternative bootchains (eg. "legacy" vs. uefi on arm64).

Note: it looks like the renaming to .wks.in may not be needed in the end.

jan-kiszka commented 2 years ago

In the ideal world, the wks file is supposed to describe the partitioning and its parameters, not a higher level.

ydirson commented 2 years ago

In the ideal world, the wks file is supposed to describe the partitioning and its parameters, not a higher level.

Not necessarily all of them (especially when other parts of the system depend on the same high-level requirement), and it's probably a good part of why there is variable substitution in there. And I still feel that selecting the target bootchain is more high-level than the specific wic plugin implementing it.

See eg. in meta-rockchip:

bootloader --ptable gpt --append="console=tty1 console=${RK_CONSOLE_DEVICE},${RK_CONSOLE_BAUD}n8 rw rootfstype=ext4 init=/sbin/init"
jan-kiszka commented 2 years ago

I agree for the given type of example. But I disagree about the boot method. You may only see bootimg-* right now, but the boot loader selection is broader and generally influences what needs to be IMAGER_INSTALL (u-boot, efibootguard, etc.).

ydirson commented 2 years ago

Maybe then more information could be added together with https://github.com/ilbers/isar/issues/69#ref-commit-ae7bdb5 ?

henning-schild commented 2 years ago

In fact it remains a hack that we install the bootloader in the buildchroot and not in the target rootfs. A hack that applies to grub and syslinux but not to u-boot or systemd-boot, not sure about efibootguard which only lives in layers but not in isar.

While that keeps the rootfs small it also has issues. i.e. we see problems when doing kernel updates with apt, because we lack grub-mkconfig. And our bill of materials (manifest) does not list bootloaders that are not installed in the rootfs but still make it into the image.

My personal long term plan would be to install the bootloader inside the rootfs, no matter which bootloader that may be.

We have several problems on that front, and the more we discuss it here the more we leave out the mailinglist while discovering more issues/solutions. So again, please close and move to where isar discussions are supposed to happen!

henning-schild commented 2 years ago

https://groups.google.com/g/isar-users/c/nEv2P_HJjEs/m/p7ivoU7SAAAJ