[PATCH 0/2] bitbake: fix ??= operator for getVarFlags and test

[PATCH 0/2] bitbake: fix ??= operator for getVarFlags and test

[Louis Rannou]

Since commit 0329a7e3ac694737f2d2c1861f65492551360663, weak default values are
omitted while they were only incorrect before. The first patch fixes the issue,
the second adds a test for validation.

Fixes [YOCTO #15685]

Signed-off-by: Louis Rannou <louis.rannou@syslinbit.com>
---
Louis Rannou (2):
      bitbake: data_smart: fix ??= operator for getVarFlags
      bitbake: tests/parse: add test for getVarFlags

 lib/bb/data_smart.py  | 16 +++++++++++++---
 lib/bb/tests/parse.py | 16 ++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)
---
base-commit: bcf090ed631bbd523a5341baebba0765f1a847f8
change-id: 20250212-varflags-7e56f73a3966

Best regards,
-- 
Louis Rannou <louis.rannou@syslinbit.com>

[PATCH 1/2] bitbake: data_smart: fix ??= operator for getVarFlags

[Louis Rannou]

From: Louis Rannou <louis.rannou@non.se.com>

Variable flags have been fixed in commit
0329a7e3ac694737f2d2c1861f65492551360663 which introduces the
"_defaultval_flag_" prefix for default values. This must not be ignored as
others "_"-prefixed flags names.

Split the processing of default values to be sure they are overwritted by the
others operators disregarding of their order.

Fixes [YOCTO #15685]

Signed-off-by: Louis Rannou <louis.rannou@non.se.com>
---
 lib/bb/data_smart.py | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
index 897ceeb32c7ce0acb8ed44c25e1bf2f2f28aa9dc..7663f28c5071a77c8452c1016e5ee0d969c84bec 100644
--- a/lib/bb/data_smart.py
+++ b/lib/bb/data_smart.py
@@ -952,12 +952,22 @@ class DataSmart(MutableMapping):
     def getVarFlags(self, var, expand = False, internalflags=False):
         local_var = self._findVar(var)
         flags = {}
+        flags_set = {}
 
         if local_var:
-            for i in local_var:
-                if i.startswith(("_", ":")) and not internalflags:
+            for i, val in local_var.items():
+                if i.startswith("_defaultval_flag_"):
+                    i = i[len("_defaultval_flag_"):]
+                    flags[i] = val
+                elif i.startswith(("_", ":")) and not internalflags:
                     continue
-                flags[i] = local_var[i]
+                else:
+                    flags_set[i] = val
+
+            # Flags sets take over defaults
+            flags.update(flags_set)
+
+            for i in flags:
                 if expand and i in expand:
                     flags[i] = self.expand(flags[i], var + "[" + i + "]")
         if len(flags) == 0:

-- 
2.48.1

Re: [bitbake-devel] [PATCH 1/2] bitbake: data_smart: fix ??= operator for getVarFlags

[Richard Purdie]

On Wed, 2025-02-12 at 13:16 +0100, Louis Rannou via lists.openembedded.org wrote:
> From: Louis Rannou <louis.rannou@non.se.com>
> 
> Variable flags have been fixed in commit
> 0329a7e3ac694737f2d2c1861f65492551360663 which introduces the
> "_defaultval_flag_" prefix for default values. This must not be ignored as
> others "_"-prefixed flags names.
> 
> Split the processing of default values to be sure they are overwritted by the
> others operators disregarding of their order.
> 
> Fixes [YOCTO #15685]
> 
> Signed-off-by: Louis Rannou <louis.rannou@non.se.com>
> ---
>  lib/bb/data_smart.py | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
> index 897ceeb32c7ce0acb8ed44c25e1bf2f2f28aa9dc..7663f28c5071a77c8452c1016e5ee0d969c84bec 100644
> --- a/lib/bb/data_smart.py
> +++ b/lib/bb/data_smart.py
> @@ -952,12 +952,22 @@ class DataSmart(MutableMapping):
>      def getVarFlags(self, var, expand = False, internalflags=False):
>          local_var = self._findVar(var)
>          flags = {}
> +        flags_set = {}
>  
>          if local_var:
> -            for i in local_var:
> -                if i.startswith(("_", ":")) and not internalflags:
> +            for i, val in local_var.items():
> +                if i.startswith("_defaultval_flag_"):
> +                    i = i[len("_defaultval_flag_"):]
> +                    flags[i] = val
> +                elif i.startswith(("_", ":")) and not internalflags:
>                      continue
> -                flags[i] = local_var[i]
> +                else:
> +                    flags_set[i] = val
> +
> +            # Flags sets take over defaults
> +            flags.update(flags_set)
> +
> +            for i in flags:
>                  if expand and i in expand:
>                      flags[i] = self.expand(flags[i], var + "[" + i + "]")
>          if len(flags) == 0:
> 

I've not gone into the details but this doesn't look right at all
unfortunately. It looks over complicated, it is special casing magic
values which we try very very hard not to do and I don't think this is
going to be maintainable code. We're need to find a better solution.
Without diving into the code I'm going to struggle to provide better
advice though.

Cheers,

Richard

Re: [bitbake-devel] [PATCH 1/2] bitbake: data_smart: fix ??= operator for getVarFlags

[Richard Purdie]

On Wed, 2025-02-12 at 13:16 +0100, Louis Rannou via lists.openembedded.org wrote:
> From: Louis Rannou <louis.rannou@non.se.com>
> 
> Variable flags have been fixed in commit
> 0329a7e3ac694737f2d2c1861f65492551360663 which introduces the
> "_defaultval_flag_" prefix for default values. This must not be ignored as
> others "_"-prefixed flags names.
> 
> Split the processing of default values to be sure they are overwritted by the
> others operators disregarding of their order.
> 
> Fixes [YOCTO #15685]
> 
> Signed-off-by: Louis Rannou <louis.rannou@non.se.com>
> ---
>  lib/bb/data_smart.py | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
> index 897ceeb32c7ce0acb8ed44c25e1bf2f2f28aa9dc..7663f28c5071a77c8452c1016e5ee0d969c84bec 100644
> --- a/lib/bb/data_smart.py
> +++ b/lib/bb/data_smart.py
> @@ -952,12 +952,22 @@ class DataSmart(MutableMapping):
>      def getVarFlags(self, var, expand = False, internalflags=False):
>          local_var = self._findVar(var)
>          flags = {}
> +        flags_set = {}
>  
>          if local_var:
> -            for i in local_var:
> -                if i.startswith(("_", ":")) and not internalflags:
> +            for i, val in local_var.items():
> +                if i.startswith("_defaultval_flag_"):
> +                    i = i[len("_defaultval_flag_"):]
> +                    flags[i] = val
> +                elif i.startswith(("_", ":")) and not internalflags:
>                      continue
> -                flags[i] = local_var[i]
> +                else:
> +                    flags_set[i] = val
> +
> +            # Flags sets take over defaults
> +            flags.update(flags_set)
> +
> +            for i in flags:
>                  if expand and i in expand:
>                      flags[i] = self.expand(flags[i], var + "[" + i + "]")
>          if len(flags) == 0:

I did spend more time looking at this and I had misunderstood the
problem. I think this can still be simplified though and I also suspect
the internalflags handling above isn't correct. If internalflags is
set, I think it probably should return _defaultval_flag_ values
unchanged. The code would therefore look more like:


for i, val in local_var.items():
    if i.startswith("_defaultval_flag_") and not internalflags:
        i = i[len("_defaultval_flag_"):]
        if i not in local_var:
            flags[i] = val
    elif i.startswith(("_", ":")) and not internalflags:
        continue
    else:
        flags_set[i] = val

I'd also add a comment to the test case explaining that we're testing
weak default make it to the getVarFlags output.

Cheers,

Richard

Re: [bitbake-devel] [PATCH 1/2] bitbake: data_smart: fix ??= operator for getVarFlags

[Louis Rannou]

Hello,

Ok for the solution which is indeed prettier. I'll add a test with 
internalflags.

I also agree the test is not exactly at the right place as it does not 
test parsing but the result of getVarFlags. Perhaps it would fit better 
in tests/data.py ? (if that's ok to make a call to bb.parse there)

Thanks,
Louis

On 12/02/2025 15:13, Richard Purdie wrote:
> On Wed, 2025-02-12 at 13:16 +0100, Louis Rannou via lists.openembedded.org wrote:
>> From: Louis Rannou <louis.rannou@non.se.com>
>>
>> Variable flags have been fixed in commit
>> 0329a7e3ac694737f2d2c1861f65492551360663 which introduces the
>> "_defaultval_flag_" prefix for default values. This must not be ignored as
>> others "_"-prefixed flags names.
>>
>> Split the processing of default values to be sure they are overwritted by the
>> others operators disregarding of their order.
>>
>> Fixes [YOCTO #15685]
>>
>> Signed-off-by: Louis Rannou <louis.rannou@non.se.com>
>> ---
>>   lib/bb/data_smart.py | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
>> index 897ceeb32c7ce0acb8ed44c25e1bf2f2f28aa9dc..7663f28c5071a77c8452c1016e5ee0d969c84bec 100644
>> --- a/lib/bb/data_smart.py
>> +++ b/lib/bb/data_smart.py
>> @@ -952,12 +952,22 @@ class DataSmart(MutableMapping):
>>       def getVarFlags(self, var, expand = False, internalflags=False):
>>           local_var = self._findVar(var)
>>           flags = {}
>> +        flags_set = {}
>>   
>>           if local_var:
>> -            for i in local_var:
>> -                if i.startswith(("_", ":")) and not internalflags:
>> +            for i, val in local_var.items():
>> +                if i.startswith("_defaultval_flag_"):
>> +                    i = i[len("_defaultval_flag_"):]
>> +                    flags[i] = val
>> +                elif i.startswith(("_", ":")) and not internalflags:
>>                       continue
>> -                flags[i] = local_var[i]
>> +                else:
>> +                    flags_set[i] = val
>> +
>> +            # Flags sets take over defaults
>> +            flags.update(flags_set)
>> +
>> +            for i in flags:
>>                   if expand and i in expand:
>>                       flags[i] = self.expand(flags[i], var + "[" + i + "]")
>>           if len(flags) == 0:
> 
> I did spend more time looking at this and I had misunderstood the
> problem. I think this can still be simplified though and I also suspect
> the internalflags handling above isn't correct. If internalflags is
> set, I think it probably should return _defaultval_flag_ values
> unchanged. The code would therefore look more like:
> 
> 
> for i, val in local_var.items():
>      if i.startswith("_defaultval_flag_") and not internalflags:
>          i = i[len("_defaultval_flag_"):]
>          if i not in local_var:
>              flags[i] = val
>      elif i.startswith(("_", ":")) and not internalflags:
>          continue
>      else:
>          flags_set[i] = val
> 
> I'd also add a comment to the test case explaining that we're testing
> weak default make it to the getVarFlags output.
> 
> Cheers,
> 
> Richard

[PATCH 2/2] bitbake: tests/parse: add test for getVarFlags

[Louis Rannou]

From: Louis Rannou <louis.rannou@non.se.com>

Run the test with:
    $ bitbake-selftest bb.tests.parse.ParseTest.test_parse_defaulttest
    test_parse_defaulttest (bb.tests.parse.ParseTest.test_parse_defaulttest) ... ok

    ----------------------------------------------------------------------
    Ran 1 test in 0.001s

    OK

This is a test case for [YOCTO #15685]

Signed-off-by: Louis Rannou <louis.rannou@non.se.com>
---
 lib/bb/tests/parse.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/bb/tests/parse.py b/lib/bb/tests/parse.py
index e3cba67ad448a491ee9c094e6725eb68eba14b32..74a6c54ad1908357b65c37c37c8b8c299fd5f3bc 100644
--- a/lib/bb/tests/parse.py
+++ b/lib/bb/tests/parse.py
@@ -127,6 +127,22 @@ A[flag_default_twice] ??= "default flag second"
         self.assertEqual(d.getVarFlag("A","flag_set_twice"), "set flag second")
         self.assertEqual(d.getVarFlag("A","flag_question_twice"), "question flag first")
         self.assertEqual(d.getVarFlag("A","flag_default_twice"), "default flag second")
+        self.assertDictEqual(
+            d.getVarFlags("A"),
+            {
+                "flag_set_vs_question": "set flag",
+                "flag_set_vs_default": "set flag",
+                "flag_question": "question flag",
+                "flag_default": "default flag",
+                "flag_question_vs_default": "question flag",
+                "flag_default_vs_question": "question flag",
+                "flag_set_question_default": "set flag",
+                "flag_set_default_question": "set flag",
+                "flag_set_twice": "set flag second",
+                "flag_question_twice": "question flag first",
+                "flag_default_twice": "default flag second",
+            }
+        )
 
     exporttest = """
 A = "a"

-- 
2.48.1