| Message ID | 20250212-varflags-v1-1-3a756c7aa95c@syslinbit.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitbake: fix ??= operator for getVarFlags and test | expand |
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
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
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
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: