Ahtenus / minecraft-init

Init script for minecraft and bukkit servers
404 stars 125 forks source link

Add spigot support, add dynamic path to server logs, correct a variable #159

Closed alfonsojon closed 10 years ago

alfonsojon commented 10 years ago

I added spigot support to the server start sequence and the updater. I also merged in a change to allow a dynamic path for server log files (https://github.com/Ahtenus/minecraft-init/pull/157) and fixed some variables.

Also, the -XX:MaxPermSize=256M is there because Bukkit and Spigot tend to crash if that is not present.

jgehrcke commented 10 years ago

This pull request is too large. There are various dubious changes. It should not be accepted in the current form. Please, properly clean this up and document, for each individual change, how current behavior is not broken.

alfonsojon commented 10 years ago

I will make a separate branch for just spigot support, sorry about that.

alfonsojon commented 10 years ago

It makes sense because if the latest.log file exists, then it can be safely assumed that the server.log file is not used anymore.

jgehrcke commented 10 years ago

I am sure you are right, but out of curiosity: why is that safe? under which circumstances is the $MCPATH/logs/latest.log used?

So, if it is used, what does it imply? How is log rolling performed then (by which component)?

alfonsojon commented 10 years ago

It will be used if the file exists; otherwise it will use server.log

alfonsojon commented 10 years ago

Log rolling will be disabled as newer minecraft server softwares automatically compresses old logs. I should add support for moving the logs to the backup directory, too.

jgehrcke commented 10 years ago

It will be used if the file exists; otherwise it will use server.log: What is "it"? What do you mean by "used"?

In your current code you print "Logs are already rolled to \"$MCPATH/logs\"." when $MCPATH/logs/latest.log does exist. I do not understand why the one implicates the other.

Can you be more specific? These are important questions:

alfonsojon commented 10 years ago

If the user is using server software which uses the logs folder with latest.log, it will not roll the logs because the server does it already.

alfonsojon commented 10 years ago

Bukkit and Spigot both petform log rolling as of their respective stable builds, making the minecraft-init log roller redundent.

alfonsojon commented 10 years ago

However, other users may not be using the latest version and still use the traditional server.log file, so the functionality should not be removed, but rather used in that certain condition.

jgehrcke commented 10 years ago

I agree with your last statement, that both should be possible. If you write that code it would be great if you added a comment containing a reference to the corresponding Bukkit/Spigot log rollig feature. I am actually using Spigot and did not know that it has built-in log rolling support. This was our "misunderstanding", but you should have said/commented on that in your code already ;-), otherwise the $MCPATH/logs/latest.log-based distinction looks like a dirty hack.

alfonsojon commented 10 years ago

Will add comments when I get home or get a chance to pull out my laptop, thanks for the feedback :)

jgehrcke commented 10 years ago

I am not able to find official reference for the feature you are referring to, at least not within 5 minutes. What I have found is a Bukkit feature request:

https://bukkit.atlassian.net/browse/BUKKIT-1802

where this feature has been rejected. There also is an outdated plugin: http://dev.bukkit.org/bukkit-plugins/roll/

Another strong argument is http://www.spigotmc.org/wiki/spigot-configuration-spigot-yml/ -- I can not see an officially documented log rolling option there.

alfonsojon commented 10 years ago

Turns out, this is a general Minecraft 1.7 feature. It's part of both Vanilla, Bukkit, and Spigot.

alfonsojon commented 10 years ago

Which explains why there is nothing about it in the changelogs.