Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

quic-vkraleti
Copy link
Contributor

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
Copy link
Collaborator

lumag commented Oct 1, 2024

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Collaborator

lumag commented Oct 5, 2024

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
Copy link
Collaborator

lumag commented Oct 5, 2024

Updated previous comment to provide better justification.

@ricardosalveti
Copy link
Contributor

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
Copy link
Collaborator

lumag commented Oct 9, 2024

@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
Copy link
Contributor

@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.

Move KERNEL_IMAGETYPES setting from qcom-common.inc to qcom-armv8a.conf
as former is common for both qcom-armv7a and qcom-armv8a machines. We
need "Image" type kernel only for qcom-armv8a to support UKI.

Signed-off-by: Viswanath Kraleti <[email protected]>
@lumag
Copy link
Collaborator

lumag commented Oct 11, 2024

I will fix the CI

@lumag lumag merged commit fc029c4 into Linaro:master Oct 11, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants