▸ Magik
Synthesize a FreeCAD-wasm port review from these verified findings (JSON). Group by theme, rank by real severity, be concrete. Produce: an overall assessment of the port's health; a ranked top-actions list; an ordered viewport_plan (the single most important thing first) drawn from the viewport-path findings; a note on fork hygiene (the CRLF->LF churn in Geometry.cpp/Constraint.cpp/FileUtilities.cpp inflated the diff ~13x — real change is 1110 insertions/18 deletions); and a full markdown report.
VERIFIED FINDINGS:
[
{
"severity": "info",
"category": "ifdef-discipline",
"summary": "Desktop builds verified unaffected by the Application/Interpreter/Main bootstrap changes",
"detail": "All desktop-affecting unguarded changes reduce to no-ops or behavioral equivalents. Interpreter is up at Application.cpp:2869 (Base::Interpreter().init() inside initConfig) before both `new Application(mConfig)` sites (3019 verbose/version path, 3046 initApplication), so the new constructor guard `if (Py_IsInitialized()) setupPythonTypes();` (Application.cpp:428-430) is always true on desktop; initApplication's added `setupPythonTypes();` (3048) then hits the idempotent early-return in setupPythonTypes (461-467). LoadParameters idempotency guard (3294-3298) only has callers at 2930 (desktop, once) and 445 (wasm bootstrapEarly). Interpreter.cpp better PyStatus error message (665-674) uses public PyStatus fields, safe. MainCmd.cpp new `catch (const std::exception&)` (line ~148) is correctly ordered after `catch (const Base::Exception&)` and previously fell into catch(...) with the same exit(101), so no desktop change. MainGui.cpp `#ifndef FC_OS_WASM` correctly scopes only the Linux/BSD putenv(\"PYTHONPATH=\") clear. FCConfig.h __EMSCRIPTEN__ branch is first and inert on desktop.",
"file": "src/App/Application.cpp",
"recommendation": "No action needed for desktop; changes are disciplined.",
"dim": "core-patches",
"verdict": "CONFIRMED",
"verify_reason": "Every cited line and behavioral claim was verified in the actual files. Application.cpp: constructor guard at 428-430, setupPythonTypes static-bool idempotency at 465-469, Interpreter().init() at 2869 inside initConfig preceding both `new Application` sites (3019, 3046), initApplication's setupPythonTypes() at 3048 hitting the early-return, and LoadParameters guard at 3296-3298 with callers only at 445 (wasm bootstrapEarly) and 2930 (desktop) — all confirmed via read + grep. Interpreter.cpp:666-675 uses public PyStatus fields err_msg/func (safe). MainCmd.cpp new `catch(const std::exception&)` at 148 is correctly ordered after Base::Exception (129) and before catch(...) (153); both it and catch(...) exit(101), so the exit-code claim holds (only the message text differs, which the reviewer's exit-code framing tolerates). MainGui.cpp #ifndef FC_OS_WASM at 193-198 scopes only the Linux/BSD PYTHONPATH clear. FCConfig.h __EMSCRIPTEN__ branch is first and inert on desktop. All evidence supports the info-level conclusion that desktop builds are unaffected.",
"adjusted_severity": "info"
},
{
"severity": "low",
"category": "correctness",
"summary": "setupPythonTypes is now permanently idempotent via a process-global static, which suppresses Python-type re-registration if an Application is ever destroyed and reconstructed in the same process",
"detail": "setupPythonTypes() (src/App/Application.cpp:461-467) gates on `static bool s_pythonTypesReady` that stays true for the process lifetime. The original code ran setupPythonTypes() unconditionally in every Application constructor. The destructor path nulls _pcSysParamMngr/_pcUserParamMngr (2083-2084), so LoadParameters would re-run on a second Application, but s_pythonTypesReady would remain true and the App-module Python types would NOT be re-registered on the second instance. Single-Application-per-process is the normal desktop case, so impact is limited to embedded/test harnesses that tear down and rebuild the Application within one process.",
"file": "src/App/Application.cpp",
"recommendation": "If reconstruction within a process is a supported scenario, reset s_pythonTypesReady in the destructor, or key idempotency off _pcSingleton lifetime rather than a process-global static.",
"dim": "core-patches",
"verdict": "CONFIRMED",
"verify_reason": "Verified in src/App/Application.cpp: setupPythonTypes() (461-469) gates on a never-reset process-global `static bool s_pythonTypesReady`. Git history confirms the original constructor called setupPythonTypes() unconditionally. The teardown in Application::destruct() nulls _pcSysParamMngr/_pcUserParamMngr (2083-2084), and LoadParameters() keys its idempotency on those managers (3296), so it re-runs on a rebuilt Application while setupPythonTypes() does not — the exact asymmetry claimed, meaning App-module Python types would not be re-registered on a second Application in the same process. The only nit is that the nulling lives in the static destruct() method (which also finalizes the interpreter) rather than ~Application(); the reviewer's 'destructor path' wording is a reasonable colloquialism and its own scoping to embedded/test-harness reconstruction is correct. Low severity is appropriate: single-Application-per-process is the normal case and this is a latent edge-case bug.",
"adjusted_severity": "low"
},
{
"severity": "low",
"category": "wasm-correctness",
"summary": "FcWasmProcess redeclares the readyRead() signal (and hides non-virtual close()) already provided by QIODevice",
"detail": "src/Gui/FcWasmProcess.h:100 declares `void readyRead();` under Q_SIGNALS, but the QIODevice base already declares readyRead() as a signal. A Q_OBJECT subclass re-declaring a base signal yields a duplicate metaobject entry and can make `connect(proc, &QProcess::readyRead, ...)` ambiguous or bind to the wrong one. Similarly `void close()` (line 89) is a non-virtual method hiding QIODevice::close(). wasm-only (guarded by __EMSCRIPTEN__), so no de …