[PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[Mike Looijmans]

kernel-fitimage inherited uboot-config, which resulted in failure to
run "menuconfig" on a system that has both multiple u-boot configs and
wants a fitImage kernel.

Fix the issue by splitting up uboot-config into uboot-config-vars that
only contains the "harmless" variable settings, and inherit that in a
new uboot-config class which contains the Python code to do the U-boot
specific settings. Inherit only uboot-config-vars in kernel-fitimage
so that it has what it needs but doesn't break.

Fixes: f9e834e31788 "meta: introduce KCONFIG_CONFIG_ENABLE_MENUCONFIG"
Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 meta/classes-recipe/kernel-fitimage.bbclass   |  2 +-
 meta/classes-recipe/uboot-config-vars.bbclass | 95 +++++++++++++++++++
 meta/classes-recipe/uboot-config.bbclass      | 80 +---------------
 3 files changed, 97 insertions(+), 80 deletions(-)
 create mode 100644 meta/classes-recipe/uboot-config-vars.bbclass

diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass
index 4b74ddc201..070a822b43 100644
--- a/meta/classes-recipe/kernel-fitimage.bbclass
+++ b/meta/classes-recipe/kernel-fitimage.bbclass
@@ -4,7 +4,7 @@
 # SPDX-License-Identifier: MIT
 #
 
-inherit kernel-uboot kernel-artifact-names uboot-config
+inherit kernel-uboot kernel-artifact-names uboot-config-vars
 
 def get_fit_replacement_type(d):
     kerneltypes = d.getVar('KERNEL_IMAGETYPES') or ""
diff --git a/meta/classes-recipe/uboot-config-vars.bbclass b/meta/classes-recipe/uboot-config-vars.bbclass
new file mode 100644
index 0000000000..5714ace579
--- /dev/null
+++ b/meta/classes-recipe/uboot-config-vars.bbclass
@@ -0,0 +1,95 @@
+# Handle U-Boot config for a machine
+#
+# The format to specify it, in the machine, is:
+#
+# UBOOT_CONFIG ??= <default>
+# UBOOT_CONFIG[foo] = "config,images,binary"
+#
+# or
+#
+# UBOOT_MACHINE = "config"
+#
+# Copyright 2013, 2014 (C) O.S. Systems Software LTDA.
+#
+# SPDX-License-Identifier: MIT
+
+
+def removesuffix(s, suffix):
+    if suffix and s.endswith(suffix):
+        return s[:-len(suffix)]
+    return s
+
+UBOOT_ENTRYPOINT ?= "20008000"
+UBOOT_LOADADDRESS ?= "${UBOOT_ENTRYPOINT}"
+
+# Some versions of u-boot use .bin and others use .img.  By default use .bin
+# but enable individual recipes to change this value.
+UBOOT_SUFFIX ??= "bin"
+UBOOT_BINARY ?= "u-boot.${UBOOT_SUFFIX}"
+UBOOT_BINARYNAME ?= "${@os.path.splitext(d.getVar("UBOOT_BINARY"))[0]}"
+UBOOT_IMAGE ?= "${UBOOT_BINARYNAME}-${MACHINE}-${PV}-${PR}.${UBOOT_SUFFIX}"
+UBOOT_SYMLINK ?= "${UBOOT_BINARYNAME}-${MACHINE}.${UBOOT_SUFFIX}"
+UBOOT_MAKE_TARGET ?= "all"
+
+# Output the ELF generated. Some platforms can use the ELF file and directly
+# load it (JTAG booting, QEMU) additionally the ELF can be used for debugging
+# purposes.
+UBOOT_ELF ?= ""
+UBOOT_ELF_SUFFIX ?= "elf"
+UBOOT_ELF_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.${UBOOT_ELF_SUFFIX}"
+UBOOT_ELF_BINARY ?= "u-boot.${UBOOT_ELF_SUFFIX}"
+UBOOT_ELF_SYMLINK ?= "u-boot-${MACHINE}.${UBOOT_ELF_SUFFIX}"
+
+# Some versions of u-boot build an SPL (Second Program Loader) image that
+# should be packaged along with the u-boot binary as well as placed in the
+# deploy directory.  For those versions they can set the following variables
+# to allow packaging the SPL.
+SPL_SUFFIX ?= ""
+SPL_BINARY ?= ""
+SPL_DELIMITER  ?= "${@'.' if d.getVar("SPL_SUFFIX") else ''}"
+SPL_BINARYFILE ?= "${@os.path.basename(d.getVar("SPL_BINARY"))}"
+SPL_BINARYNAME ?= "${@removesuffix(d.getVar("SPL_BINARYFILE"), "." + d.getVar("SPL_SUFFIX"))}"
+SPL_IMAGE ?= "${SPL_BINARYNAME}-${MACHINE}-${PV}-${PR}${SPL_DELIMITER}${SPL_SUFFIX}"
+SPL_SYMLINK ?= "${SPL_BINARYNAME}-${MACHINE}${SPL_DELIMITER}${SPL_SUFFIX}"
+
+# Additional environment variables or a script can be installed alongside
+# u-boot to be used automatically on boot.  This file, typically 'uEnv.txt'
+# or 'boot.scr', should be packaged along with u-boot as well as placed in the
+# deploy directory.  Machine configurations needing one of these files should
+# include it in the SRC_URI and set the UBOOT_ENV parameter.
+UBOOT_ENV_SUFFIX ?= "txt"
+UBOOT_ENV ?= ""
+UBOOT_ENV_SRC_SUFFIX ?= "cmd"
+UBOOT_ENV_SRC ?= "${UBOOT_ENV}.${UBOOT_ENV_SRC_SUFFIX}"
+UBOOT_ENV_BINARY ?= "${UBOOT_ENV}.${UBOOT_ENV_SUFFIX}"
+UBOOT_ENV_IMAGE ?= "${UBOOT_ENV}-${MACHINE}-${PV}-${PR}.${UBOOT_ENV_SUFFIX}"
+UBOOT_ENV_SYMLINK ?= "${UBOOT_ENV}-${MACHINE}.${UBOOT_ENV_SUFFIX}"
+
+# U-Boot EXTLINUX variables. U-Boot searches for /boot/extlinux/extlinux.conf
+# to find EXTLINUX conf file.
+UBOOT_EXTLINUX_INSTALL_DIR ?= "/boot/extlinux"
+UBOOT_EXTLINUX_CONF_NAME ?= "extlinux.conf"
+UBOOT_EXTLINUX_SYMLINK ?= "${UBOOT_EXTLINUX_CONF_NAME}-${MACHINE}-${PR}"
+
+# Options for the device tree compiler passed to mkimage '-D' feature:
+UBOOT_MKIMAGE_DTCOPTS ??= ""
+SPL_MKIMAGE_DTCOPTS ??= ""
+
+# mkimage command
+UBOOT_MKIMAGE ?= "uboot-mkimage"
+UBOOT_MKIMAGE_SIGN ?= "${UBOOT_MKIMAGE}"
+
+# Signature activation - this requires KERNEL_IMAGETYPE = "fitImage"
+UBOOT_SIGN_ENABLE ?= "0"
+
+# Arguments passed to mkimage for signing
+UBOOT_MKIMAGE_SIGN_ARGS ?= ""
+SPL_MKIMAGE_SIGN_ARGS ?= ""
+
+# Options to deploy the u-boot device tree
+UBOOT_DTB ?= ""
+UBOOT_DTB_BINARY ??= ""
+
+# uboot-fit_check_sign command
+UBOOT_FIT_CHECK_SIGN ?= "uboot-fit_check_sign"
+
diff --git a/meta/classes-recipe/uboot-config.bbclass b/meta/classes-recipe/uboot-config.bbclass
index e55fc38b7c..79e6093b6a 100644
--- a/meta/classes-recipe/uboot-config.bbclass
+++ b/meta/classes-recipe/uboot-config.bbclass
@@ -13,85 +13,7 @@
 #
 # SPDX-License-Identifier: MIT
 
-
-def removesuffix(s, suffix):
-    if suffix and s.endswith(suffix):
-        return s[:-len(suffix)]
-    return s
-
-UBOOT_ENTRYPOINT ?= "20008000"
-UBOOT_LOADADDRESS ?= "${UBOOT_ENTRYPOINT}"
-
-# Some versions of u-boot use .bin and others use .img.  By default use .bin
-# but enable individual recipes to change this value.
-UBOOT_SUFFIX ??= "bin"
-UBOOT_BINARY ?= "u-boot.${UBOOT_SUFFIX}"
-UBOOT_BINARYNAME ?= "${@os.path.splitext(d.getVar("UBOOT_BINARY"))[0]}"
-UBOOT_IMAGE ?= "${UBOOT_BINARYNAME}-${MACHINE}-${PV}-${PR}.${UBOOT_SUFFIX}"
-UBOOT_SYMLINK ?= "${UBOOT_BINARYNAME}-${MACHINE}.${UBOOT_SUFFIX}"
-UBOOT_MAKE_TARGET ?= "all"
-
-# Output the ELF generated. Some platforms can use the ELF file and directly
-# load it (JTAG booting, QEMU) additionally the ELF can be used for debugging
-# purposes.
-UBOOT_ELF ?= ""
-UBOOT_ELF_SUFFIX ?= "elf"
-UBOOT_ELF_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.${UBOOT_ELF_SUFFIX}"
-UBOOT_ELF_BINARY ?= "u-boot.${UBOOT_ELF_SUFFIX}"
-UBOOT_ELF_SYMLINK ?= "u-boot-${MACHINE}.${UBOOT_ELF_SUFFIX}"
-
-# Some versions of u-boot build an SPL (Second Program Loader) image that
-# should be packaged along with the u-boot binary as well as placed in the
-# deploy directory.  For those versions they can set the following variables
-# to allow packaging the SPL.
-SPL_SUFFIX ?= ""
-SPL_BINARY ?= ""
-SPL_DELIMITER  ?= "${@'.' if d.getVar("SPL_SUFFIX") else ''}"
-SPL_BINARYFILE ?= "${@os.path.basename(d.getVar("SPL_BINARY"))}"
-SPL_BINARYNAME ?= "${@removesuffix(d.getVar("SPL_BINARYFILE"), "." + d.getVar("SPL_SUFFIX"))}"
-SPL_IMAGE ?= "${SPL_BINARYNAME}-${MACHINE}-${PV}-${PR}${SPL_DELIMITER}${SPL_SUFFIX}"
-SPL_SYMLINK ?= "${SPL_BINARYNAME}-${MACHINE}${SPL_DELIMITER}${SPL_SUFFIX}"
-
-# Additional environment variables or a script can be installed alongside
-# u-boot to be used automatically on boot.  This file, typically 'uEnv.txt'
-# or 'boot.scr', should be packaged along with u-boot as well as placed in the
-# deploy directory.  Machine configurations needing one of these files should
-# include it in the SRC_URI and set the UBOOT_ENV parameter.
-UBOOT_ENV_SUFFIX ?= "txt"
-UBOOT_ENV ?= ""
-UBOOT_ENV_SRC_SUFFIX ?= "cmd"
-UBOOT_ENV_SRC ?= "${UBOOT_ENV}.${UBOOT_ENV_SRC_SUFFIX}"
-UBOOT_ENV_BINARY ?= "${UBOOT_ENV}.${UBOOT_ENV_SUFFIX}"
-UBOOT_ENV_IMAGE ?= "${UBOOT_ENV}-${MACHINE}-${PV}-${PR}.${UBOOT_ENV_SUFFIX}"
-UBOOT_ENV_SYMLINK ?= "${UBOOT_ENV}-${MACHINE}.${UBOOT_ENV_SUFFIX}"
-
-# U-Boot EXTLINUX variables. U-Boot searches for /boot/extlinux/extlinux.conf
-# to find EXTLINUX conf file.
-UBOOT_EXTLINUX_INSTALL_DIR ?= "/boot/extlinux"
-UBOOT_EXTLINUX_CONF_NAME ?= "extlinux.conf"
-UBOOT_EXTLINUX_SYMLINK ?= "${UBOOT_EXTLINUX_CONF_NAME}-${MACHINE}-${PR}"
-
-# Options for the device tree compiler passed to mkimage '-D' feature:
-UBOOT_MKIMAGE_DTCOPTS ??= ""
-SPL_MKIMAGE_DTCOPTS ??= ""
-
-# mkimage command
-UBOOT_MKIMAGE ?= "uboot-mkimage"
-UBOOT_MKIMAGE_SIGN ?= "${UBOOT_MKIMAGE}"
-
-# Signature activation - this requires KERNEL_IMAGETYPE = "fitImage"
-UBOOT_SIGN_ENABLE ?= "0"
-
-# Arguments passed to mkimage for signing
-UBOOT_MKIMAGE_SIGN_ARGS ?= ""
-SPL_MKIMAGE_SIGN_ARGS ?= ""
-
-# Options to deploy the u-boot device tree
-UBOOT_DTB ?= ""
-UBOOT_DTB_BINARY ??= ""
-
-# uboot-fit_check_sign command
-UBOOT_FIT_CHECK_SIGN ?= "uboot-fit_check_sign"
+inherit uboot-config-vars
 
 python () {
     ubootmachine = d.getVar("UBOOT_MACHINE")
-- 
2.34.1


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[Richard Purdie]

On Tue, 2024-05-21 at 15:57 +0200, Mike Looijmans via lists.openembedded.org wrote:
> kernel-fitimage inherited uboot-config, which resulted in failure to
> run "menuconfig" on a system that has both multiple u-boot configs and
> wants a fitImage kernel.
> 
> Fix the issue by splitting up uboot-config into uboot-config-vars that
> only contains the "harmless" variable settings, and inherit that in a
> new uboot-config class which contains the Python code to do the U-boot
> specific settings. Inherit only uboot-config-vars in kernel-fitimage
> so that it has what it needs but doesn't break.
> 
> Fixes: f9e834e31788 "meta: introduce KCONFIG_CONFIG_ENABLE_MENUCONFIG"
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
>  meta/classes-recipe/kernel-fitimage.bbclass   |  2 +-
>  meta/classes-recipe/uboot-config-vars.bbclass | 95 +++++++++++++++++++
>  meta/classes-recipe/uboot-config.bbclass      | 80 +---------------
>  3 files changed, 97 insertions(+), 80 deletions(-)
>  create mode 100644 meta/classes-recipe/uboot-config-vars.bbclass

This looks like it might be a good candidate to move to a file in
meta/conf rather than a new class file?

We could add any functions needed to lib/oe/ if they can be made
suitably generic.

Cheers,

Richard

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[mike.looijmans]

On 21-05-2024 16:51, Richard Purdie wrote:
> On Tue, 2024-05-21 at 15:57 +0200, Mike Looijmans via lists.openembedded.org wrote:
>> kernel-fitimage inherited uboot-config, which resulted in failure to
>> run "menuconfig" on a system that has both multiple u-boot configs and
>> wants a fitImage kernel.
>>
>> Fix the issue by splitting up uboot-config into uboot-config-vars that
>> only contains the "harmless" variable settings, and inherit that in a
>> new uboot-config class which contains the Python code to do the U-boot
>> specific settings. Inherit only uboot-config-vars in kernel-fitimage
>> so that it has what it needs but doesn't break.
>>
>> Fixes: f9e834e31788 "meta: introduce KCONFIG_CONFIG_ENABLE_MENUCONFIG"
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>   meta/classes-recipe/kernel-fitimage.bbclass   |  2 +-
>>   meta/classes-recipe/uboot-config-vars.bbclass | 95 +++++++++++++++++++
>>   meta/classes-recipe/uboot-config.bbclass      | 80 +---------------
>>   3 files changed, 97 insertions(+), 80 deletions(-)
>>   create mode 100644 meta/classes-recipe/uboot-config-vars.bbclass
> 
> This looks like it might be a good candidate to move to a file in
> meta/conf rather than a new class file?

What does "this" refer to exactly?

Note that this patch just moves code, it doesn't introduce anything new.

> We could add any functions needed to lib/oe/ if they can be made
> suitably generic.

What "functions" would that be?

M.


-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[Richard Purdie]

On Wed, 2024-05-22 at 07:28 +0200, mike.looijmans@topic.nl wrote:
> On 21-05-2024 16:51, Richard Purdie wrote:
> > On Tue, 2024-05-21 at 15:57 +0200, Mike Looijmans via
> > lists.openembedded.org wrote:
> > > kernel-fitimage inherited uboot-config, which resulted in failure
> > > to
> > > run "menuconfig" on a system that has both multiple u-boot
> > > configs and
> > > wants a fitImage kernel.
> > > 
> > > Fix the issue by splitting up uboot-config into uboot-config-vars
> > > that
> > > only contains the "harmless" variable settings, and inherit that
> > > in a
> > > new uboot-config class which contains the Python code to do the
> > > U-boot
> > > specific settings. Inherit only uboot-config-vars in kernel-
> > > fitimage
> > > so that it has what it needs but doesn't break.
> > > 
> > > Fixes: f9e834e31788 "meta: introduce
> > > KCONFIG_CONFIG_ENABLE_MENUCONFIG"
> > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> > > ---
> > >   meta/classes-recipe/kernel-fitimage.bbclass   |  2 +-
> > >   meta/classes-recipe/uboot-config-vars.bbclass | 95
> > > +++++++++++++++++++
> > >   meta/classes-recipe/uboot-config.bbclass      | 80 +-----------
> > > ----
> > >   3 files changed, 97 insertions(+), 80 deletions(-)
> > >   create mode 100644 meta/classes-recipe/uboot-config-
> > > vars.bbclass
> > 
> > This looks like it might be a good candidate to move to a file in
> > meta/conf rather than a new class file?
> 
> What does "this" refer to exactly?

The variable definitions you're placing into a separate class file. I'm
saying they look more suited to a shared conf file, which is a
direction I think the project needs to move in generally.

> Note that this patch just moves code, it doesn't introduce anything
> new.
> 
> > We could add any functions needed to lib/oe/ if they can be made
> > suitably generic.
> 
> What "functions" would that be?

removesuffix is the one I saw that was needed in the new class file and
wouldn't work in a conf file.

Cheers,

Richard

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[mike.looijmans]

On 22-05-2024 07:39, Richard Purdie wrote:
> On Wed, 2024-05-22 at 07:28 +0200, mike.looijmans@topic.nl wrote:
>> On 21-05-2024 16:51, Richard Purdie wrote:
>>> On Tue, 2024-05-21 at 15:57 +0200, Mike Looijmans via
>>> lists.openembedded.org wrote:
>>>> kernel-fitimage inherited uboot-config, which resulted in failure
>>>> to
>>>> run "menuconfig" on a system that has both multiple u-boot
>>>> configs and
>>>> wants a fitImage kernel.
>>>>
>>>> Fix the issue by splitting up uboot-config into uboot-config-vars
>>>> that
>>>> only contains the "harmless" variable settings, and inherit that
>>>> in a
>>>> new uboot-config class which contains the Python code to do the
>>>> U-boot
>>>> specific settings. Inherit only uboot-config-vars in kernel-
>>>> fitimage
>>>> so that it has what it needs but doesn't break.
>>>>
>>>> Fixes: f9e834e31788 "meta: introduce
>>>> KCONFIG_CONFIG_ENABLE_MENUCONFIG"
>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>>> ---
>>>>    meta/classes-recipe/kernel-fitimage.bbclass   |  2 +-
>>>>    meta/classes-recipe/uboot-config-vars.bbclass | 95
>>>> +++++++++++++++++++
>>>>    meta/classes-recipe/uboot-config.bbclass      | 80 +-----------
>>>> ----
>>>>    3 files changed, 97 insertions(+), 80 deletions(-)
>>>>    create mode 100644 meta/classes-recipe/uboot-config-
>>>> vars.bbclass
>>>
>>> This looks like it might be a good candidate to move to a file in
>>> meta/conf rather than a new class file?
>>
>> What does "this" refer to exactly?
> 
> The variable definitions you're placing into a separate class file. I'm
> saying they look more suited to a shared conf file, which is a
> direction I think the project needs to move in generally.

a "uboot.conf" then, okay.

> 
>> Note that this patch just moves code, it doesn't introduce anything
>> new.
>>
>>> We could add any functions needed to lib/oe/ if they can be made
>>> suitably generic.
>>
>> What "functions" would that be?
> 
> removesuffix is the one I saw that was needed in the new class file and
> wouldn't work in a conf file.

Interestingly, "removesuffix" is a built-in Python function since 3.8:

 >>> 'hello.all'.removesuffix('.all')
'hello'

And 3.8 is the minimum required version for Yocto anyway.


-- 
Mike Looijmans

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[Quentin Schulz]

Hi Mike,

On 5/22/24 8:43 AM, Mike Looijmans via lists.openembedded.org wrote:
> On 22-05-2024 07:39, Richard Purdie wrote:
>> On Wed, 2024-05-22 at 07:28 +0200, mike.looijmans@topic.nl wrote:
>>> On 21-05-2024 16:51, Richard Purdie wrote:
>>>> On Tue, 2024-05-21 at 15:57 +0200, Mike Looijmans via
>>>> lists.openembedded.org wrote:
>>>>> kernel-fitimage inherited uboot-config, which resulted in failure
>>>>> to
>>>>> run "menuconfig" on a system that has both multiple u-boot
>>>>> configs and
>>>>> wants a fitImage kernel.
>>>>>
>>>>> Fix the issue by splitting up uboot-config into uboot-config-vars
>>>>> that
>>>>> only contains the "harmless" variable settings, and inherit that
>>>>> in a
>>>>> new uboot-config class which contains the Python code to do the
>>>>> U-boot
>>>>> specific settings. Inherit only uboot-config-vars in kernel-
>>>>> fitimage
>>>>> so that it has what it needs but doesn't break.
>>>>>
>>>>> Fixes: f9e834e31788 "meta: introduce
>>>>> KCONFIG_CONFIG_ENABLE_MENUCONFIG"
>>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>>>> ---
>>>>>    meta/classes-recipe/kernel-fitimage.bbclass   |  2 +-
>>>>>    meta/classes-recipe/uboot-config-vars.bbclass | 95
>>>>> +++++++++++++++++++
>>>>>    meta/classes-recipe/uboot-config.bbclass      | 80 +-----------
>>>>> ----
>>>>>    3 files changed, 97 insertions(+), 80 deletions(-)
>>>>>    create mode 100644 meta/classes-recipe/uboot-config-
>>>>> vars.bbclass
>>>>
>>>> This looks like it might be a good candidate to move to a file in
>>>> meta/conf rather than a new class file?
>>>
>>> What does "this" refer to exactly?
>>
>> The variable definitions you're placing into a separate class file. I'm
>> saying they look more suited to a shared conf file, which is a
>> direction I think the project needs to move in generally.
> 
> a "uboot.conf" then, okay.
> 

I was about to comment on the v2 that this should rather be a class :) 
Do we have some information on how/why this is the direction the project 
should move towards? What are the pitfalls we're trying to avoid, or 
benefits we want to gain? Or is this just some better/best practice we 
want to go for?

On a slightly different topic, I've been bothered by KERNEL_DEVICETREE 
being defined both in machine conf files and kernel recipes (in my own 
BSP layer, I'm not saying I'm doing things right :) ). I did this 
because I wanted to make a switch between kernel recipes as easy as 
possible while having some kind of common DTB in the machine conf file 
(e.g. upstream doesn't have all our Device Trees/Device Tree Overlays 
yet, sometimes the names slightly change between recipes...). If 
everything was in machine conf file, 
PREFERRED_PROVIDER_virtual/kernel/PREFERRED_VERSION_virtual/kernel would 
be only a part of the switch). Also, I use KERNEL_DEVICETREE in the 
machine conf file because I need to extract the default device tree to 
do some logic on it (namely, modify U-Boot at build time so those half 
broken ancient vendor U-Boot could boot my image). Not sure this is 
relevant, but had it in my mind so just putting it out there.

>>
>>> Note that this patch just moves code, it doesn't introduce anything
>>> new.
>>>
>>>> We could add any functions needed to lib/oe/ if they can be made
>>>> suitably generic.
>>>
>>> What "functions" would that be?
>>
>> removesuffix is the one I saw that was needed in the new class file and
>> wouldn't work in a conf file.
> 
> Interestingly, "removesuffix" is a built-in Python function since 3.8:
> 
>  >>> 'hello.all'.removesuffix('.all')
> 'hello'
> 
> And 3.8 is the minimum required version for Yocto anyway.
> 

If we need to backport this to earlier releases, 3.8 might not be the 
minimum supported version of Python anymore, making this harder to backport.

Cheers,
Quentin

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[mike.looijmans]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail
On 23-05-2024 15:43, Quentin Schulz wrote:
> Hi Mike,
> 
> On 5/22/24 8:43 AM, Mike Looijmans via lists.openembedded.org wrote:
>> On 22-05-2024 07:39, Richard Purdie wrote:
>>> On Wed, 2024-05-22 at 07:28 +0200, mike.looijmans@topic.nl wrote:
>>>> On 21-05-2024 16:51, Richard Purdie wrote:
>>>>> On Tue, 2024-05-21 at 15:57 +0200, Mike Looijmans via
>>>>> lists.openembedded.org wrote:
>>>>>> kernel-fitimage inherited uboot-config, which resulted in failure
>>>>>> to
>>>>>> run "menuconfig" on a system that has both multiple u-boot
>>>>>> configs and
>>>>>> wants a fitImage kernel.
>>>>>>
>>>>>> Fix the issue by splitting up uboot-config into uboot-config-vars
>>>>>> that
>>>>>> only contains the "harmless" variable settings, and inherit that
>>>>>> in a
>>>>>> new uboot-config class which contains the Python code to do the
>>>>>> U-boot
>>>>>> specific settings. Inherit only uboot-config-vars in kernel-
>>>>>> fitimage
>>>>>> so that it has what it needs but doesn't break.
>>>>>>
>>>>>> Fixes: f9e834e31788 "meta: introduce
>>>>>> KCONFIG_CONFIG_ENABLE_MENUCONFIG"
>>>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>>>>> ---
>>>>>>    meta/classes-recipe/kernel-fitimage.bbclass   |  2 +-
>>>>>>    meta/classes-recipe/uboot-config-vars.bbclass | 95
>>>>>> +++++++++++++++++++
>>>>>>    meta/classes-recipe/uboot-config.bbclass      | 80 +-----------
>>>>>> ----
>>>>>>    3 files changed, 97 insertions(+), 80 deletions(-)
>>>>>>    create mode 100644 meta/classes-recipe/uboot-config-
>>>>>> vars.bbclass
>>>>>
>>>>> This looks like it might be a good candidate to move to a file in
>>>>> meta/conf rather than a new class file?
>>>>
>>>> What does "this" refer to exactly?
>>>
>>> The variable definitions you're placing into a separate class file. I'm
>>> saying they look more suited to a shared conf file, which is a
>>> direction I think the project needs to move in generally.
>>
>> a "uboot.conf" then, okay.
>>
> 
> I was about to comment on the v2 that this should rather be a class :) Do we 
> have some information on how/why this is the direction the project should move 
> towards? What are the pitfalls we're trying to avoid, or benefits we want to 
> gain? Or is this just some better/best practice we want to go for?

I'm neutral in this - all I really care about is solving the underlying issue 
of not being able to run menuconfig for my board.

> 
> On a slightly different topic, I've been bothered by KERNEL_DEVICETREE being 
> defined both in machine conf files and kernel recipes (in my own BSP layer, 
> I'm not saying I'm doing things right :) ). I did this because I wanted to 
> make a switch between kernel recipes as easy as possible while having some 
> kind of common DTB in the machine conf file (e.g. upstream doesn't have all 
> our Device Trees/Device Tree Overlays yet, sometimes the names slightly change 
> between recipes...). If everything was in machine conf file, 
> PREFERRED_PROVIDER_virtual/kernel/PREFERRED_VERSION_virtual/kernel would be 
> only a part of the switch). Also, I use KERNEL_DEVICETREE in the machine conf 
> file because I need to extract the default device tree to do some logic on it 
> (namely, modify U-Boot at build time so those half broken ancient vendor 
> U-Boot could boot my image). Not sure this is relevant, but had it in my mind 
> so just putting it out there.

Rather unrelated yeah. To me "KERNEL_DEVICETREE" has always meant "please also 
compile this dts from the kernel sources" and nothing more than that.

The devicetree that ends up on my boards is often something else entirely, and 
often u-boot and kernel don't get the same devicetree anyway.


> 
>>>
>>>> Note that this patch just moves code, it doesn't introduce anything
>>>> new.
>>>>
>>>>> We could add any functions needed to lib/oe/ if they can be made
>>>>> suitably generic.
>>>>
>>>> What "functions" would that be?
>>>
>>> removesuffix is the one I saw that was needed in the new class file and
>>> wouldn't work in a conf file.
>>
>> Interestingly, "removesuffix" is a built-in Python function since 3.8:
>>
>>  >>> 'hello.all'.removesuffix('.all')
>> 'hello'
>>
>> And 3.8 is the minimum required version for Yocto anyway.
>>
> 
> If we need to backport this to earlier releases, 3.8 might not be the minimum 
> supported version of Python anymore, making this harder to backport.

It won't be backported far as the commit that broke it is also pretty recent, 
it think it's in nanbield.

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[Richard Purdie]

On Thu, 2024-05-23 at 15:43 +0200, Quentin Schulz wrote:
> I was about to comment on the v2 that this should rather be a class :) 
> Do we have some information on how/why this is the direction the project 
> should move towards? What are the pitfalls we're trying to avoid, or 
> benefits we want to gain? Or is this just some better/best practice we 
> want to go for?

There was a post from me a while back complaining about repeatedly
splitting class files into smaller less manageable ones so that people
could access some variables without the task/function side effects.

I have a belief that more configuration files would help this kind of
problem and make classes more specific/focused and usable.

Cheers,

Richard

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[mike.looijmans]

On 24-05-2024 12:22, Richard Purdie wrote:
> On Thu, 2024-05-23 at 15:43 +0200, Quentin Schulz wrote:
>> I was about to comment on the v2 that this should rather be a class :)
>> Do we have some information on how/why this is the direction the project
>> should move towards? What are the pitfalls we're trying to avoid, or
>> benefits we want to gain? Or is this just some better/best practice we
>> want to go for?
> There was a post from me a while back complaining about repeatedly
> splitting class files into smaller less manageable ones so that people
> could access some variables without the task/function side effects.
>
> I have a belief that more configuration files would help this kind of
> problem and make classes more specific/focused and usable.
>
Okay, so what is now the way forward? v1 or v2?


-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Re: [OE-core] [PATCH] meta: uboot-config: Fix broken menuconfig for "fitImage" kernel

[Quentin Schulz]

Hi Mike,

On 5/28/24 7:20 AM, mike.looijmans@topic.nl wrote:
> [You don't often get email from mike.looijmans@topic.nl. Learn why this 
> is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 24-05-2024 12:22, Richard Purdie wrote:
>> On Thu, 2024-05-23 at 15:43 +0200, Quentin Schulz wrote:
>>> I was about to comment on the v2 that this should rather be a class :)
>>> Do we have some information on how/why this is the direction the project
>>> should move towards? What are the pitfalls we're trying to avoid, or
>>> benefits we want to gain? Or is this just some better/best practice we
>>> want to go for?
>> There was a post from me a while back complaining about repeatedly
>> splitting class files into smaller less manageable ones so that people
>> could access some variables without the task/function side effects.
>>
>> I have a belief that more configuration files would help this kind of
>> problem and make classes more specific/focused and usable.
>>
> Okay, so what is now the way forward? v1 or v2?
> 

Go for what Richard asked, he's the maintainer after all :) I was merely 
trying to understand the position of the project, not suggesting we do 
something else.

I think you've already sent a v2, so just wait for reviews or for it to 
be merged. If it isn't merged in the next few weeks, then send a 
reminder as an answer to your v2 asking for feedback.

Cheers,
Quentin