Comment 1 Bugzilla Automation 2014-10-12 06:40:25 UTC

Maintainer CC'd

Comment 2 John Marino 2014-10-12 11:32:02 UTC

Helge: If you can provide patches for the maintainer to evaluate, that's the best (most likely to succeed) approach.

Created attachment 148220 [details]
Patch fixing the "precious config files lost" issue.

Created attachment 148221 [details]
patch fixing the "version is no longer 1.7.10" issue

The other 2 issues need some more work. But the "daemon mode" stuff can mostly be copied verbatim from the previous version?

Comment 6 John Marino 2014-10-14 17:24:34 UTC

In general all the patches should be concatenated to one file.

I don't understand this:

.for file in ${CONFIG_FILES}
	${TOUCH} ${STAGEDIR}${ETCDIR}/${file}.sample
	${LN} -s ${ETCDIR}/${file} ${STAGEDIR}${DATADIR}/${file}
.endfor

What is this doing?

1) it looks like blank sample files are being created
2) It looks like the next line was intended to be
${LN} -s ${ETCDIR}/${file} ${STAGEDIR}${DATADIR}/${file}.sample

and even if it was, that's wrong.  @sample keyword would take care of it.  What's the intent here?

Can you fix the other issues?  All we want is a patch (and not have to create one ourselves)

(In reply to John Marino from comment #6)
> I don't understand this:
> 
> .for file in ${CONFIG_FILES}
> 	${TOUCH} ${STAGEDIR}${ETCDIR}/${file}.sample
> 	${LN} -s ${ETCDIR}/${file} ${STAGEDIR}${DATADIR}/${file}
> .endfor
> 
> What is this doing?
> 
> 1) it looks like blank sample files are being created

Exactly. For some reasons the maintainer decided to install blank configuration files instead of meaningful content with the 1.8 port revision. At least, for server.properties it would really be sensible to install some pre-configured content. This was the case for the previous port revision. No idea why it was changed.

Yes, these blank config files are ugly, however I felt it would be more sensible to make use of existing pkg logic rather than bloating Makefile with in-line scripts.

> 2) It looks like the next line was intended to be
> ${LN} -s ${ETCDIR}/${file} ${STAGEDIR}${DATADIR}/${file}.sample

Definitely not: The location of the configuration files is %%ETCDIR%%, i.e. /usr/local/etc/minecraft-server. In *this* location both the .sample file and the user-modified "real" configuration file are located. However, the minecraft executable expects its config files in %%DATADIR%%, i.e. /usr/local/share/minecraft-server, which is why symlinks are created there pointing to real file location (without .sample suffix). This is intentional.

> and even if it was, that's wrong.  @sample keyword would take care of it.

No, it doesn't. Tried and tested here (very recent 9-STABLE with pkg 1.3.8).

> Can you fix the other issues?  All we want is a patch (and not have to
> create one ourselves)

If I would want to be the port maintainer, I would volunteer. It did not. I am just a bug reporter.

Comment 8 John Marino 2014-10-14 18:27:35 UTC

(In reply to Helge Oldach from comment #7)
> Exactly. For some reasons the maintainer decided to install blank
> configuration files instead of meaningful content with the 1.8 port
> revision. At least, for server.properties it would really be sensible to
> install some pre-configured content. This was the case for the previous port
> revision. No idea why it was changed.

Install blank sample files seems absurd to be, let's pull this out completely.


> Yes, these blank config files are ugly, however I felt it would be more
> sensible to make use of existing pkg logic rather than bloating Makefile
> with in-line scripts.


I'm not following.  Just remove the files, right?


> 
> > 2) It looks like the next line was intended to be
> > ${LN} -s ${ETCDIR}/${file} ${STAGEDIR}${DATADIR}/${file}.sample
> 
> Definitely not: The location of the configuration files is %%ETCDIR%%, i.e.
> /usr/local/etc/minecraft-server. In *this* location both the .sample file
> and the user-modified "real" configuration file are located. However, the
> minecraft executable expects its config files in %%DATADIR%%, i.e.
> /usr/local/share/minecraft-server, which is why symlinks are created there
> pointing to real file location (without .sample suffix). This is intentional.


I think I know why the blank files are there then.
Minecraft must crash if the symlink doesn't point to a real file I'm guessing.
I still don't like blank sample files.


> If I would want to be the port maintainer, I would volunteer. It did not. I
> am just a bug reporter.


This port was just assigned maintainership and it does not bode well that he's been silent on his first PR.
So unless we completely fix the port we should just mark it broken, right?  No sense in only fixing half of it...

I do apologise for my lack of input thus-far; I have been reading the messages. Unfortunately I've been too busy lately to give the issue a proper review. I will now endeavour to review the proposals and produce a suitable patch within 24 hours.

(In reply to John Marino from comment #8)
> Minecraft must crash if the symlink doesn't point to a real file I'm
> guessing.
> I still don't like blank sample files.

Me neither, and I think I clearly stated that. However, given the existing framework of this port, maintaining blank files while fixing a problem was the least intrusive approach.

> So unless we completely fix the port we should just mark it broken, right? 
> No sense in only fixing half of it...

I don't care. As you can see from the names of the files diff'ed I do have the patches supplied in my local tree. For me they fix the most dangling issues and make the port useful again.

Created attachment 148309 [details]
diff based on proposed changes

>The bin/minecraft-server script refers to version 1.7.10 but the port installs > version 1.8.

I do apologise for that, I must have only modified my local copy without modifying the port when I tested the upgrade to 1.8. This is now fixed.

> minecraft-server does no longer work in "daemon mode" due to the removal of
> the sysutils/tmux logic. This is a rather unfortunate feature removal. Please > add it again.

 This is actually by design. This decision was modelled around the
games/xonotic port. The main reasons for this are:
- Simplified port with fewer dependencies
- Gives end users the flexibility to use whichever method of running in the background (screen, tmux, nohup, etc.)

If further demand is shown for a return to the pre-written daemon using tmux, I will likely work on it’s return.

>Install ignores the existing contents of configuration files of a previous >1.4.6 installation located in /usr/local/etc/minecraft/. Since the config file >syntax has changed from plain text to JSON (except for server.properties) it >might seem sensible to convert automatically. As a minimum, users should be >advised to make the necessary adjustments from old to new version.

Being that 1.4.6 was released in 2012, and there have been 16 updates since (over half of which have required client updates) I had assumed that there were few remaining users still using this port. However, it is a reasonable request to handle this situation more elegantly. I will look into a way a couple of possibilities for this particular issue:
Provide a notice to the user that their existing configuration will be deleted, and should be backed up
Rename the existing configs to something like [name].old and inform the user to make any relevant modifications to the new configs (many options have changed / been added since 1.4 so a straight-up conversion would be inadvisable).

>Exactly. For some reasons the maintainer decided to install blank >configuration files instead of meaningful content with the 1.8 port revision. >At least, for server.properties it would really be sensible to install some >pre-configured content. This was the case for the previous port revision. No >idea why it was changed.

The files had to be manually created so that the port could create the symlinks to /usr/local/share/minecraft-server. When you run the server for the first time it will populate the files with a default configuration (note: it will not be fully populated until you accept the EULA). I opted for this rather than manually populating them with a default configuration for a couple of reasons:
It keeps the port size and complexity to a minimum
When Mojang updates the default configuration (with additional / removed options, or a new syntax), it will not require additional work to modify the port.

If you can propose a good argument against this methodology, I will be more than happy to review this.

>Deinstall removes /usr/local/etc/minecraft-server/* but this directory >contains precious configuration data (such as server configuration, whitelist, >etc.). Please modify pkg-plist to keep these files upon deinstall and add an >appropriate note to pkg-install to remove it when really not longer needed >(similar to /var/db/minecraft-server etc).

I have tested this, and as long as you’ve run the server at least once (to generate the configs), the files are kept because they don’t meet the original SHA256 checksums of the configs. As long as the files aren’t blank this should not be a problem.

-----------------------

I have attached a patch which fixes the following problems:
- /usr/local/bin/minecraft-server now correctly points to the correct file name
- added %%DATADIR%% and %%ETCDIR%% to pkg-plist so that both folders are now owned by mcserver. This corrects an issue with EULA generation, as well as fixing a potential issue with configuration files being removed when the port is uninstalled.
- added a message notifying users that if they're upgrading from version < 1.8 to populate the new config files, and then merge any desired changes.

Created attachment 148310 [details]
diff based on proposed changes

I have updated the diff to include the modification to pkg-deinstall.

Comment 13 John Marino 2014-10-14 20:21:53 UTC

(In reply to Jonathan Price from comment #11)
> The files had to be manually created so that the port could create the
> symlinks to /usr/local/share/minecraft-server. 

You can create symlinks that don't point to existing files.  Ports / pkg should not complain about that.  Did you try?

(In reply to John Marino from comment #13)
> (In reply to Jonathan Price from comment #11)
> > The files had to be manually created so that the port could create the
> > symlinks to /usr/local/share/minecraft-server. 
> 
> You can create symlinks that don't point to existing files.  Ports / pkg
> should not complain about that.  Did you try?

I had always assumed that you symlinks needed a pre-existing inode to link to, and apparently had never questioned this assumption. I have just tested your theory, and indeed it works, which allows me to remove the blank configuration files.

Thanks! this is a much cleaner solution, and also inadvertently solves the issue of configuration files being deleted / overwritten, I shall attach a new patch to invalidate the old one.

Created attachment 148311 [details]
See comment #14 for details

See comment #14 for details

Comment 16 John Marino 2014-10-15 08:18:52 UTC

(In reply to Jonathan Price from comment #11)
> Being that 1.4.6 was released in 2012, and there have been 16 updates since
> (over half of which have required client updates) I had assumed that there
> were few remaining users still using this port. However, it is a reasonable
> request to handle this situation more elegantly. 


Where your logic falters a bit is that none of the 16 updates occurred in ports.  Anybody updating this port will have the 1.4.6 config file, right?

(In reply to John Marino from comment #16)
> (In reply to Jonathan Price from comment #11)
> > Being that 1.4.6 was released in 2012, and there have been 16 updates since
> > (over half of which have required client updates) I had assumed that there
> > were few remaining users still using this port. However, it is a reasonable
> > request to handle this situation more elegantly. 
> 
> 
> Where your logic falters a bit is that none of the 16 updates occurred in
> ports.  Anybody updating this port will have the 1.4.6 config file, right?

Yes, this is correct. However, being that until recently it was impossible to download older versions of the client (from the website at least), I assumed that people would have largely stopped using the port, and were instead downloading it manually, unless they had the forethought to backup their client each time a new update was released.

I just performed a test installation of 1.4.6, then upgrading to 1.8. The original config folder was /usr/local/etc/minecraft, the new config folder is /usr/local/etc/minecraft-server. Any modified files in the original install are preserved in /usr/local/etc/minecraft. I have now modified the patch to mention this when the port is installed.

Created attachment 148333 [details]
see comment #17

Created attachment 148334 [details]
Correct file

The previous file was uploaded erroneously (was still the previous version). This now contains the fix previously mentioned.

Comment 20 commit-hook 2014-10-16 08:07:07 UTC

A commit references this bug:

Author: marino
Date: Thu Oct 16 08:06:33 UTC 2014
New revision: 370977
URL: https://svnweb.freebsd.org/changeset/ports/370977

Log:
  games/minecraft-server: fix issues that resulted from recent upgrade

    * Fix version reference in server script
    * Remove blank sample files
    * Add note in pkg-message how to upgrade from version 1.4.6
    * Note in pkg-deinstall files files might exist in $ETCDIR as well
      (even though pkg also mentions this)

  PR:		194312
  Reported by:	Helge Oldach
  Final fix by:	maintainer (Jonathan Price)

Changes:
  head/games/minecraft-server/Makefile
  head/games/minecraft-server/files/minecraft-server
  head/games/minecraft-server/files/pkg-deinstall.in
  head/games/minecraft-server/files/pkg-message.in
  head/games/minecraft-server/pkg-plist

Comment 21 John Marino 2014-10-16 08:10:27 UTC

Okay, the only problem I saw was the @dir %%DATADIR%% line.  That's not necessary since %%DATADIR%% was already created by other files.

pkg also reminds to remove files in ETCDIR upon deinstallation, but I didn't see any harm with saying it twice.

As a side note, the patch just would not apply with "svn patch".  I finally just used regular old "patch" but got a reject hunk on Makefile.  I'm not sure how the diff was created but it was very problematic.

Thanks for providing a good fix for these issues!