MommyHeather / AdvancedBackups

BSD 3-Clause "New" or "Revised" License
21 stars 5 forks source link

Bug report : Restore JAR locates backup folder wrongly #58

Closed huanqiugame closed 3 months ago

huanqiugame commented 5 months ago

Description

When I set an absolute path to the backup folder in the .properties file on my computer, backup process runs well, but when I try to run the script file (or run the 2 lines of command manually), it says

Could not find backup directory! 
/path/to/the/backup/folder 
Have you made any backups before?

From the error message, it can be recognized that the Restore JAR locates backup folder wrongly. It only adds the backup path value set in the .properties file to the end of the .minecraft folder, which creates a wrong path.

To Reproduce

Steps to reproduce the behavior:

  1. Set an absolute path in the .properties file: config.advancedbackups.path=/Users/username/Files/Games/Minecraft/1.20.1/Backups
  2. Launch Minecraft and join a world.
  3. Use /backup start to create a backup.
  4. Save your world, and close Minecraft.
  5. Navigate to your backup folder set in the .properties file.
  6. Run the script or run the jar manually. You should notice the error message described above.

Expected behavior

Normal backup restore process is expected.

Versioning

Additional questions

  1. Can a backup folder path have characters other than ASCII character (I.E. /Users/username/文件/My Minecraft Backup Folder)? If yes, should it be quoted in " or '? (I.E. set config.advancedbackups.path="/Users/username/文件/My Minecraft Backup Folder" in the .properties file)
  2. Can a backup folder path be set a Windows path format (I.E. D:\Games\Minecraft\1.20.1\Backups) on Windows? If not, should I set /d/Games/Minecraft/1.20.1/Backups in the example above?

Thanks so much!

MommyHeather commented 5 months ago

AdvancedBackups-fabric-1.20-3.4-clihotfix.zip

Got a hotfix here that should sort it - that'll propagate to all versions in v3.5. Do let me know how that goes - note that you'll need to boot the server once to update the restoration script with the new jar, alternatively you can rename this one to have the same name as the old one or just run the jar manually.

As for your questions -

  1. All UTF-16 characters that are supported by the filesystem should work. No quotes should be needed. I would however suggest sticking to ASCII characters, as non-ASCII paths can have inconsistent behaviours on different filesystems and are therefore hard to test and debug in the event of any issues.

  2. You can use the Windows path format, however double backslashes must be used - so D:\\Games\\Minecraft\\1.20.1\\Backups. D:/Games/Minecraft/1.20.1/Backups also works.

huanqiugame commented 5 months ago

Thank you for solving my problems that quickly! But there is somehow another problem… When I try to export backup as zip, it completely crashed. Same system settings as above. Terminal screenshot below.

2024-02-03 10 20 26

I also want to let you know that macOS auto-generates a .DS_Store file used by system, and (if I choose "restore single file" or "restore entire world") the restore process regards it as a "backup". So continue with that … it runs good but I'm not sure if something would go wrong. Maybe you'd like to add an ignore list.

MommyHeather commented 5 months ago

Thanks for the info on macos, I'm unable to test there so it's nice to know about the file it adds.

As for your issue with exporting the backup, I might know the cause already - but just in case, could you please show the file structure for the backups folder, and the same for your saves folder (or server root if this is a server)?

If its what I think it is, I can get you another hotfix out tomorrow, as its almost 3am right now. I'll also aim to get 3.5 as a whole released this weekend.

(I'm thinking the issue is a edgecase to do with how I've used a few different variables, but I'd need to reproduce the error to properly test a fix.)

Do the single file / entire world options work for you?

Naturally, these would not be a good idea to actually go through with when you don't need to restore anything, so you can just stop at the warning prompt.

huanqiugame commented 5 months ago

Have a good rest. I'm really looking forward to the new 3.5 update. My world save is fine. I want to check what my world used to be like simply. And I'm glad to help this project, too.

2024-02-03 11 43 45 2024-02-03 11 44 46

Here's two screenshots. The first one shows my backup folder. Every column shows files inside the selected folder on its left one. The second one is world folder. The column info (from left to right) shows Name, Modified Time, Size and Type. You should be able to understand each… (Additionally, I set to dark mode for your eyes!👀)

If you need more information, talk to me anytime!

MommyHeather commented 5 months ago

Confirmed repro, and hopefully a confirmed fix - and here's one for you to use. AdvancedBackups-fabric-1.20-3.4-clihotfix-2.zip The same points about the filename apply as before.

Also added a couple of utility changes - mainly, better error reporting and a filter for that .DS_Store file you mentioned, could you check that too please?

I need to rewrite the whole CLI at some point. Perhaps that'll be the update after 3.5.

huanqiugame commented 5 months ago

Somehow a problem again…

Please select your world. Default for servers is "world".
1. mods test
2. mods test but infinite world
3. Huanqiu 空岛生存
3
Select a backup to restore.
Exception in thread "main" java.lang.UnsupportedOperationException: remove
    at java.base/java.util.Iterator.remove(Iterator.java:102)
    at java.base/java.util.Collection.removeIf(Collection.java:577)
    at co.uk.mommyheather.advancedbackups.cli.AdvancedBackupsCLI.getBackupDate(AdvancedBackupsCLI.java:269)
    at co.uk.mommyheather.advancedbackups.cli.AdvancedBackupsCLI.main(AdvancedBackupsCLI.java:161)

However, when I chose world 1, it works. I checked the backup folder in world 1 and 3, and there is a .DS_Store file under Huanqiu 空岛生存/differential.

MommyHeather commented 5 months ago

AdvancedBackups-fabric-1.20-3.4-clihotfix-3.zip Wonderful. I missed that a method I used returned an internal java class with the exact same name as one that supported the remove method I was using.

Definitely partially my fault. Regardless, this is hopefully the last hotfix. You should no longer see that .DS_Store file, and logging should also be better.

MommyHeather commented 5 months ago

Thanks for consistently helping with this by the way!

huanqiugame commented 5 months ago

Thanks! Sorry for my delay… I was busy editing a video yesterday and forgot about this. I'm now away from home and I'll try the new hot fix as soon as I get home. Hopefully that would be the last hot fix. 💪 I've tested everything. It seems to work well. Thank you very much! (Additionally, I suggest to export the whole world as zip to the .minecraft/saves folder, as we normally use it there.)

MommyHeather commented 5 months ago

Good to hear it's all working for you.

As for exporting into the saves folder, that's definitely an option but I'd still prefer to have the user unzip, so there'd be very little benefit. regardless, mind making it a separate feature request so this issue can stay on topic?

huanqiugame commented 5 months ago

Oh, of course I don't mind! I originally mean that the current one chooses to export the whole zip file to the minecraft folder, which makes me really confused. After the restore process, I checked the whole backup folder, the saves folder, the mods folder and even the user root (~) folder! Finally I found it in the minecraft folder. So I think it's a better way to change the export path or remind that in the prompt. However, opt on your own!

MommyHeather commented 5 months ago

One solution there is to give a full path to the file, rather than just the filename.

and come to think of it, i do already know if the user is on a server or not, so i could export to the saves directory if so... hmm.

huanqiugame commented 5 months ago

Maybe output a path is enough and, if user changes the world folder name set in server.properties, it's still able to find the export file. Is there trouble with identifying the world name in server.properties?

Another option is to create an in-game GUI that is able to open the folder directly, but then we know it's client.

github-actions[bot] commented 3 months ago

This issue has been closed - fixed in 3.5 which has now been pushed to Curseforge and Modrinth.

MommyHeather commented 3 months ago

reopening because the CF workflow is erroring. you can still get jars from here