lunarmodules / luasql

LuaSQL is a simple interface from Lua to a DBMS.
http://lunarmodules.github.io/luasql
535 stars 192 forks source link

Broken Lua 5.1 compatibility? #147

Closed cpu closed 5 months ago

cpu commented 1 year ago

Hi there,

It looks to me (admittedly, not a Lua developer) like master broke compatibility with Lua 5.1 and 5.2 when https://github.com/lunarmodules/luasql/commit/ad59e6bf09b1eab5df02a7bc2bca056222a26030 was merged. That commit introduced two invocations of lua_isinteger:

git blame src/ls_sqlite3.c | grep lua_isinteger
ad59e6bf (Wolfgang Oertl    2021-08-18 13:00:01 +0200 402)     if (lua_isinteger(L, arg)) {
ad59e6bf (Wolfgang Oertl    2021-08-18 13:00:01 +0200 450)     if (tt == LUA_TNUMBER && lua_isinteger(L, -2)) {

Building sqlite3.so from this project with Lua 5.1 results in an error loading the module at runtime in my dependent application:

/nix/store/ng8m8g0sddihql99nds5z8amd30qiaig-lua-5.1.5-env/lib/lua/5.
1/luasql/sqlite3.so: undefined symbol: lua_isinteger
            Database support will not be available.

Like the error hints I'm only able to find the lua_isinteger symbol in Lua 5.3's API documentation, not 5.1 or 5.2. The lua_isinteger function is also listed as one of the C API functions backfilled by the lua-compat-5.3 project (potentially a good way to fix this compat. break?).

I believe the problematic commit hasn't yet been included in a tagged Luarock release but I ran into it downstream in NixPkgs and suspect it might soon bite other unsuspecting users stuck on older Lua.

Thanks!

tomasguisasola commented 5 months ago

Could you test the following?

diff --git a/src/ls_sqlite3.c b/src/ls_sqlite3.c
index 4a5e5a0..28d70aa 100644
--- a/src/ls_sqlite3.c
+++ b/src/ls_sqlite3.c
@@ -399,13 +399,17 @@ static int set_param(lua_State *L, sqlite3_stmt *vm, int param_nr, int arg)
     break;

     case LUA_TNUMBER:
+#if defined(lua_isinteger)
     if (lua_isinteger(L, arg)) {
       lua_Integer val = lua_tointeger(L, arg);
       rc = sqlite3_bind_int64(vm, param_nr, val);
     } else {
+#endif
       double val = lua_tonumber(L, arg);
       rc = sqlite3_bind_double(vm, param_nr, val);
+#if defined(lua_isinteger)
     }
+#endif
     break;

     default:
cpu commented 5 months ago

Hi @tomasguisasola, thanks for the patch.

There was another lua_isinteger call-site to consider in raw_readparams_table in addition to the one in set_param. PTAL at https://github.com/lunarmodules/luasql/pull/158 where I extended your methodology to that fn. I'm able to confirm that branch resolves the issue w/ Lua 5.1