Support Spigot-style and vanilla-style world zips, for any server type #1305

wlritchi · 2022-01-25T23:42:08Z

Spigot-family servers put the Nether and End dimensions into separate world directories. Previously, the start-setupWorld script could only accept this layout when passing it to a Spigot-family jar.

This PR adjusts the approach for detecting the Spigot world layout, so that it can be applied when loading any server jar. It also adjusts the logic for world copying so that the extra dimensions will be loaded into the correct place. If the script finds both a Spigot-style and a vanilla-style Nether (or End), the Spigot-style one will take precedence. This matches the default behaviour of Spigot-family jars.

Unit tests added
Manually tested on real worlds

Fixes #1290

Spigot-family servers put the nether and end dimensions into separate world directories. Previously, the start-setupWorld script only had support for this layout when loading a Spigot-family jar. This commit adjusts the approach for detecting the Spigot layout, so that even when loading a vanilla jar, the correct world can be identified. Note that this commit does *not* fix actually loading Spigot nether and end dimensions on vanilla jars; it merely improves the detection for the main world directory.

shotah

Excited to see all the tests! Only real question I have is about the Paper vs Spigot.

shotah · 2022-01-31T16:42:33Z

tests/setuponlytests/vanilla_world_for_spigot_server/docker-compose.yml

+      EULA: "TRUE"
+      SETUP_ONLY: "TRUE"
+      VERSION: ${MINECRAFT_VERSION:-LATEST}
+      TYPE: "PAPER"


Noticed in the folder name you called out spigot, but see you are using Paper. Probably just a typo or me not knowing enough about what is going on. :)

I'm mostly protecting against the possibility of a future refactor accidentally making the Spigot logic conditional on TYPE (i.e. apply only to Spigot itself) instead of FAMILY (apply to all Spigot-style jars). It felt like overkill to write a whole separate test for it, but might as well catch it here.

shotah · 2022-01-31T16:43:57Z

tests/setuponlytests/spigot_world_for_spigot_server/docker-compose.yml

+      EULA: "TRUE"
+      SETUP_ONLY: "TRUE"
+      VERSION: ${MINECRAFT_VERSION:-LATEST}
+      TYPE: "PAPER"


I need to know more of why we are using paper instead of spigot when that is declared in the folder name.

Spigot is effectively a dead release stream; however, it lives on as a "family" designation where Paper is a member of that family.

shotah · 2022-01-31T16:44:24Z

tests/setuponlytests/conflicting_world_for_spigot_server/verify.sh

@@ -0,0 +1,5 @@
+mc-image-helper assert fileExists world/level.dat && \


Excited to see these, will need to go back and add for forge api and vanilla tweaks.

Agreed, I'm so happy to see others already using the new assertions I just added

shotah · 2022-01-31T16:48:10Z

scripts/start-setupWorld

-      [ -d "${baseDir}_the_end" ] && rsync --remove-source-files --recursive --delete "${baseDir}_the_end/" "${worldDest}_the_end"
+      if [ -d "${baseDir}_nether" ]; then
+        log "Copying Spigot Nether..."
+        rsync --remove-source-files --recursive --delete "${baseDir}_nether/" "${worldDest}_nether"


Looks great, curious if we couldn't make all these rsync's a single method that takes in "${baseDir}_nether" and "${worldDest}_nether". It really may clean up these lines 82 - 107. Either way this is great stuff and it doesn't need to happen, just thought it may make it easier to read and troubleshoot.

Just an FYI, I am gradually converting rsync usage over to sync subcommand of mc-image-helper, which takes a subset of the arguments here

https://github.com/itzg/mc-image-helper#sync-and-interpolate

itzg

Changes look great. I'll wait to merge until I hear back if there's any final tweaks you want to make based on review.

itzg · 2022-01-31T17:56:20Z

scripts/start-setupWorld

+        rm -r "$baseDir/DIM-1"
+      fi
+    fi
+    if [ -d "${baseDir}_the_end/DIM1" ]; then


Got love their consistency 😊

itzg · 2022-01-31T18:07:45Z

tests/setuponlytests/conflicting_world_for_spigot_server/verify.sh

@@ -0,0 +1,5 @@
+mc-image-helper assert fileExists world/level.dat && \


Agreed, I'm so happy to see others already using the new assertions I just added

itzg · 2022-01-31T18:08:48Z

tests/setuponlytests/conflicting_world_for_spigot_server/verify.sh

+mc-image-helper assert fileExists world/level.dat && \
+mc-image-helper assert fileExists world_nether/DIM-1/some_spigot_nether_file && \
+mc-image-helper assert fileExists world_the_end/DIM1/some_spigot_end_file && \
+! mc-image-helper assert fileExists world_nether/DIM-1/some_vanilla_nether_file && \


Nice! I don't even need to add the "not" versions of assertions now that I see this technique.

It might still be nice to have "not" versions so that the output can be silent when the test passes and explanatory when it fails, like we get for the non-negated case. Not a huge deal though.

Great point. Assertions are easy to add, so I'll get that queued up.

itzg · 2022-01-31T18:09:57Z

tests/setuponlytests/conflicting_world_for_spigot_server/verify.sh

@@ -0,0 +1,5 @@
+mc-image-helper assert fileExists world/level.dat && \


Might need to test the test, but it should be executing these will fail-fast so probably don't even need to chain these together with &&

Unfortunately not; I tested without && and with some of the asserts negated and it passed, even when it shouldn't have.

We could also add a set -e at the top of each verify script rather than &&-chaining, or we could source the verify scripts from a shell with set -e.

I thought the latter was the case, so I need to look into that.

I found and fixed the missing -e

26809ef

itzg · 2022-01-31T18:11:24Z

tests/setuponlytests/spigot_world_for_spigot_server/docker-compose.yml

+      EULA: "TRUE"
+      SETUP_ONLY: "TRUE"
+      VERSION: ${MINECRAFT_VERSION:-LATEST}
+      TYPE: "PAPER"


Spigot is effectively a dead release stream; however, it lives on as a "family" designation where Paper is a member of that family.

wlritchi · 2022-01-31T18:42:22Z

From my perspective the review suggestions here are all good, but I don't see any of them as blockers to this PR; unless you feel strongly about them I'd be inclined to leave them for a future cleanup. Personally I'd still like to do some real-world testing, but if you think the unit tests are good enough that works for me too.

itzg · 2022-01-31T21:41:48Z

No expectations on my part for any changes that are needed. I'm good with merging at this point or waiting until you have done more real world testing. Just let me know when you're ready.

Related to #1305

itzg · 2022-02-05T18:37:36Z

Hi @wlritchi , just wanted to check in and see how testing was going.

wlritchi · 2022-02-05T22:50:29Z

Sorry for the lack of activity with this; I've been on call for my day job over the last week so I haven't had a chance to put much time into side projects. I've got some time today, so I'm planning to do a few tests of this PR on my infra within the next 24 hours; I'll update you with the results. Thanks for checking in!

itzg · 2022-02-06T00:46:46Z

No problem at all. I did my oncall rotation just a couple of weeks ago, so I hear ya.

wlritchi · 2022-02-06T21:29:29Z

~~Manual testing failed; Spigot zips fail to load the overworld correctly (on either server type). I'm still determining what went wrong, but will update with a fix and tests that cover the oversight.~~

Edit: retracted; I was seeing unexpected behaviour with portals, but the world was in fact loading correctly.

wlritchi · 2022-02-06T21:36:29Z

Update to my above comment: for reasons unknown, Spigot zips are failing to retain links from portals in the nether to portals in the overworld, but only on first load. I've seen this same behaviour with the existing implementation, so it's not a regression nor a problem with this PR (still worth opening a ticket to look at though).

Since I had logged out in the nether on my testing worlds, this led me to arrive at a new, unrecognized location when I returned to the overworld on the final servers. I incorrectly believed that the overworld was being regenerated; in fact I was just somewhat far from where I expected to be.

Apologies for the false alarm above, but I wanted to block this PR from being merged while there was any doubt. I now assess that this PR passes manual tests, and can be merged safely.

itzg · 2022-02-06T21:51:17Z

Apologies for the false alarm above, but I wanted to block this PR from being merged while there was any doubt. I now assess that this PR passes manual tests, and can be merged safely.

Thanks for being thorough. Merge is on the way!

itzg · 2022-02-06T21:51:51Z

Build is going https://github.com/itzg/docker-minecraft-server/actions/runs/1803546434

wlritchi added 3 commits January 25, 2022 18:37

Add tests documenting expected behaviour

d71ca40

Reorganize dimensions for either server type

3b6b091

wlritchi changed the title ~~Adjust special case for Spigot world layout~~ Support Spigot-style and vanilla-style world zips, for any server type Jan 31, 2022

wlritchi marked this pull request as ready for review January 31, 2022 01:16

shotah reviewed Jan 31, 2022

View reviewed changes

itzg approved these changes Jan 31, 2022

View reviewed changes

itzg added a commit that referenced this pull request Feb 1, 2022

build: enable exit on failure for verify in setuponlytests

26809ef

Related to #1305

itzg merged commit b7bbe1b into itzg:master Feb 6, 2022

wlritchi deleted the fix-spigot-layout-zips branch February 6, 2022 22:24

		@@ -0,0 +1,5 @@
		mc-image-helper assert fileExists world/level.dat && \

Uh oh!

Support Spigot-style and vanilla-style world zips, for any server type #1305

Support Spigot-style and vanilla-style world zips, for any server type #1305

Uh oh!

Conversation

wlritchi commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shotah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itzg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wlritchi commented Jan 31, 2022

Uh oh!

itzg commented Jan 31, 2022

Uh oh!

itzg commented Feb 5, 2022

Uh oh!

wlritchi commented Feb 5, 2022

Uh oh!

itzg commented Feb 6, 2022

Uh oh!

wlritchi commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wlritchi commented Feb 6, 2022

Uh oh!

itzg commented Feb 6, 2022

Uh oh!

itzg commented Feb 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wlritchi commented Jan 25, 2022 •

edited

Loading

wlritchi commented Feb 6, 2022 •

edited

Loading