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

[<mike.looijmans@topic.nl>]

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.