| Message ID | YfAgXED+BhsZg1qU@decadent.org.uk |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Lua: Log Lua error messages for external handlers | expand |
Hi Ben, On 25.01.22 17:07, Ben Hutchings wrote: > Currently, any failure in initialisation of the swupdate_handlers > package results in a generic error message from SWUpdate. This makes > it very difficult to debug initialisation failures. > > Add the same error logging that is done in the case of embedded Lua > handlers. > > Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> > --- > corelib/lua_interface.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > index fe977c1..15b9e72 100644 > --- a/corelib/lua_interface.c > +++ b/corelib/lua_interface.c > @@ -1238,6 +1238,8 @@ int lua_handlers_init(void) > #else > if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) { > INFO("No Lua handler(s) found."); > + TRACE("Lua exception:\n%s", lua_tostring(gL, -1)); Ok, but we to distinguish between real errors and not provided file. In most projects, it is foreseen to have an external file, but it is not provided leading to the "No Lua handler" - and this is not an error. Your case seems thatyou provide swupdate_handler.lua, but there is an error to load the scripts. This should be detected from the case where no file is found. Best regards, Stefano Babic > + lua_pop(gL, 1); > if (luaL_dostring(gL, "return package.path:gsub('?','swupdate_handlers'):gsub(';','\\0')") == 0) { > const char *paths = lua_tostring(gL, -2); > for (int i=lua_tonumber(gL, -1); i >= 0; i--) {
Stefano Babic wrote: > Hi Ben, > > On 25.01.22 17:07, Ben Hutchings wrote: > > Currently, any failure in initialisation of the swupdate_handlers > > package results in a generic error message from SWUpdate. This makes > > it very difficult to debug initialisation failures. > > > > Add the same error logging that is done in the case of embedded Lua > > handlers. > > > > Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> > > --- > > corelib/lua_interface.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > > index fe977c1..15b9e72 100644 > > --- a/corelib/lua_interface.c > > +++ b/corelib/lua_interface.c > > @@ -1238,6 +1238,8 @@ int lua_handlers_init(void) > > #else > > if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) { > > INFO("No Lua handler(s) found."); > > + TRACE("Lua exception:\n%s", lua_tostring(gL, -1)); > > Ok, but we to distinguish between real errors and not provided file. In > most projects, it is foreseen to have an external file, but it is not > provided leading to the "No Lua handler" - and this is not an error. > > Your case seems thatyou provide swupdate_handler.lua, but there is an > error to load the scripts. Right. The current behaviour makes it impossible to debug errors at load time. > This should be detected from the case where > no file is found. That's an interesting point. I don't think there is a way within the Lua API to make that distinction. Could we install an empty swupdate_handlers.lua if there is none already installed, so that "file not found" shouldn't happen? Ben.
Hi Ben, On 26.01.22 16:35, Ben HUTCHINGS (EXT) wrote: > Stefano Babic wrote: >> Hi Ben, >> >> On 25.01.22 17:07, Ben Hutchings wrote: >>> Currently, any failure in initialisation of the swupdate_handlers >>> package results in a generic error message from SWUpdate. This makes >>> it very difficult to debug initialisation failures. >>> >>> Add the same error logging that is done in the case of embedded Lua >>> handlers. >>> >>> Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> >>> --- >>> corelib/lua_interface.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c >>> index fe977c1..15b9e72 100644 >>> --- a/corelib/lua_interface.c >>> +++ b/corelib/lua_interface.c >>> @@ -1238,6 +1238,8 @@ int lua_handlers_init(void) >>> #else >>> if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) { >>> INFO("No Lua handler(s) found."); >>> + TRACE("Lua exception:\n%s", lua_tostring(gL, -1)); >> >> Ok, but we to distinguish between real errors and not provided file. In >> most projects, it is foreseen to have an external file, but it is not >> provided leading to the "No Lua handler" - and this is not an error. >> >> Your case seems thatyou provide swupdate_handler.lua, but there is an >> error to load the scripts. > > Right. The current behaviour makes it impossible to debug errors at load > time. I understand. > >> This should be detected from the case where >> no file is found. > > That's an interesting point. I don't think there is a way within the Lua API > to make that distinction. When I looked at it, I do not find anything, too. Another solution for you is CONFIG_EMBEDDED_LUA_HANDLER. In this case, swupdate_handler.lua is not loaded, but it is linked to SWUpdate itself. And then you can consider that, if "No Lua handler(s) found." is raised but CONFIG_EMBEDDED_LUA_HANDLER is set, this is absolutely an error and you can raise ERROR (instead of TRACE). > > Could we install an empty swupdate_handlers.lua if there is none already > installed, so that "file not found" shouldn't happen? I cannot drive how users have integrated SWUpdate, and in most cases there is no swupdate_handler.lua at all, but it is open if a next version will added it. So this changes how SWUpdate is integrated and I am quite sure it will break a lot of projects. Check if my suggestion with CONFIG_EMBEDDED_LUA_HANDLER is ok for you, and I can integrate a patch like: INFO("No Lua handler(s) found."); + if defined(CONFIG_EMBEDDED_LUA_HANDLER) + ERROR("Lua exception:\n%s", lua_tostring(gL, -1)); +endif Best regards, Stefano
Hi, > > > > Currently, any failure in initialisation of the swupdate_handlers > > > > package results in a generic error message from SWUpdate. This makes > > > > it very difficult to debug initialisation failures. > > > > > > > > Add the same error logging that is done in the case of embedded Lua > > > > handlers. > > > > > > > > Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> > > > > --- > > > > corelib/lua_interface.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > > > > index fe977c1..15b9e72 100644 > > > > --- a/corelib/lua_interface.c > > > > +++ b/corelib/lua_interface.c > > > > @@ -1238,6 +1238,8 @@ int lua_handlers_init(void) > > > > #else > > > > if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) { > > > > INFO("No Lua handler(s) found."); > > > > + TRACE("Lua exception:\n%s", lua_tostring(gL, -1)); > > > > > > Ok, but we to distinguish between real errors and not provided file. In > > > most projects, it is foreseen to have an external file, but it is not > > > provided leading to the "No Lua handler" - and this is not an error. > > > > > > Your case seems thatyou provide swupdate_handler.lua, but there is an > > > error to load the scripts. > > > > Right. The current behaviour makes it impossible to debug errors at load > > time. > > I understand. Well, for me, LSPs and linters do a good job in telling me whether I did something wrong in the Lua file, leaving the cases where interaction with SWUpdate goes wrong. In this case, I do add (a variation of) the above line though... > > > > > This should be detected from the case where > > > no file is found. > > > > That's an interesting point. I don't think there is a way within the Lua API > > to make that distinction. > > When I looked at it, I do not find anything, too. If you do require("module"), module gets searched for in package.path which is initialized with $LUA_PATH or a compile-time default if absent. So, you could iterate package.path to see if module can be found therein. If not, the file is absent. Otherwise, there was an error in the file. Another option is to place the file somewhere defined and use dofile() / luaL_dofile(). Then, it's obvious whether the file is present or not. The downside is that you have to define such a path and hence establish a convention (probably compile-time configurable). What to do about require("...")'s within this file then? They're still searched in package.path and if they're not satisfied it also breaks ― even if the file is present... > Another solution for you is CONFIG_EMBEDDED_LUA_HANDLER. In this case, > swupdate_handler.lua is not loaded, but it is linked to SWUpdate itself. And > then you can consider that, if "No Lua handler(s) found." is raised but > CONFIG_EMBEDDED_LUA_HANDLER is set, this is absolutely an error and you can > raise ERROR (instead of TRACE). Indeed. The only downside is that, while development, you need to relink SWUpdate so to embed the new Lua script instead of just modifying the Lua file and restarting SWUpdate. > > Could we install an empty swupdate_handlers.lua if there is none already > > installed, so that "file not found" shouldn't happen? See dofile() / luaL_dofile() above. > I cannot drive how users have integrated SWUpdate, and in most cases there is > no swupdate_handler.lua at all, but it is open if a next version will added > it. So this changes how SWUpdate is integrated and I am quite sure it will > break a lot of projects. Check if my suggestion with > CONFIG_EMBEDDED_LUA_HANDLER is ok for you, and I can integrate a patch like: > > INFO("No Lua handler(s) found."); > + if defined(CONFIG_EMBEDDED_LUA_HANDLER) > + ERROR("Lua exception:\n%s", lua_tostring(gL, -1)); > +endif This makes sense. Kind regards, Christian
Stefano Babic wrote: > Hi Ben, > > On 26.01.22 16:35, Ben HUTCHINGS (EXT) wrote: > > Stefano Babic wrote: > >> Hi Ben, > >> > >> On 25.01.22 17:07, Ben Hutchings wrote: > >>> Currently, any failure in initialisation of the swupdate_handlers > >>> package results in a generic error message from SWUpdate. This makes > >>> it very difficult to debug initialisation failures. > >>> > >>> Add the same error logging that is done in the case of embedded Lua > >>> handlers. > >>> > >>> Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> > >>> --- > >>> corelib/lua_interface.c | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > >>> index fe977c1..15b9e72 100644 > >>> --- a/corelib/lua_interface.c > >>> +++ b/corelib/lua_interface.c > >>> @@ -1238,6 +1238,8 @@ int lua_handlers_init(void) > >>> #else > >>> if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) { > >>> INFO("No Lua handler(s) found."); > >>> + TRACE("Lua exception:\n%s", lua_tostring(gL, -1)); > >> > >> Ok, but we to distinguish between real errors and not provided file. In > >> most projects, it is foreseen to have an external file, but it is not > >> provided leading to the "No Lua handler" - and this is not an error. > >> > >> Your case seems thatyou provide swupdate_handler.lua, but there is an > >> error to load the scripts. > > > > Right. The current behaviour makes it impossible to debug errors at load > > time. > > I understand. > > > > >> This should be detected from the case where > >> no file is found. > > > > That's an interesting point. I don't think there is a way within the Lua API > > to make that distinction. > > When I looked at it, I do not find anything, too. > > Another solution for you is CONFIG_EMBEDDED_LUA_HANDLER. In this case, > swupdate_handler.lua is not loaded, but it is linked to SWUpdate itself. > And then you can consider that, if "No Lua handler(s) found." is raised > but CONFIG_EMBEDDED_LUA_HANDLER is set, this is absolutely an error and > you can raise ERROR (instead of TRACE). [...] I looked back at this and realised that I don't understand what your objection is to this patch. I understand that you don't want to see an error reported if external Lua handlers are supported but not installed. But this patch doesn't do that. Ben.
Hi Ben, On 07.02.22 21:00, Ben HUTCHINGS (EXT) wrote: > Stefano Babic wrote: >> Hi Ben, >> >> On 26.01.22 16:35, Ben HUTCHINGS (EXT) wrote: >>> Stefano Babic wrote: >>>> Hi Ben, >>>> >>>> On 25.01.22 17:07, Ben Hutchings wrote: >>>>> Currently, any failure in initialisation of the swupdate_handlers >>>>> package results in a generic error message from SWUpdate. This makes >>>>> it very difficult to debug initialisation failures. >>>>> >>>>> Add the same error logging that is done in the case of embedded Lua >>>>> handlers. >>>>> >>>>> Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> >>>>> --- >>>>> corelib/lua_interface.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c >>>>> index fe977c1..15b9e72 100644 >>>>> --- a/corelib/lua_interface.c >>>>> +++ b/corelib/lua_interface.c >>>>> @@ -1238,6 +1238,8 @@ int lua_handlers_init(void) >>>>> #else >>>>> if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) { >>>>> INFO("No Lua handler(s) found."); >>>>> + TRACE("Lua exception:\n%s", lua_tostring(gL, -1)); >>>> >>>> Ok, but we to distinguish between real errors and not provided file. In >>>> most projects, it is foreseen to have an external file, but it is not >>>> provided leading to the "No Lua handler" - and this is not an error. >>>> >>>> Your case seems thatyou provide swupdate_handler.lua, but there is an >>>> error to load the scripts. >>> >>> Right. The current behaviour makes it impossible to debug errors at load >>> time. >> >> I understand. >> >>> >>>> This should be detected from the case where >>>> no file is found. >>> >>> That's an interesting point. I don't think there is a way within the Lua API >>> to make that distinction. >> >> When I looked at it, I do not find anything, too. >> >> Another solution for you is CONFIG_EMBEDDED_LUA_HANDLER. In this case, >> swupdate_handler.lua is not loaded, but it is linked to SWUpdate itself. >> And then you can consider that, if "No Lua handler(s) found." is raised >> but CONFIG_EMBEDDED_LUA_HANDLER is set, this is absolutely an error and >> you can raise ERROR (instead of TRACE). > [...] > > I looked back at this and realised that I don't understand what > your objection is to this patch. > > I understand that you don't want to see an error reported if > external Lua handlers are supported but not installed. Ys, more or less. > But this > patch doesn't do that. What I see applying the patch and dropping swupdate_handler.lua, that is one of frequent use case where Lua handler is just activated : Without patch: --------------- Swupdate v2021.11.0 Licensed under GPLv2. See source distribution for detailed copyright notices. [INFO ] : SWUPDATE running : [lua_handlers_init] : No Lua handler(s) found. [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/local/share/lua/5.2/swupdate_handlers.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/local/share/lua/5.2/swupdate_handlers/init.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/local/lib/lua/5.2/swupdate_handlers.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/local/lib/lua/5.2/swupdate_handlers/init.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/share/lua/5.2/swupdate_handlers.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/share/lua/5.2/swupdate_handlers/init.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : ./swupdate_handlers.lua ==> no errors, Lua search path goes to trace,everything in SWUpdate's log. With the patch: ---------------- [INFO ] : SWUPDATE running : [lua_handlers_init] : No Lua handler(s) found. [TRACE] : SWUPDATE running : [lua_handlers_init] : Lua exception: [string "require ("swupdate_handlers")"]:1: module 'swupdate_handlers' not found: no field package.preload['swupdate_handlers'] no file '/usr/local/share/lua/5.2/swupdate_handlers.lua' no file '/usr/local/share/lua/5.2/swupdate_handlers/init.lua' no file '/usr/local/lib/lua/5.2/swupdate_handlers.lua' no file '/usr/local/lib/lua/5.2/swupdate_handlers/init.lua' no file '/usr/share/lua/5.2/swupdate_handlers.lua' no file '/usr/share/lua/5.2/swupdate_handlers/init.lua' no file './swupdate_handlers.lua' no file '/usr/local/lib/lua/5.2/swupdate_handlers.so' no file '/usr/lib/x86_64-linux-gnu/lua/5.2/swupdate_handlers.so' no file '/usr/lib/lua/5.2/swupdate_handlers.so' no file '/usr/local/lib/lua/5.2/loadall.so' no file './swupdate_handlers.so' ==> all of them are duplicated because they are already in, and they are just printed on stdout, even if they are not (TRACE / ERRORS drop everything after \n). [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/local/share/lua/5.2/swupdate_handlers.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/local/share/lua/5.2/swupdate_handlers/init.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/local/lib/lua/5.2/swupdate_handlers.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/local/lib/lua/5.2/swupdate_handlers/init.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/share/lua/5.2/swupdate_handlers.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : /usr/share/lua/5.2/swupdate_handlers/init.lua [TRACE] : SWUPDATE running : [lua_handlers_init] : ./swupdate_handlers.lua Best regards, Stefano
diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index fe977c1..15b9e72 100644 --- a/corelib/lua_interface.c +++ b/corelib/lua_interface.c @@ -1238,6 +1238,8 @@ int lua_handlers_init(void) #else if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) { INFO("No Lua handler(s) found."); + TRACE("Lua exception:\n%s", lua_tostring(gL, -1)); + lua_pop(gL, 1); if (luaL_dostring(gL, "return package.path:gsub('?','swupdate_handlers'):gsub(';','\\0')") == 0) { const char *paths = lua_tostring(gL, -2); for (int i=lua_tonumber(gL, -1); i >= 0; i--) {
Currently, any failure in initialisation of the swupdate_handlers package results in a generic error message from SWUpdate. This makes it very difficult to debug initialisation failures. Add the same error logging that is done in the case of embedded Lua handlers. Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> --- corelib/lua_interface.c | 2 ++ 1 file changed, 2 insertions(+)