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

[Quentin Schulz <quentin.schulz@cherry.de>]

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