From ab81f0e60bc4cbc16c9326edee1bb2bf5f2caf02 Mon Sep 17 00:00:00 2001 From: Kelsi Date: Sat, 1 Aug 2026 13:34:52 -0700 Subject: [PATCH] fix: report why each FrameXML file failed, and imply the API fallback Sixty-seven failures arrived with their reasons scattered across thousands of log lines, and one broken script takes down every file referencing it, so what matters is seeing them together. Collect a reason per file and print them as one block; carry the real cause up through includes and referenced scripts rather than naming the file that referenced them. WOWEE_LOAD_FRAMEXML now implies WOWEE_LUA_API_FALLBACK. FrameXML cannot get through its own load without it, so two switches where one is useless alone was only a way to be handed a wall of failures for setting the obvious one. The compile checker counted a file it could not parse as neither pass nor fail, reporting a clean run over files that never loaded. --- include/addons/addon_manager.hpp | 3 +++ include/addons/lua_engine.hpp | 6 +++++ src/addons/addon_manager.cpp | 42 +++++++++++++++++++++++++++++--- src/addons/lua_engine.cpp | 14 +++++++++-- tools/framexml_compile_check.cpp | 15 +++++++++--- 5 files changed, 71 insertions(+), 9 deletions(-) diff --git a/include/addons/addon_manager.hpp b/include/addons/addon_manager.hpp index 05263030..52884ecf 100644 --- a/include/addons/addon_manager.hpp +++ b/include/addons/addon_manager.hpp @@ -63,6 +63,9 @@ private: // addonName -> enabled. Absent means enabled (default on). std::unordered_map addonEnabled_; std::string frameXmlDir_; + /// Why the last loadXmlFile returned false, so a caller loading many files + /// can report the reasons together instead of leaving them scattered. + std::string lastXmlError_; bool addonsLoaded_ = false; static std::string enabledStatePath(); void loadEnabledState(); diff --git a/include/addons/lua_engine.hpp b/include/addons/lua_engine.hpp index 41b6f861..541109d7 100644 --- a/include/addons/lua_engine.hpp +++ b/include/addons/lua_engine.hpp @@ -28,6 +28,11 @@ public: bool executeFile(const std::string& path); bool executeString(const std::string& code); + /// Error from the last executeFile/executeString that returned false. + /// Lets a caller loading many files report them together rather than + /// leaving the reasons scattered through the log. + const std::string& lastError() const { return lastError_; } + void setGameHandler(game::GameHandler* handler); void setLuaServices(const LuaServices& services); @@ -70,6 +75,7 @@ private: game::GameHandler* gameHandler_ = nullptr; LuaServices luaServices_; LuaErrorCallback luaErrorCallback_; + std::string lastError_; void callFrameScript(uint32_t wid, const char* script, const char* arg = nullptr); diff --git a/src/addons/addon_manager.cpp b/src/addons/addon_manager.cpp index 00310238..cbdb4ec0 100644 --- a/src/addons/addon_manager.cpp +++ b/src/addons/addon_manager.cpp @@ -10,6 +10,8 @@ #include #include #include +#include +#include namespace fs = std::filesystem; @@ -269,6 +271,11 @@ bool AddonManager::loadFrameXml(const std::string& frameXmlDir) { toc->files.size(), " files from ", resolvedDir); int lua = 0, xml = 0, failed = 0; + // Kept and printed together at the end. Spread through the log these are + // unreadable: the reasons land among thousands of other lines, and one + // broken script takes down every file that references it, so what matters + // is seeing them side by side and spotting the cause they share. + std::vector> failures; for (const auto& filename : toc->files) { std::string lower = filename; for (char& c : lower) c = static_cast(std::tolower(static_cast(c))); @@ -276,6 +283,7 @@ bool AddonManager::loadFrameXml(const std::string& frameXmlDir) { if (resolved.empty()) { LOG_WARNING("FrameXML: ", filename, " is listed but not on disk"); ++failed; + failures.emplace_back(filename, "listed in the manifest but not on disk"); continue; } const std::string full = resolved.string(); @@ -285,14 +293,29 @@ bool AddonManager::loadFrameXml(const std::string& frameXmlDir) { // inherit from them. Following it is most of what makes this possible // at all. if (lower.size() >= 4 && lower.compare(lower.size() - 4, 4, ".lua") == 0) { - if (luaEngine_.executeFile(full)) ++lua; else { ++failed; - LOG_ERROR("FrameXML: ", filename, " failed"); } + if (luaEngine_.executeFile(full)) { + ++lua; + } else { + ++failed; + failures.emplace_back(filename, luaEngine_.lastError()); + } } else if (lower.size() >= 4 && lower.compare(lower.size() - 4, 4, ".xml") == 0) { - if (loadXmlFile(full, 0)) ++xml; else ++failed; + lastXmlError_.clear(); + if (loadXmlFile(full, 0)) { + ++xml; + } else { + ++failed; + failures.emplace_back(filename, lastXmlError_.empty() + ? "(no reason recorded)" + : lastXmlError_); + } } } LOG_WARNING("FrameXML: ", lua, " Lua files and ", xml, " XML files loaded, ", failed, " failed"); + for (const auto& [file, why] : failures) { + LOG_WARNING("FrameXML: ", file, " — ", why); + } return failed == 0; } @@ -301,12 +324,14 @@ bool AddonManager::loadXmlFile(const std::string& path, int depth) { // until the stack gives out. constexpr int kMaxDepth = 16; if (depth > kMaxDepth) { + lastXmlError_ = "include nesting too deep"; LOG_ERROR("AddonManager: include nesting too deep at ", path); return false; } std::ifstream in(path, std::ios::binary); if (!in) { + lastXmlError_ = "not on disk"; LOG_WARNING("AddonManager: XML not found: ", path); return false; } @@ -316,6 +341,7 @@ bool AddonManager::loadXmlFile(const std::string& path, int depth) { ui::XmlNode root; std::string error; if (!ui::parseXml(buffer.str(), root, error)) { + lastXmlError_ = "XML parse: " + error; LOG_ERROR("AddonManager: ", path, ": ", error); return false; } @@ -340,17 +366,25 @@ bool AddonManager::loadXmlFile(const std::string& path, int depth) { // Order matters and is not the order the emitter reports things in. Includes // carry the templates a file inherits from, and scripts define the functions // its handlers name, so both have to be in place before any frame is built. + // A file is only as loadable as what it pulls in, so the reason kept here is + // the first real one — the include or script that actually broke — rather + // than the name of whichever file happened to reference it. for (const auto& inc : emitted.includeFiles) { - if (!loadXmlFile(sibling(inc).string(), depth + 1)) ok = false; + if (!loadXmlFile(sibling(inc).string(), depth + 1)) { + if (ok) lastXmlError_ = "include " + inc + ": " + lastXmlError_; + ok = false; + } } for (const auto& script : emitted.scriptFiles) { if (!luaEngine_.executeFile(sibling(script).string())) { + if (ok) lastXmlError_ = "script " + script + ": " + luaEngine_.lastError(); LOG_ERROR("AddonManager: ", path, " referenced ", script, " which failed"); ok = false; } } if (!emitted.lua.empty()) { if (!luaEngine_.executeString(emitted.lua)) { + if (ok) lastXmlError_ = "frames: " + luaEngine_.lastError(); LOG_ERROR("AddonManager: frames from ", path, " failed to build"); ok = false; } else { diff --git a/src/addons/lua_engine.cpp b/src/addons/lua_engine.cpp index 21db6a4c..3ca50837 100644 --- a/src/addons/lua_engine.cpp +++ b/src/addons/lua_engine.cpp @@ -2096,8 +2096,16 @@ void LuaEngine::installMissingApiFallback() { // newer client. That is the right trade for bringing FrameXML up, where the // point is to get past a missing name and find out what actually matters, // and the wrong one for everyday addon loading. - const char* env = std::getenv("WOWEE_LUA_API_FALLBACK"); - const bool enabled = env && *env && std::string(env) != "0"; + auto isSet = [](const char* name) { + const char* v = std::getenv(name); + return v && *v && std::string(v) != "0"; + }; + // Loading FrameXML implies it. FrameXML cannot get through its own load + // without the fallback, so two separate switches where one is useless + // without the other is only a way to be handed a wall of failures for + // setting the obvious one. + const bool enabled = isSet("WOWEE_LUA_API_FALLBACK") || + isSet("WOWEE_LOAD_FRAMEXML"); if (!enabled) return; lua_pushcfunction(L_, lua_RecordMissingApi); @@ -2459,6 +2467,7 @@ bool LuaEngine::executeFile(const std::string& path) { if (err != 0) { const char* errMsg = lua_tostring(L_, -1); std::string msg = errMsg ? errMsg : "(unknown error)"; + lastError_ = msg; LOG_ERROR("LuaEngine: error loading '", path, "': ", msg); if (luaErrorCallback_) luaErrorCallback_(msg); if (gameHandler_) { @@ -2481,6 +2490,7 @@ bool LuaEngine::executeString(const std::string& code) { if (err != 0) { const char* errMsg = lua_tostring(L_, -1); std::string msg = errMsg ? errMsg : "(unknown error)"; + lastError_ = msg; LOG_ERROR("LuaEngine: script error: ", msg); if (luaErrorCallback_) luaErrorCallback_(msg); if (gameHandler_) { diff --git a/tools/framexml_compile_check.cpp b/tools/framexml_compile_check.cpp index f480d4f1..607f2fd1 100644 --- a/tools/framexml_compile_check.cpp +++ b/tools/framexml_compile_check.cpp @@ -25,12 +25,20 @@ extern "C" { #include int main(int argc, char** argv) { lua_State* L = luaL_newstate(); - int ok = 0, bad = 0, shown = 0; + int ok = 0, bad = 0, shown = 0, unparsed = 0; for (auto& e : std::filesystem::directory_iterator(argv[1])) { if (e.path().extension() != ".xml") continue; std::ifstream f(e.path()); std::stringstream ss; ss << f.rdbuf(); wowee::ui::XmlNode root; std::string err; - if (!wowee::ui::parseXml(ss.str(), root, err)) continue; + // Counted, not skipped. A file the reader cannot get through never + // reaches the compiler at all, so passing over it quietly reports a + // clean run on files that in fact never loaded. + if (!wowee::ui::parseXml(ss.str(), root, err)) { + ++unparsed; + printf(" UNPARSED %-30s %s\n", e.path().filename().string().c_str(), + err.c_str()); + continue; + } auto r = wowee::ui::emitFrameXml(root); if (r.lua.empty()) { ++ok; continue; } std::string chunk = "local __WoweeTemplates={} " @@ -44,6 +52,7 @@ int main(int argc, char** argv) { } lua_settop(L, 0); } - printf("emitted Lua compiles: %d fails: %d\n", ok, bad); + printf("emitted Lua compiles: %d fails: %d unparsed XML: %d\n", + ok, bad, unparsed); return 0; }