Crashes related to ruleset file load order

[Delian]

Currently we're loading files in the order of... newest to oldest (last write time descending order).
Usually this means z to a, because when users extract mod zip files (or clone a repository), these files are created on the hard drive in alphabetical order (the order in which they're downloaded, or order in which they appear in the zip file, which is usually alphabetically), so the z file is created last, and thus it's loaded first because it is the newest file.

This is both confusing (a normal user would expect the order to be a-z), and problematic, because if someone manually edits one of the rul files, even if they simply resave it without any changes, it changes the order in which the files (and folders) are loaded.

Currently we're loading files in the order of... z-a (descending case-sensitive alphabetical order).

This is both confusing (a normal user would expect the order to be case-insensitive a-z), and problematic, because if someone renames one of the files/folders, it changes the order in which the files are loaded.
And currently the majority of mods crash (or have other issues) if files are loaded in the wrong order.

Why is loading files in the wrong order problematic?
1. Wrong rule values. If a Rule object is defined in two .rul files, then the Rule object properties in the last file processed get to overwrite the properties of the first one.
2. Crashes due to erroneous error checks. If a Rule object is defined in two .rul files, then loading the wrong one first could throw due to object temporarily being in an invalid state.
3. Failed script parsing. Tag names need to be defined before Rule objects are allowed to set values for those tags. So the ruleset with tag names needs to be loaded first.

The 2nd problem could be solved by moving the error check to the afterLoad() function.
The 3nd problem could be solved by loading all the rul files and processing their "extended:" nodes before processing any other yaml nodes.
The 1st problem however cannot be solved. Even if we forcefully sort the files in a specific order, there's no guarantee that the modder won't rename them and thus cause a crash. It would also be inconvenient to modders if we forced a certain sort order.

Basically, what we need is to allow modders to specify which rul files to load first. That would solve all 3 of the problems. Perhaps something in metadata.yml or vars.rul.

[Meridian]

We don't guarantee any load order.

Mods need to be written in a way that is not dependent on load order.

[Delian]

You say that, but... I've checked, and currently most the major mods (XPZ, XCF, 40k, ROS, TWoTS) have errors if files are loaded in the wrong order. The mod authors don't even know about it. Or maybe they know and they don't know how to fix it. The fact that they don't know that this is an issue is an issue in itself lol

[Solarius Scorch]

I certainly don't know about it.

Please report such issues on the XCF subforum.

[Yankes]

This could even depend on operating system and if we try load zip itself.

[Meridian]

Currently we're loading files in the order of... newest to oldest (last write time descending order).

I am not aware of this.
I created a new file (create and last write time = now) "mmm.rul" and it was loaded in the middle, before L-files and after N-files.

As far as I know, OpenXcom doesn't sort the files in any particular way and doesn't guarantee any particular load order.
The order depends on the operating system, regional settings, load mode and probably some other factors.

1. Wrong rule values. If a Rule object is defined in two .rul files, then the Rule object properties in the last file processed get to overwrite the properties of the first one.

Within one mod, the rule value should not be defined twice.
Best practice is to define each object only once.
If you must split the definition into multiple places, for whatever reason, then at least keep them distinct (i.e. don't repeat the same attributes for the same object).
Otherwise... garbage in, garbage out.

2. Crashes due to erroneous error checks. If a Rule object is defined in two .rul files, then loading the wrong one first could throw due to object temporarily being in an invalid state.

Since 2014, I haven't seen or heard even a single false positive from this check. All reports were correctly found issues.

But you are technically correct, we can move it to afterLoad.

3. Failed script parsing. Tag names need to be defined before Rule objects are allowed to set values for those tags. So the ruleset with tag names needs to be loaded first.

Either define them in each file you need them,
or use the shared definition: https://openxcom.org/forum/index.php?topic=11102.0

You say that, but... I've checked, and currently most the major mods (XPZ, XCF, 40k, ROS, TWoTS) have errors if files are loaded in the wrong order. The mod authors don't even know about it. Or maybe they know and they don't know how to fix it. The fact that they don't know that this is an issue is an issue in itself lol

I will update the ruleset wiki to explicitly mention this.

[Delian]

I am not aware of this.

CrossPlatform.cpp#L642
Oops, I was wrong. I assumed the files were sorted based on last write time, because there's an elaborate lastWriteTime reading, conversion and storing. This last write time is then used... nowhere. Huh. Why is it being read then? Stoddaaaaard!
But yeah, sorry, the sorting is actually z to a. Unless, as Yankes mentioned, we're using a ziploader. Ziploader doesn't perform any sorting (but files in zips are usually a-z). So if a mod is inside a zip file, .ruls are (usually) loaded a-z, and if files are in a plain folder, then they're loaded z-a.
Hmm, if sorting is actually z-a, then the whole issue is a lot less problematic. And I didn't need to make this thread at all...

Within one mod, the rule value should not be defined twice.
Best practice is to define each object only once.
If you must split the definition into multiple places, for whatever reason, then at least keep them distinct (i.e. don't repeat the same attributes for the same object).

I agree. That would solve such issues.
Hmm. Modders are currently allowed to merge objects, but the order is only guaranteed in the order of mod hierarchy, from top (master) to bottom. In theory, if the modders were guaranteed or allowed to specify the load order, then the "Rule object merging" feature would also be safe between the .rul files within the mod. One could argue that the specific load order is a wanted feature based on the fact that modders already depend on it hehe. A good example is how the rulesets are defined in ROSIGMA. He's got normal object definitions, and then he's got a BALANCE folder, which overwrites his normal definitons. He's able to change and experiment with stuff in the BALANCE folder without having to worry about duplicate keys and specifics of his base definitions.
In other words, while we currently don't guarantee any load order, if we did allow modders to specify it, it would be a useful feature to the modders because it would allow them to safely layer their rule definitions in more possible ways.

Since 2014, I haven't seen or heard even a single false positive from this check. All reports were correctly found issues.

I know about it because yesterday, after I optimized VFS loading to be 3x faster...

Code: [Select]

[15-12-2024_11-50-19] [ERROR] failed to load '40k ROSIGMA Submod'; mod disabled
C:/OXCE/user/mods/rosigma/Ruleset/BALANCE/plasma.rul: Weapon STR_INQ_PLASMA has clip size 0 and no ammo defined. Please use 'clipSize: -1' for unlimited ammo, or allocate a compatibleAmmo item....this check failed for me when I tried loading ROSIGMA files in the wrong (a-z) order hehe.

Either define them in each file you need them,
or use the shared definition: https://openxcom.org/forum/index.php?topic=11102.0

Uhh. So that solution is basically an awkward way of trying to solve this same problem.
Why do I say it's awkward? Because with that solution the modders have to put extended: { tagsFile: "Ruleset/tagsFile.rul" } (boilerplate) into every .rul file. A better solution would've been for modders to set to load "Ruleset/tagsFile.rul" first, which would happen one time in one place, and then never worry about it again.

[Yankes]

Uhh. So that solution is basically an awkward way of trying to solve this same problem.
Why do I say it's awkward? Because with that solution the modders have to put extended: { tagsFile: "Ruleset/tagsFile.rul" } (boilerplate) into every .rul file. A better solution would've been for modders to set to load "Ruleset/tagsFile.rul" first, which would happen one time in one place, and then never worry about it again.

I disagree, I would even ban sharing tags between files (but this would break too many mods),
explicit loading is better as every one will know what is loaded and when.
For small "codebases" implicit loads like this are fine but more bigger code base things like this become "black magic" and nobody will
know when and how its work. Image that C++ allow skipping `#include` for some headers, image fun when one include conflict with other you
explicitly load. Right now I had couple of day "fun" figure out why sometimes OXCE compile or not under Cygwin with you commit:
https://github.com/biojppm/rapidyaml/issues/482
Image solving this problem when headers are loaded implicitly.

[Delian]

You're comparing apples to oranges. C++ only builds the files/functions that are encountered through the include chain, but we are unconditionally parsing everything. Different problem, different solution. Think of it like specifying an entry point: main.cpp. You do that in C++, without needing #include main.cpp in every file.

That repidyaml issue indeed looks fun (it seems like it's only cygwin problem?) hehe, but it's off topic.

[Yankes]

You're comparing apples to oranges. C++ only builds the files/functions that are encountered through the include chain, but we are unconditionally parsing everything. Different problem, different solution. Think of it like specifying an entry point: main.cpp. You do that in C++, without needing #include main.cpp in every file.

Do you know that old C do not need includes? You could write `foo(a, b)` and compiler will figure out that you want call some function and call it directly.
This was because C all functions need to have globally unique name and as long you link `foo.o` that had this function you code will create exe.
Less boilerplate and finding what include had this function, pure profit :>

But sometime after C community start creating jokes like "if it compile its means its work", thus C11 explicitly ban this fature.

[Meridian]

This last write time is then used... nowhere. Huh. Why is it being read then? Stoddaaaaard!

Maybe for the list of saves?
They can be sorted from newest to oldest...

Hmm, if sorting is actually z-a, then the whole issue is a lot less problematic. And I didn't need to make this thread at all...

If it only were that easy.

Sorting depends on a lot of things and is completely unreliable.

1/ some systems sort case-independent (...A a B...), some case-dependent (...Z a b...)
2/ some regions have special characters at the end of the alphabet (...y z ä...), some in the middle (a ä b...)
3/ some regions have character groups even for the basic 26... e.g. in English: ...Belgium < China < Denmark..., but in Slovak: ...Honduras < China < Island... ('ch' is sorted after 'h', not after 'c')
4/ some systems sort even files beginning with numbers differently, sometimes alphabetically: 1 < 2 < 222 < 3, sometimes numerically: 1 < 2 < 3 < 222

As I said... absolutely not guaranteed.

And no, I am definitely not going to code an OpenXcom-specific file sorting.

Uhh. So that solution is basically an awkward way of trying to solve this same problem.
Why do I say it's awkward? Because with that solution the modders have to put extended: { tagsFile: "Ruleset/tagsFile.rul" } (boilerplate) into every .rul file. A better solution would've been for modders to set to load "Ruleset/tagsFile.rul" first, which would happen one time in one place, and then never worry about it again.

It's not the same problem, don't mix apples and oranges

Loading that file first would not solve it.
Tag definitions are not visible cross-file.
They are needed in each file, directly or indirectly via "shared" file.

[Delian]

Maybe for the list of saves?
They can be sorted from newest to oldest...

Nope. While it is possible to sort save files base on the last write time, that's implemented independently. I mean, even though we're calling the getFolderContents() function when retrieving the list of sav files, and the returned tuples include the last write time, the last write time is discarded. Yes, it would make sense to pass the tuple to the getSaveInfo() function because then we wouldn't need to call getDateModified() for every sav file again. But for now, I can say that the last write time, as retrieved by the getFolderContents() function (which is the function based on which we build the VFS and determines the sort order we load .rul files in), is not used anywhere.

Sorting depends on a lot of things and is completely unreliable.

I linked the part of code where we're sorting the list of retrieved filenames. In that link, I show that we're using std::sort() to sort this list. This std::sort() call uses a lambda which compares the filenames using the string > operator. The C++ 17 standard states that the comparison is done using the compare() member function. And the compare function states, quote:
"By default (with the default std::char_traits), this function is not locale-sensitive."
Tldr; You are talking about locale-aware sorting the OS does, but we ignore that because we perform std::sort() after retrieving the list of files, which is locale-independent.

Sure, I can code the OXCE-specific file sorting for you. But, do I need to do anything besides simply calling std::sort() to achieve that?

Loading that file first would not solve it.

That's literally what's happening right now. XPZ loads Yankes_Scripts.rul first, because Y is at the end of the alphabet, it's the last file in the folder, so it's loaded first. That's why it doesn't error during the loading. Because Yankes_Scripts.rul contains all the tag name definitions. These tags names are not defined anywhere else and the shared file isn't linked anywhere, yet it works.

Tag definitions are not visible cross-file.

As far as I can tell, script loading internally uses a shared list of tag names, and this list persists until the loading is finished. Meaning, once a tag is defined, the tag's value can subsequently be set by any rule object, no matter which file it's in.

[Meridian]

I linked the part of code where we're sorting the list of retrieved filenames. In that link, I show that we're using std::sort() to sort this list. This std::sort() call uses a lambda which compares the filenames using the string > operator. The C++ 17 standard states that the comparison is done using the compare() member function. And the compare function states, quote:
"By default (with the default std::char_traits), this function is not locale-sensitive."
Tldr; You are talking about locale-aware sorting the OS does, but we ignore that because we perform std::sort() after retrieving the list of files, which is locale-independent.

Sure, I can code the OXCE-specific file sorting for you. But, do I need to do anything besides simply calling std::sort() to achieve that?

OK, maybe it changed in the meantime and I have obsolete information.
I can check later.

The example I mentioned with 'CH' being between 'H' and 'I' instead of between 'B' and 'D' definitely was happening several years ago.
Solarius (Polish locale) had a crash, which I (Slovak locale) could not reproduce... at the end it turned out to be different file load order due to different locale. 100% sure.

That's literally what's happening right now. XPZ loads Yankes_Scripts.rul first, because Y is at the end of the alphabet, it's the last file in the folder, so it's loaded first. That's why it doesn't error during the loading. Because Yankes_Scripts.rul contains all the tag name definitions. These tags names are not defined anywhere else and the shared file isn't linked anywhere, yet it works.

As far as I can tell, script loading internally uses a shared list of tag names, and this list persists until the loading is finished. Meaning, once a tag is defined, the tag's value can subsequently be set by any rule object, no matter which file it's in.

OK, maybe I have again obsolete information.
(Again 100% sure it was like that before.)

[Meridian]

That's literally what's happening right now. XPZ loads Yankes_Scripts.rul first, because Y is at the end of the alphabet, it's the last file in the folder, so it's loaded first. That's why it doesn't error during the loading. Because Yankes_Scripts.rul contains all the tag name definitions. These tags names are not defined anywhere else and the shared file isn't linked anywhere, yet it works.

Btw. all tag usage in XPZ is also in `Yankes_Scripts.rul`

I have found only two usages outside of this file, and they are probably mistakes.

So XPZ does it correctly and defines tags in each file, where they are used.
File load order doesn't matter for XPZ.

[Meridian]

And here's an example of some problems resulting from different locales: https://openxcom.org/forum/index.php?topic=5146.msg83361#msg83361

Swedish flag with Spanish name;
or Italian name with a Japanese flag

(offset errors due to soldier name files sorted differently)

Not saying it's still happening, maybe it was already fixed/changed... just providing evidence that I am not making this up