| Message ID | 20260725143002.23596-2-zizuzacker@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | data_smart: fix key expansion in conditional overrides | expand |
On Sat, 2026-07-25 at 21:30 +0700, Zk47T via lists.openembedded.org wrote: > An operation whose override name needs key expansion is silently dropped: > > RDEPENDS:${PN}:append:pn-foo-${MACHINE} = " bar" > > renameVar() rebuilds the dependent override keys with a plain string > replace, so renaming RDEPENDS:${PN} leaves the append attached to > RDEPENDS:foo:append:pn-foo-${MACHINE}, which never matches an active > override. expandKeys() cannot fix that up afterwards either, as it works > from a list of keys collected before any renaming happened. > > Expand the derived name before renaming it. Only do so once newkey is > itself expanded, otherwise expandKeys() has still to rename newkey and > handles the dependent keys along with it. > > Add regression tests for both cases. > > Fixes [YOCTO #14867] > > Signed-off-by: Nguyen Minh Tien <zizuzacker@gmail.com> > --- > lib/bb/data_smart.py | 11 +++++++++-- > lib/bb/tests/data.py | 25 +++++++++++++++++++++++++ > 2 files changed, 34 insertions(+), 2 deletions(-) > > diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py > index 9961269a3..5738aff95 100644 > --- a/lib/bb/data_smart.py > +++ b/lib/bb/data_smart.py > @@ -697,8 +697,15 @@ class DataSmart(MutableMapping): > found = True > self.overridedata[newkey] = [] > for (v, o) in self.overridedata[key]: > - self.overridedata[newkey].append([v.replace(key, newkey), o]) > - self.renameVar(v, v.replace(key, newkey)) > + newv = v.replace(key, newkey) > + # The derived name may still hold a variable reference which > + # expandKeys() will never revisit, so expand it here. Only once > + # newkey is expanded though, otherwise expandKeys() has still to > + # rename newkey and handles the dependent keys along with it. > + if '${' in newv and '${' not in newkey: > + newv = self.expand(newv) > + self.overridedata[newkey].append([newv, o]) > + self.renameVar(v, newv) > Thanks for the patch and the test cases. I had a deeper look at this as I was a bit puzzled how this would work without your patch: XYZ = "yocto" ABC = "123" ABC:append:pn-linux-${XYZ} = " 456" $ bitbake-getvar -r linux-yocto ABC ABC="123 456" as if the code can handle that, it should be able to handle the other case too. I think this is because expandKeys() calls renameVar and then renameVar itself also calls renameVar() on the same element, which breaks things. That means that if you do: diff --git a/lib/bb/data.py b/lib/bb/data.py index 5fdcdb04a..e3af12a35 100644 --- a/lib/bb/data.py +++ b/lib/bb/data.py @@ -94,7 +94,7 @@ def expandKeys(alterdata, readdata = None): val = alterdata.getVar(key, False) if val is not None: bb.warn("Variable key %s (%s) replaces original key %s (%s)." % (key, val, ekey, newval)) - alterdata.renameVar(key, ekey) + alterdata.renameVar(key, ekey, recurse=False) def inheritFromOS(d, savedenv, permitted): """Inherit variables from the initial environment.""" diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py index 110dfa111..78ff9b961 100644 --- a/lib/bb/data_smart.py +++ b/lib/bb/data_smart.py @@ -661,7 +661,7 @@ class DataSmart(MutableMapping): def getVar(self, var, expand=True, noweakdefault=False, parsing=False): return self.getVarFlag(var, "_content", expand, noweakdefault, parsing) - def renameVar(self, key, newkey, **loginfo): + def renameVar(self, key, newkey, recurse=True, **loginfo): """ Rename the variable key to newkey """ @@ -693,7 +693,8 @@ class DataSmart(MutableMapping): self.overridedata[newkey] = [] for (v, o) in self.overridedata[key]: self.overridedata[newkey].append([v.replace(key, newkey), o]) - self.renameVar(v, v.replace(key, newkey)) + if recurse: + self.renameVar(v, v.replace(key, newkey)) if ':' in newkey and val is None: self._setvar_update_overrides(newkey, **loginfo) then I think that resolve the issue? It would also perhaps resolve an issue where a variable with three different key expansions in it might not work! I'm less sure that three different key variables would work with plain renameVar, that may also need to set the "no recurse" option, I'm not sure. Could you see if this makes sense to you? Cheers, Richard
> I think this is because expandKeys() calls renameVar and then > renameVar itself also calls renameVar() on the same element, which > breaks things. [...] then I think that resolve the issue? Yes, it does, and it is the better fix. Both keys are already in the todolist with their expanded names, so expandKeys() can rename them both. My patch only worked around the second rename with expand(). I tested your diff on a fresh clone at c251833d2. It fixes the exact form from the bug report, :prepend and :remove, a conditional override with no operation keyword, several variables in one override name, and a variable in the key as well as in the override name, all of which fail on master. bb.tests.data passes, oe-core parses clean at 952 recipes with no new warnings, and both v1 test cases pass on your diff unchanged. > I'm less sure that three different key variables would work with plain > renameVar, that may also need to set the "no recurse" option. It does not need it, and it must not have it. In the native.bbclass shape a key is renamed to another unexpanded key before expandKeys() runs. recurse=True gives the right answer with no warnings. I tried recurse=False there and the override value is lost: the dependent key is left behind, and expandKeys() then expands it from the old parent name, so it no longer matches. Two things before I send a v2. First, the new argument breaks recipe_sanity.bbclass, which replaces DataSmart.renameVar with a wrapper taking a fixed set of arguments. Any build using INHERIT += "recipe_sanity" now dies before parsing starts: TypeError: myrename() got an unexpected keyword argument 'recurse' A one line oe-core patch passing *args/**kwargs through fixes it, it just has to go in at the same time. Second, and this is why I have not sent a v2 yet. There is an ordering bug here and it is not yours. Master already has it, in the case where nothing gets dropped: ABC = "123" ABC:append:pn-linux-${XYZ} = " 456" ABC:append:cfg-${SFX} = " 789" master gives "123 789 456" should be "123 456 789" Both appends are applied, just in the wrong order, and your diff does not change that case at all. It matters because of the case your diff does fix. Master drops those operations, so nobody ever sees the order being wrong. Once they stop being dropped, it shows: PN = "packagegroup-cross-canadian-qemux86" RDEPENDS:${PN} = "base" RDEPENDS:${PN}:append:pn-packagegroup-cross-canadian-${MACHINE} = " a" RDEPENDS:${PN}:append:libc-${TCLIBC} = " b" RDEPENDS:${PN}:append:class-target = " c" master "base c" a and b dropped your diff "base c b a" my v1 "base c a b" should be "base a b c" pn-${PN}, libc-glibc and class-target are all in OVERRIDES at once, so all three appends are meant to apply. The reason seems to be __setvar_regexp__, which ends in (:(?P<add>[^A-Z]*))?$. That came from 6eb56624e, where you stopped capitalised overrides being processed. ${MACHINE} has uppercase in it, so an override name holding it fails that test and is stored as a variable of its own, while a plain override name passes and becomes an entry in the :append flag on the base variable. The operations for one variable then live in two places, and the flag group is applied first regardless of what was written first. With your diff all six orderings of a, b and c give "base c b a". Mine keeps the order within the second group but still puts c first, so it is wrong as well. That line leaves a second gap neither patch closes. A lowercase variable name passes the regexp, so an append written with ${machine} rather than ${MACHINE} becomes a flag whose override name is still pn-gizmo-${machine}, and that name is never expanded when overrides are matched. Master, your diff and mine all return "base" for it. Nobody writes ${machine} in practice, but it is another way to lose the same operation. Would you like me to fix the drop now and report the ordering as its own bug, or look at these together? I did not want to send a v2 that changes ordering without asking first. Best regards, Tien Vào CN, 26 thg 7, 2026 vào lúc 19:21 Richard Purdie <richard.purdie@linuxfoundation.org> đã viết: > > On Sat, 2026-07-25 at 21:30 +0700, Zk47T via lists.openembedded.org wrote: > > An operation whose override name needs key expansion is silently dropped: > > > > RDEPENDS:${PN}:append:pn-foo-${MACHINE} = " bar" > > > > renameVar() rebuilds the dependent override keys with a plain string > > replace, so renaming RDEPENDS:${PN} leaves the append attached to > > RDEPENDS:foo:append:pn-foo-${MACHINE}, which never matches an active > > override. expandKeys() cannot fix that up afterwards either, as it works > > from a list of keys collected before any renaming happened. > > > > Expand the derived name before renaming it. Only do so once newkey is > > itself expanded, otherwise expandKeys() has still to rename newkey and > > handles the dependent keys along with it. > > > > Add regression tests for both cases. > > > > Fixes [YOCTO #14867] > > > > Signed-off-by: Nguyen Minh Tien <zizuzacker@gmail.com> > > --- > > lib/bb/data_smart.py | 11 +++++++++-- > > lib/bb/tests/data.py | 25 +++++++++++++++++++++++++ > > 2 files changed, 34 insertions(+), 2 deletions(-) > > > > diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py > > index 9961269a3..5738aff95 100644 > > --- a/lib/bb/data_smart.py > > +++ b/lib/bb/data_smart.py > > @@ -697,8 +697,15 @@ class DataSmart(MutableMapping): > > found = True > > self.overridedata[newkey] = [] > > for (v, o) in self.overridedata[key]: > > - self.overridedata[newkey].append([v.replace(key, newkey), o]) > > - self.renameVar(v, v.replace(key, newkey)) > > + newv = v.replace(key, newkey) > > + # The derived name may still hold a variable reference which > > + # expandKeys() will never revisit, so expand it here. Only once > > + # newkey is expanded though, otherwise expandKeys() has still to > > + # rename newkey and handles the dependent keys along with it. > > + if '${' in newv and '${' not in newkey: > > + newv = self.expand(newv) > > + self.overridedata[newkey].append([newv, o]) > > + self.renameVar(v, newv) > > > > Thanks for the patch and the test cases. I had a deeper look at this as > I was a bit puzzled how this would work without your patch: > > XYZ = "yocto" > ABC = "123" > ABC:append:pn-linux-${XYZ} = " 456" > $ bitbake-getvar -r linux-yocto ABC > ABC="123 456" > > as if the code can handle that, it should be able to handle the other > case too. I think this is because expandKeys() calls renameVar and then > renameVar itself also calls renameVar() on the same element, which > breaks things. That means that if you do: > > diff --git a/lib/bb/data.py b/lib/bb/data.py > index 5fdcdb04a..e3af12a35 100644 > --- a/lib/bb/data.py > +++ b/lib/bb/data.py > @@ -94,7 +94,7 @@ def expandKeys(alterdata, readdata = None): > val = alterdata.getVar(key, False) > if val is not None: > bb.warn("Variable key %s (%s) replaces original key %s (%s)." % (key, val, ekey, newval)) > - alterdata.renameVar(key, ekey) > + alterdata.renameVar(key, ekey, recurse=False) > > def inheritFromOS(d, savedenv, permitted): > """Inherit variables from the initial environment.""" > diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py > index 110dfa111..78ff9b961 100644 > --- a/lib/bb/data_smart.py > +++ b/lib/bb/data_smart.py > @@ -661,7 +661,7 @@ class DataSmart(MutableMapping): > def getVar(self, var, expand=True, noweakdefault=False, parsing=False): > return self.getVarFlag(var, "_content", expand, noweakdefault, parsing) > > - def renameVar(self, key, newkey, **loginfo): > + def renameVar(self, key, newkey, recurse=True, **loginfo): > """ > Rename the variable key to newkey > """ > @@ -693,7 +693,8 @@ class DataSmart(MutableMapping): > self.overridedata[newkey] = [] > for (v, o) in self.overridedata[key]: > self.overridedata[newkey].append([v.replace(key, newkey), o]) > - self.renameVar(v, v.replace(key, newkey)) > + if recurse: > + self.renameVar(v, v.replace(key, newkey)) > > if ':' in newkey and val is None: > self._setvar_update_overrides(newkey, **loginfo) > > > > then I think that resolve the issue? It would also perhaps resolve an > issue where a variable with three different key expansions in it might > not work! I'm less sure that three different key variables would work > with plain renameVar, that may also need to set the "no recurse" > option, I'm not sure. > > Could you see if this makes sense to you? > > Cheers, > > Richard > > > >
On Sun, 2026-07-26 at 21:53 +0700, Minh Tiến Nguyễn wrote: > > I think this is because expandKeys() calls renameVar and then > > renameVar itself also calls renameVar() on the same element, which > > breaks things. [...] then I think that resolve the issue? > > Yes, it does, and it is the better fix. Both keys are already in the > todolist with their expanded names, so expandKeys() can rename them > both. My patch only worked around the second rename with expand(). > > I tested your diff on a fresh clone at c251833d2. It fixes the exact > form from the bug report, :prepend and :remove, a conditional override > with no operation keyword, several variables in one override name, and a > variable in the key as well as in the override name, all of which fail > on master. bb.tests.data passes, oe-core parses clean at 952 recipes > with no new warnings, and both v1 test cases pass on your diff unchanged. I have to wonder whether my reply is just being fed to AI? :/ The project's policy is that AI use isn't prohibited but it's use should be made clear... Cheers, Richard
Hi Tien, On Sun, 2026-07-26 at 21:53 +0700, Minh Tiến Nguyễn wrote: > > I think this is because expandKeys() calls renameVar and then > > renameVar itself also calls renameVar() on the same element, which > > breaks things. [...] then I think that resolve the issue? > > Yes, it does, and it is the better fix. Both keys are already in the > todolist with their expanded names, so expandKeys() can rename them > both. My patch only worked around the second rename with expand(). > > I tested your diff on a fresh clone at c251833d2. It fixes the exact > form from the bug report, :prepend and :remove, a conditional override > with no operation keyword, several variables in one override name, and a > variable in the key as well as in the override name, all of which fail > on master. bb.tests.data passes, oe-core parses clean at 952 recipes > with no new warnings, and both v1 test cases pass on your diff unchanged. > > > I'm less sure that three different key variables would work with plain > > renameVar, that may also need to set the "no recurse" option. > > It does not need it, and it must not have it. In the native.bbclass > shape a key is renamed to another unexpanded key before expandKeys() > runs. recurse=True gives the right answer with no warnings. I tried > recurse=False there and the override value is lost: the dependent key is > left behind, and expandKeys() then expands it from the old parent name, > so it no longer matches. > > Two things before I send a v2. > > First, the new argument breaks recipe_sanity.bbclass, which replaces > DataSmart.renameVar with a wrapper taking a fixed set of arguments. Any > build using INHERIT += "recipe_sanity" now dies before parsing starts: > > TypeError: myrename() got an unexpected keyword argument 'recurse' > > A one line oe-core patch passing *args/**kwargs through fixes it, it > just has to go in at the same time. > > Second, and this is why I have not sent a v2 yet. There is an ordering > bug here and it is not yours. Master already has it, in the case where > nothing gets dropped: > > ABC = "123" > ABC:append:pn-linux-${XYZ} = " 456" > ABC:append:cfg-${SFX} = " 789" > > master gives "123 789 456" > should be "123 456 789" > > Both appends are applied, just in the wrong order, and your diff does > not change that case at all. > > It matters because of the case your diff does fix. Master drops those > operations, so nobody ever sees the order being wrong. Once they stop > being dropped, it shows: > > PN = "packagegroup-cross-canadian-qemux86" > > RDEPENDS:${PN} = "base" > RDEPENDS:${PN}:append:pn-packagegroup-cross-canadian-${MACHINE} = " a" > RDEPENDS:${PN}:append:libc-${TCLIBC} = " b" > RDEPENDS:${PN}:append:class-target = " c" > > master "base c" a and b dropped > your diff "base c b a" > my v1 "base c a b" > should be "base a b c" > > pn-${PN}, libc-glibc and class-target are all in OVERRIDES at once, so > all three appends are meant to apply. > > The reason seems to be __setvar_regexp__, which ends in > (:(?P<add>[^A-Z]*))?$. That came from 6eb56624e, where you stopped > capitalised overrides being processed. ${MACHINE} has uppercase in it, > so an override name holding it fails that test and is stored as a > variable of its own, while a plain override name passes and becomes an > entry in the :append flag on the base variable. The operations for one > variable then live in two places, and the flag group is applied first > regardless of what was written first. With your diff all six orderings > of a, b and c give "base c b a". Mine keeps the order within the second > group but still puts c first, so it is wrong as well. > > That line leaves a second gap neither patch closes. A lowercase variable > name passes the regexp, so an append written with ${machine} rather than > ${MACHINE} becomes a flag whose override name is still > pn-gizmo-${machine}, and that name is never expanded when overrides are > matched. Master, your diff and mine all return "base" for it. Nobody > writes ${machine} in practice, but it is another way to lose the same > operation. > > Would you like me to fix the drop now and report the ordering as its own > bug, or look at these together? I did not want to send a v2 that changes > ordering without asking first. Were you going to send an updated patch? I think the issues are separate, we should fix the operations in one patch. I'm not convinced the ordering issue is "real" in that we've not commited to any particular order, only that for a given bitbake version, it should always be consistent. It would be good to close out the first issue. I'm happy to write some patches if you're not able to. Cheers, Richard
diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py index 9961269a3..5738aff95 100644 --- a/lib/bb/data_smart.py +++ b/lib/bb/data_smart.py @@ -697,8 +697,15 @@ class DataSmart(MutableMapping): found = True self.overridedata[newkey] = [] for (v, o) in self.overridedata[key]: - self.overridedata[newkey].append([v.replace(key, newkey), o]) - self.renameVar(v, v.replace(key, newkey)) + newv = v.replace(key, newkey) + # The derived name may still hold a variable reference which + # expandKeys() will never revisit, so expand it here. Only once + # newkey is expanded though, otherwise expandKeys() has still to + # rename newkey and handles the dependent keys along with it. + if '${' in newv and '${' not in newkey: + newv = self.expand(newv) + self.overridedata[newkey].append([newv, o]) + self.renameVar(v, newv) if not found: # No variable to rename so not worth the work in writing extra diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py index fd690a9e2..5ef159eae 100644 --- a/lib/bb/tests/data.py +++ b/lib/bb/tests/data.py @@ -406,6 +406,31 @@ class TestOverrides(unittest.TestCase): bb.data.expandKeys(self.d) self.assertEqual(self.d.getVar("VERSION"), "2") + # Test an :append whose override name is only resolved by key expansion + def test_append_in_expanded_override(self): + self.d.setVar("MACHINE", "qemux86") + self.d.setVar("PN", "gizmo") + self.d.setVar("OVERRIDES", "gizmo:pn-gizmo-qemux86") + self.d.setVar("TEST:${PN}", "base") + self.d.setVar("TEST:${PN}:append:pn-gizmo-${MACHINE}", " appended") + bb.data.expandKeys(self.d) + self.assertEqual(self.d.getVar("TEST"), "base appended") + + # Test renaming to a key which is itself not expanded yet, as native.bbclass + # does. The dependent override keys must be left for expandKeys() to rename. + def test_rename_to_unexpanded_key_with_override(self): + self.d.setVar("BPN", "gizmo") + self.d.setVar("PN", "gizmo-native") + self.d.setVar("OVERRIDES", "class-target") + self.d.setVar("TEST:${PN}-lib", "base") + self.d.setVar("TEST:${PN}-lib:class-target", "target") + with LogRecord() as logs: + self.d.renameVar("TEST:${PN}-lib", "TEST:${BPN}-lib-native") + bb.data.expandKeys(self.d) + self.assertFalse(logContains("renameVar with equivalent keys", logs)) + self.assertFalse(logContains("replaces original key", logs)) + self.assertEqual(self.d.getVar("TEST:gizmo-lib-native"), "target") + def test_remove_with_override(self): self.d.setVar("TEST:bar", "testvalue2") self.d.setVar("TEST:some_val", "testvalue3 testvalue5")
An operation whose override name needs key expansion is silently dropped: RDEPENDS:${PN}:append:pn-foo-${MACHINE} = " bar" renameVar() rebuilds the dependent override keys with a plain string replace, so renaming RDEPENDS:${PN} leaves the append attached to RDEPENDS:foo:append:pn-foo-${MACHINE}, which never matches an active override. expandKeys() cannot fix that up afterwards either, as it works from a list of keys collected before any renaming happened. Expand the derived name before renaming it. Only do so once newkey is itself expanded, otherwise expandKeys() has still to rename newkey and handles the dependent keys along with it. Add regression tests for both cases. Fixes [YOCTO #14867] Signed-off-by: Nguyen Minh Tien <zizuzacker@gmail.com> --- lib/bb/data_smart.py | 11 +++++++++-- lib/bb/tests/data.py | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-)