Page MenuHomeWildfire Games

Get XML syntax errors not only the first time that file is loaded

Authored by elexis on Jun 12 2018, 10:54 AM.



If an XML file happens to have syntax error (such as the one in credentials.xml in rP21847), then CXeromyces::ConvertFile will parse that file for the first time, throw a syntax error and then save a cached version of that file that doesn't have a syntax error to the cache.
Further loads will then go for the valid cache file, hiding the syntax error and possibly misleading authors into assuming their XML files are valid (such as in that commit).

Test Plan

Correctness: Notice that not writing a syntactically valid (but possibly semnatically wrong) cache file meets the only requirement of the claimed task.
Completeness: Judge whether this can break under some edge cases, such as packaged mods.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jun 12 2018, 10:54 AM
elexis updated this revision to Diff 6753.Jun 12 2018, 11:04 AM

Add comment.

Vulcan added a subscriber: Vulcan.Jun 12 2018, 11:04 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build:

Successful build - Chance fights ever on the side of the prudent.

Link to build:

elexis updated the Trac tickets for this revision.Jun 12 2018, 11:09 AM
bb accepted this revision.Jun 12 2018, 4:13 PM
bb added a subscriber: bb.


If it would crash for mods, the mods would receive an error in the current code, thus not caching the file wouldn't change much there.

This revision is now accepted and ready to land.Jun 12 2018, 4:13 PM
Stan added a subscriber: Stan.Jun 12 2018, 4:21 PM

What happens when archiving ?

vladislavbelov added inline comments.
176 ↗(On Diff #6753)

I think it's better to call it validSource, because of the function result is xmb.

ArchiveBuilder.cpp runs GenerateCachedXMB, which calls ConvertFile, which should the LOGERROR.
If the LOGERROR does not show up when running the archive build, that will be an important independent bug to solve.
mod bundlers already should see the error when they budnle, with this patch they will also see it more than once when running the affected code.
Since the cached file is not written anymore, the bundling script will exclude the XMB file for the invalid XML file.
That however is analogous to the other two LOGERRORs upon syntax errors.

The archive builder has an ENSURE that is triggered for the other LOGERRORs however, it's not triggered for this one.
So it seems like a return value would be useful.

It even says so: // For now, log the error and continue, in the future we might fail.

There is the same code in CXeromyces::LoadString.


Spent too much time trying to create a testcase, but it doesnt seem possible without changing the existing code,
mostly due to the static global defined in the implementation file that is not accessible.

176 ↗(On Diff #6753)

(Guess its not so ambiguous since valid refers to the process of validation, but if we want to narrow it down, I'd suggest XML, as that's even more narrow than source)

elexis added inline comments.Sep 16 2019, 11:54 PM
176 ↗(On Diff #6753)

Actually validXML is wrong, becaues the XML is valid according to XML specs, but the XML is not valid for that grammar.
And the ambiguity also exists for the validSource. validated would be wrong as well, since it also passes if there is no grammar.

elexis updated this revision to Diff 9814.Sep 16 2019, 11:56 PM

Return error code as well, i.e. doesnt load the file that is valid XML and also making the archive builder fail instead of silently accepting the file, like it does with XML files with (other) syntax errors.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

| 115| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classXMBFile{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build:

This revision was landed with ongoing or failed builds.Sep 18 2019, 1:00 AM
This revision was automatically updated to reflect the committed changes.