Re: [OE-core] [PATCH] meta: simplify conditional operations with bb.utils.filter

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

Hi João,

On 4/27/26 10:38 AM, Joao Marcos Costa wrote:
> Hello, Quentin
> 
> On 4/24/26 17:40, Quentin Schulz via lists.openembedded.org wrote:
>> Hi João,
>>
>> On 4/24/26 5:17 PM, Joao Marcos Costa via lists.openembedded.org wrote:
>>> Some recipes and configuration files use bb.utils.contains to check for
>>> a string inside a variable, and return the exact same string if true.
>>>
>>> This can be simplified by a call to bb.utils.filter, since the result is
>>> the same, and the inline is shorter.
>>>
>>> Replace "bb.utils.contains(A, 'a', 'a', '', d)" by 
>>> "bb.utils.filter(A, 'a', d)".
>>>
>>> Signed-off-by: João Marcos Costa <joaomarcos.costa@bootlin.com>
>>> ---
> 
> (...)
> 
>> I'm pretty sure your changes in the conf files won't work, but maybe 
>> I'm wrong?
>>
>> Before, we only added a space if there was a new tune to add. Now, 
>> it's always added.
>>
>> Considering TUNE_CCARGS_MFPU can now possibly contain only spaces, the 
>> check if TUNE_CCARGS_MFPU is not the empty string to access the last 
>> element in the list returned by split(), which will now be the empty 
>> list, will raise an IndexError exception.
>>
>> You need to adapt anything that's using this variable to check if it 
>> only contains spaces or at least remove spurious ones, possibly with 
>> str.strip() for example. Unfortunately, if TUNE_CCARGS_MFPU doesn't 
>> exist, d.getVar() will return None and you cannot strip() on None. Of 
>> course, considering TUNE_CCARGS_MFPU is declared just above in the 
>> file, it'll always be defined, so maybe that's fine to not check for 
>> None.
>>
>> If the space didn't matter, we probably would have used += instead 
>> of .=, so I'm wary of this change (the rest looks fine).
>>
>> Cheers,
>> Quentin
> 
> Thanks for your review. And yes, I'm looking at this part of the patch 
> again, and I'm considering two things:
> 
> a) Add the strip() as you said, here, and keep my changes:
> 
> -TUNE_CCARGS  .= "${@ (' -mfpu=%s' % 
> d.getVar('TUNE_CCARGS_MFPU').split()[-1]) if 
> (d.getVar('TUNE_CCARGS_MFPU') != '') else ''}"
> +TUNE_CCARGS  .= "${@ (' -mfpu=%s' % 
> d.getVar('TUNE_CCARGS_MFPU').split()[-1]) if 
> (d.getVar('TUNE_CCARGS_MFPU').strip() != '') else ''}"
>   # The following deals with both vfpv3-d16 and vfpv4-d16
> -ARMPKGSFX_FPU = "${@ ('-%s'       % 
> d.getVar('TUNE_CCARGS_MFPU').split()[-1].replace('-d16', 'd16')) if 
> (d.getVar('TUNE_CCARGS_MFPU') != '') else ''}"
> +ARMPKGSFX_FPU = "${@ ('-%s'       % 
> d.getVar('TUNE_CCARGS_MFPU').split()[-1].replace('-d16', 'd16')) if 
> (d.getVar('TUNE_CCARGS_MFPU').strip() != '') else ''}"
> 

Yet another option is to do

(d.getVar('TUNE_CCARGS_MFPU') or '').strip() != ''

but it's a bit eww :)

> b) Simply remove those changes, then send another patch keeping only the 
> changes in recipes.
> 

Yeah that works too. It's the same change but the implications are 
different. So maybe three patches (one for machihes, one for QA, one for 
recipes (maybe even in two, one for PACKAGECONFIG, one for RDEPENDS 
since Chen Qi doesn't seem to like the latter too much :) ) so everybody 
can bikeshed as much as they want on one and still have the rest merged :)

Cheers,
Quentin