[Bug]: Mod shadows nightconfig instead of JiJing, causing potential issues with mods that JiJ a different version
lukebemish opened this issue ยท 17 comments
Mod Loader (Required)
Quilt
Minecraft Version (Required)
1.20.1
Mod Version (Required)
8.0.0
Notes (Required)
I have a mod that JiJs nightconfig. I cannot guarantee that I will always keep the same version, for the same minecraft version, as forgeconfigapiport. This leads to two issues:
- If I JiJ an older version of nightconfig than you, and you depend on new features; if the classes from my JiJed version get picked, there is an issue
- If I JiJ a newer version of nightconfig than you, and depend on new features; if the classes from your shadowed version get loaded instead of the JiJed ones, there is an issue
Is there any particuar reason you are shadowing nightconfig instead of JiJing it, or at the absolute least relocating it? If nothing else, it creates a large amount of log spam while quilt warns the user about duplicate classes being present.
latest.log (Optional)
No response
This may have been caused by the fix to #42. Is it possible to relocate it if you're going to shadow it or is it supposed to be exposed to end users?
(Notably, I still suspect the thing that made #42 hard to solve was caused by some sort of fabric loader bug, so figuring out a consistent, simple reproduction case for that bug and reporting it to fabric loader, and getting a fix in on that end, is probably the better solution in the long run, as the "intended" solution for stuff like this is JiJing, so if that isn't working something has gone wrong)
Relocating is not an option unfortunately as that breaks multi-loader projects, since Night Config classes will no longer be the same as on Forge.
I see your point and I thought about that, too, but honestly as long as this remains just a potential issue I'm fine with the current approach of shadowing the library. Night Config has only been receiving very small bugfix updates in the recent years and it doesn't look like that's going to change anytime soon.
Maybe I'll check out JiJ again for some future release and do some testing regarding the issue it was giving me in the 1.18 era, but yeah as long as there is no actual issue here I don't see the need to act right now.
If JiJ was giving you issues, that points towards a loader bug, so we should probably look into that and try to pin it down. Shadowing is... Not a long term solution, due to the log spam that ensues if nothing else.
I'm not 100% sure but it seems to break Distant Horizons 2.0.0
' config:
https://mclo.gs/jEs3DiS
This seems unrelated, the TOML writer simply doesn't support maps.
I was able to reproduce this exact issue with a simple Night Config instance just by setting up a toml config with a map value and trying to write that to a file.
Ah, alright. I haven't done great tests for this, but DH's config breaks due to some mod compat. Still not sure what mod then, it's not Cloth Config or YACL.
Sorry for the bother!
Distant Horizons dev here.
I did some digging and was able to reproduce the issue with the following:
- DistantHorizons-2.0.0-a-dev-1.20.1
- fabric-api-0.86.1+1.20.1
- ForgeConfigAPIPort-v8.0.0-1.20.1-Fabric
- LegendaryTooltips-1.20.1-fabic-1.4.4
- Iceberg-1.20.1-fabric-1.1.13
needed by LegendaryTooltips
- Prism-1.20.1-fabric-1.0.5
needed by LegendaryTooltips
- Iceberg-1.20.1-fabric-1.1.13
It appears the issue is that both ForgeConfig and DH shade electronwill.nightconfig
in an incompatible way.
Here's what I think is happening:
- ForgeConfig loads, putting its version of nightconfig into the JVM
- A user opens and closes DH's config
- DH tries to use its version of nightconfig to save the config
- Since the nightconfig versions don't match (DH's version supports
LinkedHashMap
's, it appears ForgeConfig's does not), DH fails to save its config and (intentionally) crashes the game.
I'll see if I can fix this on DH's side, but if anyone else wants to use a shaded version of electronwill.nightconfig
, this could be an issue.
It doesn't look to me like a version mismatch could be an issue here, both our mods shade v3.6.6 of Night Config.
Also I don't see how the two shades would interfere since they are placed in different domains, but not sure about that and what else besides the domains could matter.
Also could you please provide a compiled jar of your latest Fabric release so I can do some testing on my own?
Oh, we are using the same version? That's odd...
DH can use HashMap<>
's in our config if ForgeConfig (and other mods) aren't present. I'll have to do some more digging.
Here is the jar I've been using for testing:
DistantHorizons-fabric-2.0.0-a-dev-1.20.1.zip
To reproduce the issue:
- Download the mods listed in my post above.
- Launch MC Fabric 1.20.1
- Open MC's options menu
- Click on DH's config icon next to the FOV slider
- Click the "advanced" button
- The game should crash
Oh this is weird...
When I run DH by itself our HashMap<string, string>
config entry's value is automatically converted to a string in nightconfig.TableWriter
, but when I have ForgeConfig, the config entry returns the HashMap<>
object instead.
Is there some setting in nightconfig that would cause this behavior (unfortunately I wasn't the dev who set up nightconfig initially, so I'll have to do some digging to see what setup DH is doing).
Oh, well this would've saved us all a lot of effort.
NightConfig doesn't support HashMap<>
's: TheElectronWill/night-config#114 so DH has just been using NightConfig wrong.
I'll fix DH so it doesn't use HashMap<>
's (we can just (de)serialize them into JSON and have NightConfig handle that); however, its very weird that this issue only popped up when both DH and ForgeConfig were running together.
Yeah that's what I meant here haha: #45 (comment)
It's also stated here: https://github.com/TheElectronWill/night-config/wiki/Config-formats#files
Fixed in commit fc54e0f8 on our GitLab.