Enable use of system Lua 5.4 C++ on non-Windows platforms, fix pcall()/xpcall() blocking jailbreak exceptions, and fix strict mode missing some Lua errors #8234
Conversation
github-actions
bot
added
Building
|
As expected, the "ubuntu (cmake, gcc, g++, release, false, true)" job I added in the last commit is failing because the Docker container needs to be rebuilt per the Dockerfile change in the same commit. |
|
As far as scons check is concerned you could look for files manually(for example with FindFile()) and do CheckLib() call only if it's found. CheckLib() does more than just looking for file, it compiles a test program. |
|
Thanks, yeah, I realize SCons.Conftest.CheckLib() compiles a test program; I intended that. (I read SCons.SConf and SCons.Conftest source code to understand what all the functions actually do.) FindFile() can at least help find headers and reduce the noise from SCons.Conftest.CheckLib(), since I already had to hardcode include paths (which is documented in the SCons manual, so it's apparently idiomatic). But to find the library file, I'd have to also hardcode library paths (which on many distributions contain a GNU system type triplet). Does SCons really not have a portable way to ask the compiler what include and library paths it uses, or a built-in list of paths? I couldn't find any, hence I just compiled a test program a bunch of times to let the compiler find the library. |
|
I amended scons/lua.py to use SCons.Environment.FindFile() to at least reduce the number of SCons.Conftest.CheckLib() calls. |
|
Somehow I missed and/or forgot about So by searching for the include directory before trying to compile and link a test program against the library, there are now at most eight "Checking for C++ library ..." messages. Still not perfect (which again would require either getting library paths from the compiler or silencing |
|
I think the system lua job should be part of one of the other Ubuntu jobs. We did once get our CI turned off by Github because it was using so many resources it got flagged as being suspicious, so I'm not keen on adding more jobs unless it's unavoidable. |
|
In the current state this seems good, but i am little worried that this could eventually end up with different wesnoth versions using different lua versions (for example when a new lua 5.5 comes out), which could lead to addons working on one client but not on another which is something we put quite some effort in to avoid. Also it could be a possible source of OOS errors when lua behaves differently on one client. |
pehjota
force-pushed
the
system-lua
branch
2 times, most recently
from
1624ff8
to
97fda4c
Compare
Yes, I wasn't sure if I should be adding more jobs to the CI. Done: there are once again two Ubuntu jobs, one with submodule Lua and one with system Lua.
Lua version differences aren't possible with this PR. The CMake and SCons checks look for a specific major/minor version of Lua, so Lua 5.5 won't be used until Wesnoth requires that 5.5 (and not 5.4 anymore) be used. And I added documentation at src/modules/lua_README.md to hopefully remind anyone who updates the submodule to also update the Lua major/minor version in those checks. (If there's somewhere more prominent this should go, I'm open to suggestions.) |
pehjota
force-pushed
the
system-lua
branch
2 times, most recently
from
6a4f155
to
0ebc83c
Compare
|
Rebased and added to src/modules/lua_README.md a sentence about updating the other CMake modules. Is src/modules/lua_README.md a good place for instructions on updating Lua, or is there somewhere more prominent it should go? Any chance of seeing this in 1.17.25 (releasing next Sunday I assume), or is that not enough time to collect more reviews/testing and to update the Docker container? |
sound good to me then |
|
Seems the macOS builds are failing still: |
|
We probably need to add an |
Yeah, huh, that's odd.
Would that help? With GCC at least, |
github-actions
bot
added
AI
|
Two new commits: I removed the warning-suppression Lua wrapper headers Then I disabled building the Lua submodule when it's unused with |
|
macos-intel still fails: CMake via Xcode CompileC isn't giving clang In fact, CompileC isn't giving clang any Maybe we do need wrapper headers after all (but not with the same names as Lua's headers, to avoid recursion). And I guess we could compile with a I know nothing about Xcode. Any ideas? |
|
The wrapper headers can't be removed - they're there to suppress certain warnings in the lua code that would otherwise cause the build to fail due to 0052d26 fixes the error where it's not using |
|
Using But I think I'll have to keep the wrapper headers and add a |
pehjota
force-pushed
the
system-lua
branch
2 times, most recently
from
081cec1
to
f124ef2
Compare
Jailbreak exceptions thrown in Wesnoth C++ code called under Lua pcall() or xpcall() aren't handled immediately, allowing such exceptions to potentially accumulate and cause a failed assertion. For example, if a user tries to quit while Lua code is running under pcall() or xpcall(), LUAI_TRY() stores and consumes the jailbreak exception and the rest of the Lua code continues running. If the user tries to quit again before the Lua code finishes, LUAI_TRY() attempts to store another jailbreak exception, which causes wesnoth to abort. This is a longstanding bug (since jailbreak exceptions were introduced in commit d6512a0, version 1.9.5) that could affect World Conquest and 16 of the 588 addons in 1.16 and 7 of the 141 in 1.17. Fix this by wrapping pcall() and xpcall() to rethrow jailbreak exceptions.
Wesnoth's Lua submodule is built with LUAI_TRY() defined to catch a std::exception, push its what() string onto the Lua stack, and call the error handler given via lua_pcall() (which push_error_handler() and luaW_pcall_internal() set to Lua's debug.traceback()) or in Lua via xpcall(). If lua_pcall() returns an error code, luaW_pcall() logs an error, but only if it finds an exception string on the stack (due to an apparent oversight in commit 3820b14, version 1.9.5) and that exception string doesn't contain "~lua:" (oversight in commit e5562a1, version 1.9.0). But stock LUAI_TRY() doesn't push an exception string onto the stack, so luaW_pcall() won't log the error. Either way, luaW_pcall() always returns false when lua_pcall() returns an error, so the calling C++ code works the same with either Wesnoth or stock LUAI_TRY(). The only consequence is that strict mode doesn't break, because the error isn't logged. This will cause the filter_formula_unit_error test to return a result of "PASS TEST (0)" instead of "BROKE STRICT (PASS) (9)" with stock LUAI_TRY(). In other words, the test passes normally either way, but strict mode doesn't break. Fix this by making luaW_pcall() log all errors, including those without exception strings on the stack, as well as those with exception strings containing "~lua:".
These macros were in Lua 5.3 but have been removed in Lua 5.4: lua/lua@34b00c1
The documentation for IMPLEMENT_LUA_JAILBREAK_EXCEPTION() says, "This macro needs to be placed in the definition of the most derived class, which uses lua_jailbreak_exception as baseclass." Storing exceptions in constructors will require this to mean that classes that derive from lua_jailbreak_exception must be treated as final, because of the "assert(!jailbreak_exception);" in lua_jailbreak_exception::store(). Specifying overridden clone() and execute() functions as final in IMPLEMENT_LUA_JAILBREAK_EXCEPTION() enforces this. leavegame_wesnothd_error is derived from a class that derives from lua_jailbreak_exception, so also fix this inheritance and a block that might catch leavegame_wesnothd_error.
LUAI_TRY() is an internal part of Lua that may not exist forever, and
reliance on overriding it prevents the use of system copies of Lua.
Document in lua_jailbreak_exception this requirement to call
this->store() in derived class constructors.
Also, count the luaW_pcall_internal() recursion depth and store and
rethrow jailbreak exceptions until the recursion depth reaches 0,
because:
1. luaW_pcall_internal() sometimes runs recursively (C++ calls Lua
calls C++ calls Lua calls C++), so the middle C++ layer needs to
rethrow exceptions instead of clearing them. LUAI_TRY() previously
stored them each time, but now lua_jailbreak_exception::rethrow()
needs to know when not to clear().
2. Jailbreak exceptions can be thrown while no Lua code is running.
Now that constructors store() all exceptions instead of LUAI_TRY()
storing only those caught by Lua, lua_jailbreak_exception::store()
needs to know when not to store them. Otherwise, for example,
leaving a game to return to the menu while no Lua code is running
throws and needlessly stores an exception that isn't cleared, with
two possible effects:
a. If another jailbreak exception is thrown and the constructor
tries to store it, we abort from assert() because one is already
stored.
b. Otherwise, if luaW_pcall_internal() runs without a new jailbreak
exception being thrown, the stored one is rethrown and handled a
second time (e.g. immediately leaving a new game).
These will be changed to conditionally include system Lua headers,
e.g. "lua.h", instead of submodule Lua headers, e.g. "module/lua/lua.h".
If a header named "lua.h" includes "lua.h", the build will fail due to
recursion.
This can't be solved using angle brackets to include system headers,
because macos builds won't find them:
In file included from /Users/runner/work/wesnoth/wesnoth/src/ai/registry.cpp:30:
In file included from /Users/runner/work/wesnoth/wesnoth/src/ai/composite/aspect.hpp:24:
In file included from /Users/runner/work/wesnoth/wesnoth/src/ai/lua/lua_object.hpp:25:
/Users/runner/work/wesnoth/wesnoth/src/lua/lua.h:4:14: error: 'lua.h' file not found with <angled> include; use "quotes" instead
#include <lua.h>
^~~~~~~
"lua.h"
Renamed with (requires GNU sed):
$ for f in src/lua/*.h; do
> git mv "${f}" "src/lua/wrapper_${f#src/lua/}";
> done
$ git grep -El -- '#[ \t]*include[ \t]+"lua/[^"]+[.]h"' src | \
> xargs sed -Ei -- '
> s|(#[ \t]*include[ \t]+"lua/)(lua[.]h")( )?|\1wrapper_\2|;
> s|(#[ \t]*include[ \t]+"lua/)(lualib[.]h")( )?|\1wrapper_\2|;
> s|(#[ \t]*include[ \t]+"lua/)(lauxlib[.]h")( )?|\1wrapper_\2|;
> '
|
Rebased. |
|
Ping, are there any remaining issues that prevent this from being merged? I'm almost ready to add these patches to a 1.17.25 package for Debian, and before the package is uploaded I/we would like to know that the changes are likely to be accepted upstream, ideally for 1.17.26. |
|
@CelticMinstrel any concerns? |
|
I don't think so, no. |
|
@stevecotton any concerns? |
|
Mentioned a few more distributions in the changelog entry, no code changes. Let me know if I should merge in master (for comparison) and force-push another rebase (which could be compared to verify there are no other changes). But it looks like there are no conflicts with master, so I assume you'll rebase it. Reminder: please don't squash on merge. |
src/wesnoth_lua_config.md
Outdated
| @@ -4,7 +4,7 @@ This document describes the process used to install Lua 5.4.4 for Wesnoth. The e | |||
|
|
|||
| The goal of this process was, as much as possible, install clean, unchanged sources. | |||
| Traditionally, Wesnoth maintainers have made changes directly to the Lua source kit. | |||
| __This is strongly discouraged.__ | |||
| __This is no longer allowed.__ | |||
There was a problem hiding this comment.
Adding ", because it will break ..." would be good here, otherwise it's just waiting for someone to say "but I need to to do X, thus this rule is discarded".
There was a problem hiding this comment.
Good idea, thanks. Done, with other similar documentation improvements.
|
Looks reasonable to me. Currently I'm getting linker errors when trying to build it locally with the system Lua, with it apparently not finding any of the Lua functions. However, I can see it built on the CI, so I'm looking assuming there's something wrong locally. That's with CMake on Debian testing, with Clang 16, having first run I'd be happy for this to merge, even if it ends with a fixup when I work out what's failing locally. |
|
Improved documentation based on @stevecotton's review. The linker errors are odd. CI tests the system Lua with CMake and Clang on Ubuntu 22.04. I've been testing with both CMake and SCons with GCC 10 on Debian 11 bullseye. And I'll soon (within the next several hours or so hopefully) test with CMake and GCC 13 on Debian unstable (almost identical to Debian testing). |
scons/lua.py makes use of the vestigial luadir option from commit e94dcec. Like FindLua.cmake, scons/lua.py searches for the Lua headers and library, instead of using pkg-config like the old scons/lua.py (removed in commit 9929d3c) did, because even though distributions typically provide .pc files for Lua, upstream Lua doesn't. It's likely that all distributions that compile Lua as a C++ library will also provide .pc files, but this check doesn't rely on that (just as the CMake module doesn't). Unfortunately, SCons.Conftest.CheckLib() prints up to eight messages like "Checking for C++ library lua54-c++... no" until a working library name is found. Also conditionally include system Lua headers in src/lua/*.h and update documentation in src/modules/lua_README.md, src/wesnoth_lua_config.h, and src/wesnoth_lua_config.md. The two lines about "The primary commit, after replacing the sources," in src/wesnoth_lua_config.md don't make sense since the instructions were updated for submodule Lua in commit d32cfb8 and make even less sense now with preceding commits for updating CMake modules.
|
@stevecotton Amazingly, it's a bug in Debian's lua5.4 source package (in testing and unstable but not stable or oldstable). A patch to I don't think that bug should block this from being merged. No fixup is necessary (or possible) in Wesnoth. But I did just push a change for CMake to skip an unnecessary (but harmless) |
|
\o/ I've confirmed that this links against my patched src:lua5.4 packages in a Debian unstable chroot. Also when I suggested building and installing src:lua5.4 with that patch I should have recommended changing the version in the first line of |
Wesnoth no longer changes Lua source (since commit 5155a74, pull #6549, version 1.17.2), and lua5.4 was promoted to Ubuntu main where it is officially maintained since Ubuntu 23.10 (but wesnoth is still in universe where it isn't officially maintained or updated). This is an opportunity to again (as was done before 1.9.0) enable distributions to use Wesnoth with their own Lua 5.4 packages that will receive any security updates or other important bug fixes (since Lua CVEs happen, and distributions usually don't update Wesnoth and its submodule copy of Lua in their stable releases, except via optional backports or PPAs).
Wesnoth wants Lua compiled as C++ so that it can recover from C++ exceptions. Lua's build system doesn't compile as C++, but Debian and Arch do (and Fedora doesn't). Wesnoth also still wants some compile-time definitions, although the
LUA_COMPAT_*ones are from Lua 5.3 and are unused in 5.4 (so this PR removes them). So that leaves the Windowsstrcoll()redefinition and exception rethrowing, hence this PR excludes Windows.The CMake module and SCons check search for the Lua headers and library (instead of using pkg-config like the old scons/lua.py did), because even though distributions typically provide .pc files for Lua, upstream Lua doesn't. It's likely that all distributions that compile Lua as a C++ library will also provide .pc files, but these checks don't rely on that. Unfortunately, SCons is a little noisy when looking for a working include directory and library name. I'm open to suggestions on the SCons check, like using pkg-config (although I tried to instead make it consistent with the FindLua module pulled from CMake, which doesn't use pkg-config) or somehow suppressing all the "Checking for C++ library lua54-c++... no" etc. messages.
Build tested locally with both system and submodule Lua using both CMake and SCons. Please don't squash on merge.