e.data.getVar() gets a bit old in a large event handler, and it means a simple handler has to be modified if switching between an event handler (e.g. RecipeParsed) and anonymous python. I think it would make sense to restore the 'd' convention here to align with python elsewhere. It'd just be a convenience, d==e.data, to avoid the common pattern of setting it at the top of the event handler.
I had a look at doing this, it turns out to be quite hard/ugly. The issue is that the event handler code is precompiled and takes a single argument, e. In order to get the same behaviour as anonymous python, we'd need to pass d into the locals() of the function but to do that, as far as I can tell we need exec/eval. exec/eval don't work with functions which take parameters. There are tricks you can use to exec/eval functions with parameters but they work by wrapping the function, then we lose the ability to influence locals again. I also tried injecting d into globals() however globals() is module specific and we therefore can't access globals past the first function exec where a different namespace takes over. This left __builtins__ and something like: diff --git a/bitbake/lib/bb/event.py b/bitbake/lib/bb/event.py index f755eb2..80e3796 100644 --- a/bitbake/lib/bb/event.py +++ b/bitbake/lib/bb/event.py @@ -72,6 +72,7 @@ _eventfilter = None def execute_handler(name, handler, event, d): event.data = d + __builtins__['d'] = d try: ret = handler(event) except bb.parse.SkipRecipe: @@ -87,6 +88,7 @@ def execute_handler(name, handler, event, d): logger.error("Execution of event handler '%s' failed (exit code %s)" % (name, exc.code)) finally: del event.data + del __builtins__['d'] def fire_class_handlers(event, d): if isinstance(event, logging.LogRecord): does work. Do we accept that ugliness though?
Is there a reason we don't just add a second argument? I realize it's probably not conceptually ideal, since setting d is just a convenience, but it would work. As far as I know, the only code which compiles the handler functions is bb.event.register(), so it should be trivial to change the signature of the handler functions. Alternatively, we could inject d = e.data as the first line of the functions, but then we'd need a way to adjust the line numbers for tracebacks, so probably not worthwhile. diff --git i/bitbake/lib/bb/event.py w/bitbake/lib/bb/event.py index ba0de11..1399ba5 100644 --- i/bitbake/lib/bb/event.py +++ w/bitbake/lib/bb/event.py @@ -73,7 +73,7 @@ _eventfilter = None def execute_handler(name, handler, event, d): event.data = d try: - ret = handler(event) + ret = handler(event, d) except (bb.parse.SkipRecipe, bb.BBHandledException): raise except Exception: @@ -180,9 +180,9 @@ def register(name, handler, mask=[]): if handler is not None: # handle string containing python code if isinstance(handler, basestring): - tmp = "def %s(e):\n%s" % (name, handler) + tmp = "def %s(e, d):\n%s" % (name, handler) try: - code = compile(tmp, "%s(e)" % name, "exec") + code = compile(tmp, "%s(e, d)" % name, "exec") except SyntaxError: logger.error("Unable to register event handler '%s':\n%s", name, ''.join(traceback.format_exc(limit=0)))
http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=df123ddab43a221cc26028e0aff211ac34c1eafc(In reply to comment #2) > Is there a reason we don't just add a second argument? I realize it's > probably not conceptually ideal, since setting d is just a convenience, but > it would work. As far as I know, the only code which compiles the handler > functions is bb.event.register(), so it should be trivial to change the > signature of the handler functions. > > Alternatively, we could inject d = e.data as the first line of the > functions, but then we'd need a way to adjust the line numbers for > tracebacks, so probably not worthwhile. > > diff --git i/bitbake/lib/bb/event.py w/bitbake/lib/bb/event.py > index ba0de11..1399ba5 100644 > --- i/bitbake/lib/bb/event.py > +++ w/bitbake/lib/bb/event.py > @@ -73,7 +73,7 @@ _eventfilter = None > def execute_handler(name, handler, event, d): > event.data = d > try: > - ret = handler(event) > + ret = handler(event, d) > except (bb.parse.SkipRecipe, bb.BBHandledException): > raise > except Exception: > @@ -180,9 +180,9 @@ def register(name, handler, mask=[]): > if handler is not None: > # handle string containing python code > if isinstance(handler, basestring): > - tmp = "def %s(e):\n%s" % (name, handler) > + tmp = "def %s(e, d):\n%s" % (name, handler) > try: > - code = compile(tmp, "%s(e)" % name, "exec") > + code = compile(tmp, "%s(e, d)" % name, "exec") > except SyntaxError: > logger.error("Unable to register event handler '%s':\n%s", > name, > ''.join(traceback.format_exc(limit=0))) The reason I didn't do this is that that under that if block, the register function would appear to take functions as handlers as well as strings. Those functions would then break. At the time I assumed something used that code path, perhaps it doesn't though?
http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=df123ddab43a221cc26028e0aff211ac34c1eafc http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=8c3dd2d9eb2c593d5b7cf28d7627e6725ab7dbfe Still considering the change Chris proposed though...