search

chirs241097 / jline4mcdsrv

Command history, auto completion and syntax highlighting for every fabric server.
MIT License
24 stars 4 forks source link

Add config support (via autoconfig1u), improved code style #4

Closed Fourmisain closed 3 years ago

Fourmisain commented 3 years ago

EDIT updated the description to reflect all changes.

The default TOML file looks like this:

logPattern = "%style{[%d{HH:mm:ss}]}{blue} %highlight{[%t/%level]}{FATAL=red, ERROR=red, WARN=yellow, INFO=green, DEBUG=green, TRACE=blue} %style{(%logger{1})}{cyan} %highlight{%msg%n}{FATAL=red, ERROR=red, WARN=normal, INFO=normal, DEBUG=normal, TRACE=normal}"
highlightColors = ["CYAN", "YELLOW", "GREEN", "MAGENTA", "WHITE"]

* Highlight colors are a bit weird because setting them all red still leaves the suggestions cyan weird

chirs241097 commented 3 years ago

Thank you for your patch. I like the general direction we are going here (especially the set of config you have chosen to add), however personally I would prefer not adding an external mod as dependency since this is a server-side mod. Plus Cloth is a config screen API. I don't see a point giving a server-side mod a config screen.

Either way, I will merge this before releasing for 1.17. If you'd like to update your patch to remove dependencies on external mods, please go ahead. Otherwise I'll do it myself.

Fourmisain commented 3 years ago

I would prefer not adding an external mod as dependency since this is a server-side mod

Adding a dependency should not be an issue at all! Gradle automatically downloads dependencies and the build automatically integrates the dependencies into the final jar file (under META-INF\jar\...). That means the dependency requires no further setup for both developers or end-users, you can try it out yourself!

Plus Cloth is a config screen API. I don't see a point giving a server-side mod a config screen.

This PR doesn't add Cloth or anything GUI related, autoconfig1u is a pure config library (reads/writes from the config directoy). Cloth does have direct integration support for it, so maybe that's why you got it mixed up.

chirs241097 commented 3 years ago

I'm okay with it as long as this mod keeps its zero dependencies on other mods (it should not even depend on fabric api).

Auto Config does say it requires Cloth Config 2 API as a dependency on its curseforge page. Is it just an optional dependency, or does your patch also pulls that into the final jar?

I don't really have time to work on any Minecraft related stuff right now and I'm not very familiar with Java anyway, so I'll take your word for that. Again, thanks for your awesome work!

(Also I would bump the version to 0.1.0 rather than 0.0.4 once this is merged. Adding config support is huge.)

Fourmisain commented 3 years ago

I'm okay with it as long as this mod keeps its zero dependencies on other mods (it should not even depend on fabric api).

I'm confused what by what exactly you mean with "keeping zero dependencies", do you simply mean that the final mod jar runs without having to install any other mods? If that's what you mean, then: Absolutely yes, this PR does keep zero dependencies!

If you however mean the code shouldn't depend on any other external code, then this is obviously not the case. That said, JLine in itself is a dependency and the Jansi binaries are a dependency as well, so I'm gonna assume this is not what you mean...

Btw, I run a Minecraft server of my own with a very conservative setup (24 Vanilla compatible mods) and at least 5 of them also use Auto Config, so it's really not uncommon even on the server-side.

Auto Config does say it requires Cloth Config 2 API as a dependency on its curseforge page. Is it just an optional dependency, or does your patch also pulls that into the final jar?

Oh, true, it does say that, but no, it does not pull Cloth in there (I checked), so it's an optional dependency.

chirs241097 commented 3 years ago

That's great! And obviously I meant dependency of extra mod jars, not external code (this mod already pulls classes from JLine directly into its jar). Our server only has a single mod other than this one (lithium). So I want to keep installing this mod as simple as dropping a single jar.

Fourmisain commented 3 years ago

Ah, yeah, that's what I meant by "requires no further setup for (...) end-users", I could have been clearer on that. Installing is still simply dropping the one jar file.

Fourmisain commented 3 years ago

I wasn't that fond of the Log4j2 "JLine" appender thing to set the log pattern, so I removed it again in favor of the TOML config. I also set the version to 0.1.0.

Fourmisain commented 3 years ago

No code changes (can be merged at any time), Github just didn't think my commits were mine

Fourmisain commented 3 years ago

Apparently the gradle build was broken the whole time - I only noticed because I recently cleared my gradle cache. These changes should fix the build, though don't ask me why exactly they are needed.

While at it I also updated the all the versions of Loom, Minecraft, Yarn mappings and Fabric Loader. Note that this does not necessarily means the project will only run on 1.16.5, it could/should still be backwards compatible.

chirs241097 commented 3 years ago

I'm getting the following error while trying to run the modded server. Does autoconfig1u require fabric api anyway?

 java.lang.RuntimeException: Could not execute entrypoint stage 'main' due to errors, provided by 'autoconfig1u'!
        at net.fabricmc.loader.entrypoint.minecraft.hooks.EntrypointUtils.invoke0(EntrypointUtils.java:53) ~[fabric-loader-0.11.1.jar:?]
        at net.fabricmc.loader.entrypoint.minecraft.hooks.EntrypointUtils.invoke(EntrypointUtils.java:36) ~[fabric-loader-0.11.1.jar:?]
        at net.fabricmc.loader.entrypoint.minecraft.hooks.EntrypointServer.start(EntrypointServer.java:32) ~[fabric-loader-0.11.1.jar:?]
        at net.minecraft.server.Main.main(Main.java:92) [minecraft-1.16.5-mapped-net.fabricmc.yarn-1.16.5+build.3-v2.jar:?]
        at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
        at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:?]
        at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
        at java.lang.reflect.Method.invoke(Method.java:566) ~[?:?]
        at net.fabricmc.loader.game.MinecraftGameProvider.launch(MinecraftGameProvider.java:226) [fabric-loader-0.11.1.jar:?]
        at net.fabricmc.loader.launch.knot.Knot.init(Knot.java:139) [fabric-loader-0.11.1.jar:?]
        at net.fabricmc.loader.launch.knot.KnotServer.main(KnotServer.java:27) [fabric-loader-0.11.1.jar:?]
        at net.fabricmc.devlaunchinjector.Main.main(Main.java:86) [dev-launch-injector-0.2.1+build.8.jar:?]
Caused by: java.lang.NoClassDefFoundError: net/fabricmc/fabric/api/event/EventFactory
        at me.sargunvohra.mcmods.autoconfig1u.ConfigManager.<init>(ConfigManager.java:21) ~[autoconfig1u-3.3.1.jar:?]
        at me.sargunvohra.mcmods.autoconfig1u.AutoConfig.register(AutoConfig.java:45) ~[autoconfig1u-3.3.1.jar:?]
        at me.sargunvohra.mcmods.autoconfig1u.example.ExampleInit.onInitialize(ExampleInit.java:15) ~[autoconfig1u-3.3.1.jar:?]
        at net.fabricmc.loader.entrypoint.minecraft.hooks.EntrypointUtils.invoke0(EntrypointUtils.java:50) ~[fabric-loader-0.11.1.jar:?]
        ... 11 more
Caused by: java.lang.ClassNotFoundException: net.fabricmc.fabric.api.event.EventFactory
        at jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581) ~[?:?]
        at jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) ~[?:?]
        at java.lang.ClassLoader.loadClass(ClassLoader.java:522) ~[?:?]
        at net.fabricmc.loader.launch.knot.KnotClassLoader.loadClass(KnotClassLoader.java:168) ~[fabric-loader-0.11.1.jar:?]
        at java.lang.ClassLoader.loadClass(ClassLoader.java:522) ~[?:?]
        at me.sargunvohra.mcmods.autoconfig1u.ConfigManager.<init>(ConfigManager.java:21) ~[autoconfig1u-3.3.1.jar:?]
        at me.sargunvohra.mcmods.autoconfig1u.AutoConfig.register(AutoConfig.java:45) ~[autoconfig1u-3.3.1.jar:?]
        at me.sargunvohra.mcmods.autoconfig1u.example.ExampleInit.onInitialize(ExampleInit.java:15) ~[autoconfig1u-3.3.1.jar:?]
        at net.fabricmc.loader.entrypoint.minecraft.hooks.EntrypointUtils.invoke0(EntrypointUtils.java:50) ~[fabric-loader-0.11.1.jar:?]
        ... 11 more
Fourmisain commented 3 years ago

It looks like it does! Let me see if I can fix the build again (because autoconfig should include fabric-api by itself).

Fourmisain commented 3 years ago

Hm... I just did a clean build and only put jline4mcdsrv-0.1.0.jar in my mods and I can't replicate the issue.

jline4mcdsrv-0.1.0.jar contains autoconfig1u-3.3.1.jar, which contains fabric-api-base-0.1.3+12a8474c34.jar and the server seems to boot just fine:

[main/INFO] [FabricLoader] Loading 6 mods: minecraft@1.16.5, autoconfig1u@3.3.1, jline4mcdsrv@0.1.0, java@11, fabricloader@0.11.1, fabric-api-base@0.1.3+12a8474c34

Fourmisain commented 3 years ago

No matter what I try, I still can't replicate the issue.

It wants to use net/fabricmc/fabric/api/event/EventFactory and that is indeed inside fabric-api-base-0.1.3+12a8474c34.jar, which means that the latter must be missing for you for some reason.

Can you try and change https://github.com/chirs241097/jline4mcdsrv/blob/1141845ae26e05fcb346c62dfbab721dd058d20a/build.gradle#L37-L39 to just

    modApi("me.sargunvohra.mcmods:autoconfig1u:${project.auto_config_version}")

and check if this fixes the issue for you?

If that's still a no, can you try the aggressive approach and delete the .gradle, build and even .idea folder, reimport the project and try building it again?

Fourmisain commented 3 years ago

Give this one a try, the include may have caused issues for you - I also updated the gradle version just to be sure.

If it still does not work, you have to clear your gradle caches, because there's nothing left to fix and it's 100% an issue with gradle or loom that's unfixable on my side.

By "gradle cache" I mean

edit And as I say that, it now doesn't include jars at all, at least I've something to go off now edit2 Nope, the include is exactly what inserts the jar-in-jar, so I stand by my word that it's unfixable on my side.

chirs241097 commented 3 years ago

Indeed the remapped jar is working. Maybe my workspace is messed up. Merging. I'll publish a release on curseforge later today. Really appreciate your awesome work!