`--override-default-config` parameter is ignored

jdrueckert commented 2 years ago

What you were trying to do

Start a headless server and override its default configuration to start Metal Renegades instead of Core Sample Gameplay.

./gradlew server --args="--override-default-config=override.cfg"

with override.cfg contents:

{
  "defaultModSelection": {
    "modules": [
      "MetalRenegades"
    ],
    "defaultGameplayModuleName": "MetalRenegades"
  },
  "worldGeneration": {
    "defaultGenerator": "MetalRenegades:dynamicWorld"
  }
}

What actually happened

The headless server started with Core Sample Gameplay

How to reproduce

Run ./gradlew server and notice that it activates CoreSampleGameplay [main] INFO o.t.e.c.m.loadProcesses.RegisterMods - Activating module: CoreSampleGameplay:4.1.1-SNAPSHOT
Stop the server
Create an override.cfg in the workspace root with the above-mentioned contents
Run ./gradlew server --args="--override-default-config=override.cfg" and notice that it still activates CoreSampleGameplay [main] INFO o.t.e.c.m.loadProcesses.RegisterMods - Activating module: CoreSampleGameplay:4.1.1-SNAPSHOT

Workaround

Didn't find a workaround yet.

Additional Notes / Context

:exclamation: This is highly impacting testability of module changes on headless server.

Note, that currently you'll need to take additional actions due to https://github.com/MovingBlocks/Terasology/issues/4956, https://github.com/MovingBlocks/Terasology/issues/4957, and https://github.com/MovingBlocks/Terasology/issues/4958

keturn commented 2 years ago

Is this specifically a problem when run from source with gradle? You've successfully used --override-default-config with release builds, right?

jdrueckert commented 2 years ago

We are using it successfully directly as a java argument for the playtest server. So yes, I think it's only occurring when using it with gradle.

keturn commented 2 years ago

My reading of the current implementation:

The CLI handler in the PC facade takes this value and sets it as the Config.PROPERTY_OVERRIDE_DEFAULT_CONFIG property.

PathManager doesn't do anything with it.

It's all in o.t.engine.config.Config, where it's used as-is (not adjusted for homeDir or anything like that).

I am surprised by the ordering here https://github.com/MovingBlocks/Terasology/blob/efe4a53be54ec68211f4a8c732f0f21d0cb46d4b/engine/src/main/java/org/terasology/engine/config/Config.java#L139-L148
as I would have expected the thing named "override" to be merged in last, not before userConfig. But that hasn't changed in five years, so presumably that's still working as expected.

I haven't seen anything yet that would explain what has changed here or why it would work when it's run from a release build but not when run from source.

jdrueckert commented 2 years ago

If this is in Config.java (which is not gradle-specific), does it work for the playtest server because that one doesn't have a user config?

I haven't seen anything yet that would explain what has changed here or why it would work when it's run from a release build but not when run from source.

To be honest, I'm not sure whether headless server was ever used for proper local testing of things...

I would have expected the thing named "override" to be merged in last, not before userConfig

Me, too! Or maybe change the name if we don't want it to be merged in last. I wonder if there are situations where we'd want userConfig to take precedence :thinking:

For headless server situations, I guess we don't expect a userConfig to be present "in production". In test scenarios it will be present, but we want it to be overridden.
For headed server situations, I guess there will be a userConfig... but we would want the override to take precedence, right?
For headed client situations, there definitely is a userConfig and we want it to take precedence I think.
For headless client situations, there probably also will be a userConfig but I'd say we would want the override to take precedence :thinking:

Does that make sense so far? If so, then the only case where the userConfig should take precedence is for headed clients. I wonder if it would make sense and work to check for this case and ignore the override in that case? :thinking: Btw, is override.cfg a file that's present in the workspace by default?

jdrueckert commented 2 years ago

Guess it does:

https://github.com/MovingBlocks/Terasology/blob/7dfebd3ef75b3dca2bc2e913040351e812a0d008/build.gradle#L233-L243

keturn commented 2 years ago

That list of conditions also adds to my feeling like maybe we need a better interface for whatever it is that we've been trying to accomplish via these override.cfg files.

jdrueckert commented 2 years ago

I feel like override.cfg should not be there by default but only added on purpose...

MovingBlocks / Terasology