Linaro / meta-qcom

OpenEmbedded/Yocto Project BSP layer for Qualcomm based platforms
MIT License
64 stars 76 forks source link

Don't list 'Image' multiple times in KERNEL_IMAGETYPES #661

Closed quic-vkraleti closed 1 month ago

quic-vkraleti commented 1 month ago

Listing 'Image' multiple times in KERNEL_IMAGETYPES list leads to fatal QA errors while packaging [packages-list]. Using += operator instead of :append to add to list helps to avoid duplication.

lumag commented 1 month ago

Bitbake doesn't document duplicate removal for the += operator. Could you please provide corresponding (only KERNEL_IMAGETYPES part) dumps of bitbake -e virtual/kernel, when using += and when using :append.

quic-vkraleti commented 1 month ago

Yes bitbake document doesn't say and in fact bitbake isn't removing duplicates with +=. This particular behavior seems to be specific to handling KERNEL_IMAGETYPES in kernel.bbclass. The pre condition to reproduce this problem is to set KERNEL_IMAGETYPE to "Image". With this bitbake -e virtual/kernel |grep KERNEL_IMAGETYPE output is

KERNEL_IMAGETYPES:append = " Image" KERNEL_IMAGETYPES += "Image"
# $KERNEL_IMAGETYPE [4 operations]
KERNEL_IMAGETYPE="Image"
# $KERNEL_IMAGETYPES [4 operations]
# [_defaultval] "${KERNEL_IMAGETYPE}"
# [doc] "The list of types of kernel to build for a device, usually set by the machine configuration files and defaults to KERNEL_IMAGETYPE."
KERNEL_IMAGETYPES="Image Image"
# $KERNEL_IMAGETYPE [4 operations]
KERNEL_IMAGETYPE="Image"
# $KERNEL_IMAGETYPES [4 operations]
# [_defaultval] "${KERNEL_IMAGETYPE}"
# [doc] "The list of types of kernel to build for a device, usually set by the machine configuration files and defaults to KERNEL_IMAGETYPE."
KERNEL_IMAGETYPES="Image"

Seems the timing of appending the value in kernel.bblcass is resulting in this behavior. As kernel.bbclass is resetting KERNEL_IMAGETYPES by merging KERNEL_IMAGETYPE & KERNEL_ALT_IMAGETYPE may be it is good if KERNEL_IMAGETYPE is updated instead of KERNEL_IMAGETYPES.

# Merge KERNEL_IMAGETYPE and KERNEL_ALT_IMAGETYPE into KERNEL_IMAGETYPES
type = d.getVar('KERNEL_IMAGETYPE') or ""
alttype = d.getVar('KERNEL_ALT_IMAGETYPE') or ""
types = d.getVar('KERNEL_IMAGETYPES') or ""
if type not in types.split():
    types = (type + ' ' + types).strip()
if alttype not in types.split():
    types = (alttype + ' ' + types).strip()
d.setVar('KERNEL_IMAGETYPES', types)
quaresmajose commented 1 month ago

Seems the timing of appending the value in kernel.bblcass is resulting in this behavior. As kernel.bbclass is resetting KERNEL_IMAGETYPES by merging KERNEL_IMAGETYPE & KERNEL_ALT_IMAGETYPE may be it is good if KERNEL_IMAGETYPE is updated instead of KERNEL_IMAGETYPES.

I think is better to replace the KERNEL_IMAGETYPES by KERNEL_IMAGETYPE on layer as the first one is managed by the kernel.bbclass.

lumag commented 1 month ago

According to the documentation KERNEL_IMAGETYPES lists types additional to the one specified in KERNEL_IMAGETYPE.

So, specifying the type in KERNEL_IMAGETYPE and then adding it to KERNEL_IMAGETYPES is an error.

My suggestion would be to stop using per-machine configs. Please use qcom-armv8 instead. This machine config sets values correctly: KERNEL_IMAGETYPE is set to Image.gz, while KERNEL_IMAGETYPES gets extended with just Image.

lumag commented 1 month ago

Updated previous comment to provide better justification.

ricardosalveti commented 1 month ago

I think we should drop this KERNEL_IMAGETYPES settings here, first because it is in common, which is used by both armv7 and armv8, and second because this should really be machine specific in the end.

Taking qcom-armv8a as an example, we can probably just change KERNEL_IMAGETYPES to have both Image and Image.gz, because in the end it is always useful to have compressed and uncompressed images (taking into account that UKI/systemd-boot needs uncompressed images by default).

Then we should have:

diff --git a/conf/machine/include/qcom-common.inc b/conf/machine/include/qcom-common.inc
index 6682b8b..9de2ad5 100644
--- a/conf/machine/include/qcom-common.inc
+++ b/conf/machine/include/qcom-common.inc
@@ -58,6 +58,3 @@ EFI_LINUX_IMG_DTB ?= ""

 # Place dtb at EFIDTDIR to seamlessly package
 KERNEL_DTBDEST = "${EFI_DTB_DIR}"
-
-# UKI generation needs uncompressed Kernel image
-KERNEL_IMAGETYPES:append = " Image"
diff --git a/conf/machine/qcom-armv8a.conf b/conf/machine/qcom-armv8a.conf
index f6acd84..b8ab8cc 100644
--- a/conf/machine/qcom-armv8a.conf
+++ b/conf/machine/qcom-armv8a.conf
@@ -15,7 +15,8 @@ UBOOT_CONFIG[sdm845-db845c] = "qcom_defconfig"
 PREFERRED_PROVIDER_virtual/kernel ?= "linux-yocto"

 # Support for dragonboard{410, 820, 845}c, rb5
-KERNEL_IMAGETYPE ?= "Image.gz"
+KERNEL_IMAGETYPE ?= "Image"
+KERNEL_IMAGETYPES ?= "Image.gz Image"
 SERIAL_CONSOLE ?= "115200 ttyMSM0"
 KERNEL_DEVICETREE ?= " \
     qcom/apq8016-sbc.dtb \

And then we can add a similar change to whatever other board conf that is including this one.

lumag commented 1 month ago

@ricardosalveti I'd prefer to keep Image.gz as a primary kernel type (aka KERNEL_IMAGETYPE). So, I think, it should be enough to add KERNEL_IMAGETYPES = "Image" to qcom-armv8a.conf and drop the KERNEL_IMAGETYPES:append from qcom-common.inc. WDYT?

ricardosalveti commented 1 month ago

@ricardosalveti I'd prefer to keep Image.gz as a primary kernel type (aka KERNEL_IMAGETYPE). So, I think, it should be enough to add KERNEL_IMAGETYPES = "Image" to qcom-armv8a.conf and drop the KERNEL_IMAGETYPES:append from qcom-common.inc. WDYT?

Sure, that works as well, since the class merges KERNEL_IMAGETYPE into KERNEL_IMAGETYPES.

lumag commented 1 month ago

I will fix the CI