Re: [bitbake-devel] [PATCH 1/1] data_smart: fix operations lost when an override name contains a variable
Richard Purdie <[email protected]>
| Newsgroups | org.openembedded.lists.bitbake-devel |
|---|---|
| Message-ID | <bf89837cf89ae1bdfdabc4adc4bf5bf49593e72d.camel@linuxfoundation.org> |
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