libxml2 problem on Linux

[OrbWeaver]

For an as-yet unknown reason, this commit seems to break XML parsing on Linux:

#6439: Use xmlReadFile instead of xmlParseFile which has been deprecated and removed. Privatise Document() constructor accepting an xmlDocPtr.

As far as I can see, the commit is entirely correct. xmlParseFile is indeed deprecated, and the new usage of xmlReadFile matches what the libxml2 examples are suggesting. But the result is that although the xmlDoc* returned from the function is not NULL, nothing XML-related works, the entire registry system returns only empty values, and almost all of the tests are broken (because the main radiant core cannot be initialised without any registry values available). Changing back to xmlParseFile makes the problem go away but is an unsatisfactory solution because it specifically reintroduces a deprecated function call.

I am not sure whether this is a bug in the specific version of libxml2 on my Ubuntu system, or something incorrect about how we are calling xmlReadFile (i.e. perhaps it requires an encoding or a particular non-default option to correctly process our XML files). Unfortunately like many of the core GNOME C libraries, the documentation is bare-bones and explains almost nothing (like what any of the parsing options actually mean), and I cannot see an obvious way to ask libxml2 to return meaningful errors, or to query exactly what might be wrong with a constructed xmlDoc* object.

It makes me wonder if it would be better in the long term to ditch the reliance on libxml2 and instead use one of the light-weight C++ XML parsing libraries like RapidXml or pugixml instead. Not exactly a trivial change but might not be too cumbersome since the existing XML code is wrapped in our own xmlutil classes and not generally used directly by the rest of the codebase.

[OrbWeaver]

I did some debugging in unit tests.

The first problem is that although we have a basic XmlTest, it uses the full RadiantTest fixture which can only be constructed if the XML registry is working fine, so these basic XML tests are not runnable. I managed to fix that by changing the behaviour (on Linux only) to use TEST_BASE_PATH instead of _context.getTestResourcePath() to find the test resource files, so that RadiantTest is not required.

This confirmed that the basic functionality of loading XML is working perfectly fine, even with the switch to xmlReadFile(). All of the XML tests pass, and I can load one of the game files in a unit test and examine its properties. So there is nothing fundamentally wrong with the XML structure being created by the new function call.

The problem seems to be that within the Game class, any attempt to look up key values in the registry fail. Although each Game class is constructed successfully and imports its content, any searches for its own XPath root (e.g. "//game[@name='Doom 3 Demo']") return a list of 0 nodes, even though that exact XPath string can be used successfully within the basic XML test to find the <game> node which was loaded directly into an xml::Document. So there must be something going wrong with either when or how the .game file content is being merged into the global registry hierarchy.

[greebo]

Oh boy. Thanks for investigating this, reading the issue description I suspected that the XPath queries might be failing, but I thought it was rather due to the libxml2 version used in newer Ubuntus. I recall now that the automated Ubuntu build on Github is not running the unit tests (couldn't get it to run and gave up at some point) - so I missed that this is breaking stuff.

I searched around for existing code to find out what to pass for that encoding parameter in xmlReadFile, but I couldn't find anything useful, everybody seems to pass NULL as argument, which I ended up doing too.

12 hours ago, OrbWeaver said:

It makes me wonder if it would be better in the long term to ditch the reliance on libxml2 and instead use one of the light-weight C++ XML parsing libraries like RapidXml or pugixml instead

I'm all for that, I was that close of doing it when I upgraded libxml2 for Windows. As long as it's supporting XPath and XML Tree manipulations, I'd go for the most light-weight one, maybe even header-only. Our libxml2 usage is confined to xmlutil, so it should be possible to switch. edit: I'd vote for pugixml, it seems to be still alive and has XPath support. It's what I've been using in the TDM game code too.

But still, if it's the tree manipulations that break the queries, we can either try to find out what we are doing wrong here, or move away from pushing the .game data into the registry trees - it might not be necessary after all, since most (if not all) code is relying on the GameManager interface to get do the queries.

For the moment being, I can revert the change to xmlReadFile, so that my repo is functional again.

Are you still investigating this? You seem to have gotten quite far already.

[greebo]

I took the liberty to change the title of the existing issue and link it to this topic here. https://bugs.thedarkmod.com/view.php?id=6472

[OrbWeaver]

9 hours ago, greebo said:

Are you still investigating this? You seem to have gotten quite far already.

Yes, I would like to find out what the issue is even if the ultimate decision is to use a different library in future.

So far I have established that this line in Game.cpp is apparently working (no exception is thrown):

		// Import the game file into the registry
		GlobalRegistry().import(fullPath, "", Registry::treeStandard);

but this (and any subsequent attempt to getKeyValue) then fails:

		// Get the engine path
		_enginePath = getKeyValue(enginePath);

I can only think of two hypotheses:

1. It's a timing issue or race condition: the import call is returning but something in libxml2 has not "finalised" the tree so it isn't ready to do XPath queries yet. I don't think there is any asynchronous or threaded code in the libxml2 API but who knows.
2. There's something different about how XPath queries need to be formatted after merging. E.g. perhaps "//game" no longer works, and it has to have some other prefix.

Further debugging is therefore needed.

Edit: OK, there's definitely something weird going on with encodings. Adding a Registry::dump call into the Game constructor gives me output like this:

output error : string is not in UTF-8
output error : string is not in UTF-8
eCrosshairs" ="mU" ���mU="SHIFT"/>< ="ToggleGrid" ="(&#xB3;R"/>< ="ToggleView" ="U" ���mU="SHIFT+CONTROL"/>< ="NextView" ="" ���mU="CONTROL"/>< ="ZoomIn" ="Delete"/>< ="ZoomOut" ="Insert"/>< ="CenterXYViews" ="" ���mU="CONTROL+SHIFT"/>< ="CenterXYView" ="" ���mU="CONTROL+ALT"/>< ="ToggleCubicClip" ="" ���mU="SHIFT"/>< ="ToggleCamera" ="U" ���mU="SHIFT+CONTROL"/>< ="TogTexLock" ="" ���mU="SHIFT"/>< ="DragVertices" ="U"/>< ="DragEdges" =""/>< ="DragFaces" =""/>< ="ToggleSelectionFocus" ="" ���mU="CONTROL"/>< ="ThickenPatchDialog" ="" ���mU="CONTROL"/>< ="ToggleShowAllLightRadii" ="" ���mU="CONTROL+SHIFT+ALT"/>< ="ToggleClipper" ="mU"/>< ="MouseTranslate" =""/>< ="MouseRotate" =""/>< ="MouseDrag" =""/>< ="NewOrthoView" ="" ���mU="CONTROL+ALT"/>< ="SetGrid0.125" ="" ���mU=""/>< ="SetGrid0.25" ="" ���mU=""/>< ="SetGrid0.5" ="" ���mU=""/>< ="SetGrid1" ="" ���mU=""/>< ="SetGrid2" ="U" ���mU=""/>< ="SetGrid4" ="" ���mU=""/>< ="SetGrid8" ="" ���mU=""/>< ="SetGrid16" ="" ���mU=""/>< ="SetGrid32" ="" ���mU=""/>< ="SetGrid64" ="" ���mU=""/>< ="SetGrid128" ="" ���mU=""/>< ="SetGrid256" ="" ���mU=""/>< ="ToggleTextureTool" ="" ���mU="CONTROL+ALT"/>< ="ToggleMainControl_TextureTool" ="" ���mU="CONTROL+ALT+SHIFT"/>< ="NormaliseTexture" ="" ���mU=""/>< ="TexToolGridUp" ="plus" ���mU="SHIFT"/>< ="TexToolGridDown" ="minus" ���mU="SHIFT"/>< ="TexToolMergeItems" ="" ���mU="Y&#x11D;mU"/>< ="GroupCycleBackward" ="ISO_Left_Tab" ���mU="SHIFT"/>< ="GroupCycleForward" ="" ���mU=""/>< ="RevertToWorldspawn" ="" ���mU="SHIFT"/>< ="SavePosition1" ="" ���mU="CONTROL+ALT"/>< ="SavePosition10" ="(&#xB3;R" ���mU="CONTROL+ALT"/>< ="SavePosition2" ="U" ���mU="CONTROL+ALT"/>< ="SavePosition3" ="" ���mU="CONTROL+ALT"/>< ="SavePosition4" ="" ���mU="CONTROL+ALT"/>< ="SavePosition5" ="" ���mU="CONTROL+ALT"/>< ="SavePosition6" ="" ���mU="CONTROL+ALT"/>< ="SavePosition7" ="" ���mU="CONTROL+ALT"/>< ="SavePosition8" ="" ���mU="CONTROL+ALT"/>< ="SavePosition9" ="" ���mU="CONTROL+ALT"/>< ="LoadPosition1" ="" ���mU="Y&#x11D;mU"/>< ="LoadPosition10" ="(&#xB3;R" ���mU="Y&#x11D;mU"/>< ="LoadPosition2" ="U" ���mU="Y&#x11D;mU"/>< ="LoadPosition3" ="" ���mU="Y&#x11D;mU"/>< ="LoadPosition4" ="" ���mU="Y&#x11D;mU"/>< ="LoadPosition5" ="" ���mU="Y&#x11D;mU"/>< ="LoadPosition6" ="" ���mU="Y&#x11D;mU"/>< ="LoadPosition7" ="" ���mU="Y&#x11D;mU"/>< ="LoadPosition8" ="" ���mU="Y&#x11D;mU"/>< ="LoadPosition9" ="" ���mU="Y&#x11D;mU"/>< ="ToggleOrthoBackgroundPanel" ="U" ���mU="Y&#x11D;mU"/>< ="ToggleRotationPivot" ="" ���mU="CONTROL"/>< ="ToggleAasVisualisationPanel" ="" ���mU="CONTROL+SHIFT"/>< ="GroupSelected" ="" ���mU="CONTROL+SHIFT"/>< ="UngroupSelected" ="" ���mU="CONTROL+SHIFT"/></mU><>

Whereas with xmlParseFile there are no weird characters and the output just looks like properly formatted XML. I wonder if we have some nonstandard characters in our XML files for some reason.
 

[OrbWeaver]

Well I think I've gone as far as I can with this but libxml is simply not doing what it's supposed to. I can put code like this in the Game constructor:

        auto games = GlobalRegistry().findXPath("//game");
        assert(!games.empty()); // SUCCESS
        assert(games[0].getAttributeValue("name") == _name); // SUCCESS
        assert(!GlobalRegistry().findXPath("//game[@name='" + _name + "']").empty()); // FAIL

The "//game" node is there, it has an attribute "name" with value "Doom 3 Demo" (or whatever the first game to be loaded is), but passing that exact name string back to an XPath query using the [@name='blah'] syntax just doesn't work, even though it used to work fine, and should work according to the XPath specification. I even dug down into the Document::findXPath function to see if something was being set in one of the C structures to indicate what is going wrong, but even though there is a lastError struct, it is empty. If there is any indication of the problem, it is buried deep within impenetrable and poorly-documented C structures.

So, I am done with libxml and will look at C++ alternatives. 

On 1/28/2024 at 5:03 AM, greebo said:

I'd vote for pugixml, it seems to be still alive and has XPath support. It's what I've been using in the TDM game code too.

If we were going to integrate this, what is the best way of doing so? I seem to recall from previous discussions you would prefer not to have git submodules linked into the repo, and it looks like pugixml is just one .cpp and one header file, so I guess the simplest approach is just to download the files and add them directly to the build in a suitable directory (like we do with the libfmt library).

[greebo]

Thanks for doing the digging. Libxml2 is so widely used, that it's hard to imagine nobody else has this kind of trouble, so I'm still thinking it must be in the way we use it. Also, why just in Linux, it all doesn't make much sense. I agree it's not worth doing more investigations when an easier alternative is at hand.

Yes, I'd add them to the libs, and I'm probably going to set the PUGIXML_HEADER_ONLY flag, at least in Windows.

[OrbWeaver]

16 hours ago, greebo said:

Libxml2 is so widely used, that it's hard to imagine nobody else has this kind of trouble, so I'm still thinking it must be in the way we use it. Also, why just in Linux, it all doesn't make much sense.

You're right, libxml2 can't be fundamentally broken since it is such a core dependency on Linux, plus the exact same XPath queries work perfectly fine even within our own XmlTest, so it must be something specific to how XML is being used within the registry setup.

I found a couple of online sources which suggested that XPath queries might fail if the DTD doesn't validate, but we don't have any DTDs in our XML files and the XML_PARSE_DTDVALID option is not set by default in any case. Encodings are another possible culprit (especially since the problem seems to be OS-specific), but as far as I know encodings wouldn't change the parsing of characters like "[" or "@", and if the encoding was so fundamentally wrong that even regular 7-bit ASCII failed to parse, then how would "//game" ever be found? It seems that whatever the cause of the problem is, it is very well hidden within the API and would probably require building libxml2 from source and diving into it with the debugger to find out what is going wrong.

16 hours ago, greebo said:

Yes, I'd add them to the libs, and I'm probably going to set the PUGIXML_HEADER_ONLY flag, at least in Windows.

Right, that sounds like the best approach. I'll set the HEADER_ONLY flag on Linux too then for consistency, and integrate it as a header-only dependency like libfmt.