Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compilation with luajit and 5.1 #480

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Fix compilation with luajit and 5.1 #480

merged 5 commits into from
Apr 8, 2024

Conversation

Rochet2
Copy link
Member

@Rochet2 Rochet2 commented Apr 8, 2024

No description provided.

@Rochet2
Copy link
Member Author

Rochet2 commented Apr 8, 2024

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
Copy link
Member

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

@iThorgrim
Copy link
Contributor

It's strange don't have the same errors

@Rochet2
Copy link
Member Author

Rochet2 commented Apr 8, 2024

@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
Copy link
Contributor

@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
Copy link
Member Author

Rochet2 commented Apr 8, 2024

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
Copy link
Contributor

This works with LuaJit : c2bf6da

@Foereaper
Copy link
Member

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
Copy link
Contributor

iThorgrim commented Apr 8, 2024

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
Copy link
Member

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 Rochet2 merged commit 3d1ead7 into master Apr 8, 2024
0 of 30 checks passed
@Rochet2 Rochet2 deleted the fix-jit branch April 8, 2024 21:49
@Rochet2
Copy link
Member Author

Rochet2 commented Apr 8, 2024

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

@Rochet2 Rochet2 changed the title Fix compilation with luajit Fix compilation with luajit and 5.1 Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants