diff mbox series

[RFC] ast: Better variable history for builtin fragments

Message ID 20251009160717.2527351-1-yoann.congal@smile.fr
State New
Headers show
Series [RFC] ast: Better variable history for builtin fragments | expand

Commit Message

Yoann Congal Oct. 9, 2025, 4:07 p.m. UTC
From: Yoann Congal <yoann.congal@smile.fr>

As of now, the variable history for builtin fragments looks like this
(edited for clarity):
   $ bitbake-getvar MACHINE
  #
  # $MACHINE [2 operations]
  #   set ast.py:368 [check_and_set_builtin_fragment]
  #     "qemux86-64"
  # pre-expansion value:
  #   "qemux86-64"
  MACHINE="qemux86-64"
User can't know where MACHINE was set, this is bad.

This patch tries to reconstruct a MACHINE history from OE_FRAGMENTS
history.
With this patch, history looks like this (for a simple case):
   $ bitbake-getvar MACHINE
  NOTE: Starting bitbake server...
  #
  # $MACHINE [2 operations]
  #   set .../auto.conf:2
  #     "qemux86-64 (OE_FRAGMENTS contains "machine/qemux86-64")"
  # pre-expansion value:
  #   "qemux86-64"
  MACHINE="qemux86-64"
The path where the "machine/qemux86-64" fragment was added to
OE_FRAGMENTS is displayed, this is definitely better.

But, (and this is why this patch is a RFC), on a complex case : adding
a fragment, another, then the first one again, history looks like this:
   $ bitbake-getvar MACHINE
  NOTE: Starting bitbake server...
  #
  # $MACHINE [4 operations]
  #   set .../auto.conf:4     # <<<<=== This line number is wrong
  #     "qemux86-64 (OE_FRAGMENTS contains "machine/qemux86-64")"
  #   set .../auto.conf:3
  #     "test (OE_FRAGMENTS contains "machine/test")"
  #   set .../auto.conf:4
  #     "qemux86-64 (OE_FRAGMENTS contains "machine/qemux86-64")"
  # pre-expansion value:
  #   "qemux86-64"
  MACHINE="qemux86-64"
I don't know how to match OE_FRAGMENTS history with MACHINE history in
this case. When MACHINE is set to "qemux86-64", how does the code can
know whether it is the first instance or the second? (My patch take the
last one)

Also, a fragment can be removed from OE_FRAGMENTS with:
OE_FRAGMENTS:remove = "machine/test"
In this case, as of now, that does not generate a line in MACHINE
variable history. I not entirely sure this is a problem or not.

Related to [YOCTO #15939]

Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
---
 lib/bb/parse/ast.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Alexander Kanavin Oct. 9, 2025, 4:28 p.m. UTC | #1
On Thu, 9 Oct 2025 at 18:07, Yoann Congal via lists.openembedded.org
<yoann.congal=smile.fr@lists.openembedded.org> wrote:
> But, (and this is why this patch is a RFC), on a complex case : adding
> a fragment, another, then the first one again, history looks like this:
>    $ bitbake-getvar MACHINE
>   NOTE: Starting bitbake server...
>   #
>   # $MACHINE [4 operations]
>   #   set .../auto.conf:4     # <<<<=== This line number is wrong
>   #     "qemux86-64 (OE_FRAGMENTS contains "machine/qemux86-64")"
>   #   set .../auto.conf:3
>   #     "test (OE_FRAGMENTS contains "machine/test")"
>   #   set .../auto.conf:4
>   #     "qemux86-64 (OE_FRAGMENTS contains "machine/qemux86-64")"
>   # pre-expansion value:
>   #   "qemux86-64"
>   MACHINE="qemux86-64"
> I don't know how to match OE_FRAGMENTS history with MACHINE history in
> this case. When MACHINE is set to "qemux86-64", how does the code can
> know whether it is the first instance or the second? (My patch take the
> last one)

I don't think this is something we should worry about. The tool for
managing fragments is fixed to not allow this anymore:
https://git.openembedded.org/openembedded-core/commit/?h=master&id=aea2d69d0533bf005cd58eb91fd9b3a3ae194610
and if users end up setting the same built-in fragments multiple times
some other way, that's a misconfiguration on their part.

Alex
Yoann Congal Oct. 9, 2025, 5:12 p.m. UTC | #2
Le jeu. 9 oct. 2025 à 18:29, Alexander Kanavin <alex.kanavin@gmail.com> a
écrit :

> On Thu, 9 Oct 2025 at 18:07, Yoann Congal via lists.openembedded.org
> <yoann.congal=smile.fr@lists.openembedded.org> wrote:
> > But, (and this is why this patch is a RFC), on a complex case : adding
> > a fragment, another, then the first one again, history looks like this:
> >    $ bitbake-getvar MACHINE
> >   NOTE: Starting bitbake server...
> >   #
> >   # $MACHINE [4 operations]
> >   #   set .../auto.conf:4     # <<<<=== This line number is wrong
> >   #     "qemux86-64 (OE_FRAGMENTS contains "machine/qemux86-64")"
> >   #   set .../auto.conf:3
> >   #     "test (OE_FRAGMENTS contains "machine/test")"
> >   #   set .../auto.conf:4
> >   #     "qemux86-64 (OE_FRAGMENTS contains "machine/qemux86-64")"
> >   # pre-expansion value:
> >   #   "qemux86-64"
> >   MACHINE="qemux86-64"
> > I don't know how to match OE_FRAGMENTS history with MACHINE history in
> > this case. When MACHINE is set to "qemux86-64", how does the code can
> > know whether it is the first instance or the second? (My patch take the
> > last one)
>
> I don't think this is something we should worry about. The tool for
> managing fragments is fixed to not allow this anymore:
>
> https://git.openembedded.org/openembedded-core/commit/?h=master&id=aea2d69d0533bf005cd58eb91fd9b3a3ae194610
> and if users end up setting the same built-in fragments multiple times
> some other way, that's a misconfiguration on their part.
>

Should we think about displaying a warning in this case (multiple values
for the same fragment) ?
Alexander Kanavin Oct. 9, 2025, 5:19 p.m. UTC | #3
On Thu, 9 Oct 2025 at 19:12, Yoann Congal <yoann.congal@smile.fr> wrote:
>> and if users end up setting the same built-in fragments multiple times
>> some other way, that's a misconfiguration on their part.
>
>
> Should we think about displaying a warning in this case (multiple values for the same fragment) ?

I think so, yes.

Alex
Yoann Congal Oct. 10, 2025, 6:36 a.m. UTC | #4
Le jeu. 9 oct. 2025 à 19:19, Alexander Kanavin <alex.kanavin@gmail.com> a
écrit :

> On Thu, 9 Oct 2025 at 19:12, Yoann Congal <yoann.congal@smile.fr> wrote:
> >> and if users end up setting the same built-in fragments multiple times
> >> some other way, that's a misconfiguration on their part.
> >
> >
> > Should we think about displaying a warning in this case (multiple values
> for the same fragment) ?
>
> I think so, yes.
>

Patch sent here:
[PATCH] ast: Warn on multiple builtin config fragments for the same variable
https://lists.openembedded.org/g/bitbake-devel/topic/patch_ast_warn_on_multiple/115680515


>
> Alex
>
diff mbox series

Patch

diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py
index cb06e8917..27adbd860 100644
--- a/lib/bb/parse/ast.py
+++ b/lib/bb/parse/ast.py
@@ -364,8 +364,16 @@  class AddFragmentsNode(AstNode):
         def check_and_set_builtin_fragment(fragment, data, builtin_fragments):
             prefix, value = fragment.split('/', 1)
             if prefix in builtin_fragments.keys():
+                fragment_history = data.varhistory.variable(self.fragments_variable)
+                loginfo={}
+                for fh in fragment_history[::-1]:
+                    if fh['op'] in ("set", "append") and fragment in fh["detail"]:
+                        loginfo["file"]   = fh["file"]
+                        loginfo["line"]   = fh["line"]
+                        loginfo["detail"] = f"{value} ({self.fragments_variable} contains \"{fragment}\")"
+                        break
                 # parsing=True since we want to emulate X=Y and allow X:override=Z to continue to exist
-                data.setVar(builtin_fragments[prefix], value, parsing=True)
+                data.setVar(builtin_fragments[prefix], value, parsing=True, **loginfo)
                 return True
             return False