ElunaLuaEngine / Eluna

Eluna Lua Engine © for WoW Emulators
https://elunaluaengine.github.io
GNU General Public License v3.0
372 stars 356 forks source link

Fix compilation with luajit and 5.1 #480

Closed Rochet2 closed 5 months ago

Rochet2 commented 5 months ago

Looks like lua51 has errors:

[ 39%] Building CXX object src/server/game/CMakeFiles/game.dir/LuaEngine/LuaValue.cpp.o
/home/runner/work/Eluna/Eluna/src/server/game/LuaEngine/LuaValue.cpp: In static member function ‘static LuaVal* LuaVal::GetLuaVal(lua_State*, int)’:
/home/runner/work/Eluna/Eluna/src/server/game/LuaEngine/LuaValue.cpp:41:33: error: ‘luaL_testudata’ was not declared in this scope; did you mean ‘luaL_checkudata’?
   41 |     return static_cast<LuaVal*>(luaL_testudata(L, index, LUAVAL_MT_NAME));
[ 39%] Building CXX object src/server/game/CMakeFiles/game.dir/LuaEngine/lmarshal.cpp.o
      |                                 ^~~~~~~~~~~~~~
      |                                 luaL_checkudata
compilation terminated due to -Wfatal-errors.
make[2]: *** [src/server/game/CMakeFiles/game.dir/build.make:2781: src/server/game/CMakeFiles/game.dir/LuaEngine/LuaValue.cpp.o] Error 1
/home/runner/work/Eluna/Eluna/src/server/game/LuaEngine/lmarshal.cpp: In function ‘int luaopen_marshal(lua_State*)’:
/home/runner/work/Eluna/Eluna/src/server/game/LuaEngine/lmarshal.cpp:579:5: error: ‘luaL_setfuncs’ was not declared in this scope; did you mean ‘lua_setfenv’?
  579 |     luaL_setfuncs(L, R, 0);
      |     ^~~~~~~~~~~~~
      |     lua_setfenv
compilation terminated due to -Wfatal-errors.
make[2]: *** [src/server/game/CMakeFiles/game.dir/build.make:2797: src/server/game/CMakeFiles/game.dir/LuaEngine/lmarshal.cpp.o] Error 1
[ 39%] Building CXX object src/server/game/CMakeFiles/game.dir/LuaEngine/hooks/BattleGroundHooks.cpp.o
/home/runner/work/Eluna/Eluna/src/server/game/LuaEngine/ElunaLoader.cpp: In member function ‘bool ElunaLoader::CompileScript(lua_State*, LuaScript&)’:
/home/runner/work/Eluna/Eluna/src/server/game/LuaEngine/ElunaLoader.cpp:202:16: error: ‘LUA_OK’ was not declared in this scope; did you mean ‘LUA_QL’?
  202 |     if (err != LUA_OK)
      |                ^~~~~~
      |                LUA_QL
compilation terminated due to -Wfatal-errors.
make[2]: *** [src/server/game/CMakeFiles/game.dir/build.make:2717: src/server/game/CMakeFiles/game.dir/LuaEngine/ElunaLoader.cpp.o] Error 1
[ 39%] Building CXX object src/server/game/CMakeFiles/game.dir/LuaEngine/hooks/CreatureHooks.cpp.o
/home/runner/work/Eluna/Eluna/src/server/game/LuaEngine/LuaEngine.cpp: In member function ‘void Eluna::OpenLua()’:
/home/runner/work/Eluna/Eluna/src/server/game/LuaEngine/LuaEngine.cpp:159:39: error: for increment expression has no effect [-Werror=unused-value]
  159 |     for (int i = lua_rawlen(L, -1); i >= newLoaderIndex; --i) {
      |                                     ~~^~~~~~~~~~~~~~~~~
compilation terminated due to -Wfatal-errors.
cc1plus: all warnings being treated as errors
make[2]: *** [src/server/game/CMakeFiles/game.dir/build.make:2749: src/server/game/CMakeFiles/game.dir/LuaEngine/LuaEngine.cpp.o] Error 1
[ 39%] Building CXX object src/server/game/CMakeFiles/game.dir/LuaEngine/hooks/GameObjectHooks.cpp.o
Foereaper commented 5 months ago

Should we even continue supporting 5.1 realistically? I don't think I've ever seen anyone specifically use it.

iThorgrim commented 5 months ago

It's strange don't have the same errors

Rochet2 commented 5 months ago

@Foereaper Well, maybe not. But luajit is basically same as lua51 + some extensions. @iThorgrim Lua51 has these errors and luajit only has one error - the one you reported.

I suspect that the issue is here

    #define lua_rawlen(L, idx) \
-        lua_objlen(L, idx);
+        lua_objlen(L, idx)

The semicolon should be removed as its not a part of the macro we replace. The result of lua_rawlen(L, idx); is currently probably lua_rawlen(L, idx);;

iThorgrim commented 5 months ago

@Foereaper Well, maybe not. But luajit is basically same as lua51 + some extensions. @iThorgrim Lua51 has these errors and luajit only has one error - the one you reported.

I suspect that the issue is here

    #define lua_rawlen(L, idx) \
-        lua_objlen(L, idx);
+        lua_objlen(L, idx)

The semicolon should be removed as its not a part of the macro we replace. The result of lua_rawlen(L, idx); is currently probably lua_rawlen(L, idx);;

Then, in terms of "usage", LuaJit is much more widely used than Lua53 / Lua54

Rochet2 commented 5 months ago

We can reduce the amount of lua versions we support. Currently we default to 5.2 due to backwards compatibility and ppl probably swap to jit for performance. Not sure if other versions are used.

iThorgrim commented 5 months ago

This works with LuaJit : https://github.com/ElunaLuaEngine/Eluna/pull/480/commits/c2bf6da5025bc27384c9f4abfd1105426627b129

Foereaper commented 5 months ago

I don't think 5.3 and 5.4 is widely used in the Lua community in general, they don't really give us anything that is super necessary so they could probably be dropped. I think 5.2 and LuaJIT are probably the more relevant ones to support, jit for obvious reasons and 5.2 for module support.

iThorgrim commented 5 months ago

I don't think 5.3 and 5.4 is widely used in the Lua community in general, they don't really give us anything that is super necessary so they could probably be dropped. I think 5.2 and LuaJIT are probably the more relevant ones to support, jit for obvious reasons and 5.2 for module support.

Luajit support really seems to me to be important and more "necessary" just when you see the performance there's not much to add. (https://eklausmeier.wordpress.com/2020/05/14/performance-comparison-pallene-vs-lua-5-1-5-2-5-3-5-4-vs-c/)

For Lua 5.3 and Lua 5.4 in any case, you'll need to update AIO and a lot of tools / scripts that use the bit system for example, which is now native.

I think a lot of people stuck to Lua 5.2 for their projects, since it was the "default" version of Eluna.

Foereaper commented 5 months ago

I tried fixing 5.1 support, and the more I fix the more is missing to fix it properly. I think we should at the very least drop 5.1 support at this point.

Rochet2 commented 5 months ago

For now the compilation is fixed. We can disable the other versions if we want to separately.