Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Advanced Copy Options - Symlinks and more! #833

Merged
merged 46 commits into from May 2, 2023

Conversation

Ryex
Copy link
Contributor

@Ryex Ryex commented Feb 7, 2023

Lets let users copy instances with some more advanced settings, potentially preserving disk-space.

Symbolic links and Hard links are the standard, but I was even able to do CoW on ZFS, BTRFS, XFS, APFS on MacOS, and even BTRFS (via winbrtfs driver) and ReFS on windows!

fix: #761

related: #422, #38

  • Windows
    • Filesystem::create_link (hard links) (no perms needed!)
    • Filesystem::create_link (symbolic) (need elevated process)
    • Filesystem::clone (winbtrfs and ReFS only, clone function uses FSCTL_DUPLICATE_EXTENTS_TO_FILE and DeviceIoControl )
  • Linux
    • Filesystem::create_link (hard links)
    • Filesystem::create_link (symbolic)
    • Filesystem::clone (COW via ioctl_ficlone)
  • Macos
    • Filesystem::create_link (hard links)
    • Filesystem::create_link (symbolic)
    • Filesystem::clone (COW via clonefile)
  • Export process, knows to follow symlinks.
  • safety feature so users know when they are modifying linked content.

launcher/DesktopServices.cpp Fixed Show fixed Hide fixed
launcher/FileSystem.cpp Fixed Show fixed Hide fixed
launcher/FileSystem.cpp Fixed Show fixed Hide fixed
launcher/FileSystem.cpp Fixed Show fixed Hide fixed
@Ryex
Copy link
Contributor Author

Ryex commented Feb 9, 2023

OK. basic functionality is in place. Now there just needs to be a discussion of the safety feature that need to be in play.

image

please review!

@Ryex Ryex marked this pull request as ready for review February 9, 2023 09:04
@Ryex Ryex changed the title WIP: Advanced Copy Options - Symlinks and more! Advanced Copy Options - Symlinks and more! Feb 9, 2023
launcher/CMakeLists.txt Outdated Show resolved Hide resolved
launcher/ui/dialogs/CopyInstanceDialog.ui Outdated Show resolved Hide resolved
launcher/ui/dialogs/CopyInstanceDialog.ui Outdated Show resolved Hide resolved
launcher/ui/dialogs/CopyInstanceDialog.ui Outdated Show resolved Hide resolved
@Ryex
Copy link
Contributor Author

Ryex commented Feb 10, 2023

I updated the logic on links
Now there is no danger of an instance overwriting the original config when "copied" via symbolic or hard links
if "link recursively" is turned off instance-folder/* is linked to new-instance-folder/* (top level items are linked except for instance.cfg)
if "link recursively" is turned on instance-folder/**/* is linked to new-instance-folder/**/* (all items are linked except for instance.cfg)

UI logic is cleaned up:
soft and hard links are mutually exclusive check-boxes now. Both are exclusive to clone.

@Ryex
Copy link
Contributor Author

Ryex commented Feb 10, 2023

windows btrfs reflink support by calling ReflinkCopyW from shellbtrfs.dll provided by winbtrfs!

NOTE: This is slight shenanigans because ReflinkCopyW is a bit broken atm
maharmstone/btrfs#556

but the code should be transparently backwards compatible if it get fixed.

@Ryex Ryex force-pushed the advanced_copy_instance branch 5 times, most recently from 7eccecf to a421df2 Compare February 11, 2023 02:53
@Ryex
Copy link
Contributor Author

Ryex commented Feb 11, 2023

I'v now implimented windows Reflink on both winbtrfs and ReFS via DeviceIoControl calls.

The old code remains in place for now until we can be confident there are no issues.

@DioEgizio DioEgizio added this to the 7.0 milestone Feb 11, 2023
@DioEgizio DioEgizio added enhancement New feature or request macOS Issues and PRs related to macOS specifically Windows Issues and PRs related to WIndows specifically Linux Issues and PRs related to Linux specifically labels Feb 11, 2023
@Ryex
Copy link
Contributor Author

Ryex commented Feb 12, 2023

Latest change adds the UAC shield to the symlink checkbox on windows. selecting it adds the shield icon to the ok button too.

image

@Ryex
Copy link
Contributor Author

Ryex commented Feb 12, 2023

and... done.

now there are warnings when resources are symbolically linked or hard linked.

image

launcher/minecraft/WorldList.cpp Outdated Show resolved Hide resolved
launcher/minecraft/WorldList.cpp Outdated Show resolved Hide resolved
launcher/minecraft/WorldList.cpp Outdated Show resolved Hide resolved
launcher/minecraft/mod/ModFolderModel.cpp Outdated Show resolved Hide resolved
launcher/minecraft/mod/ModFolderModel.cpp Outdated Show resolved Hide resolved
launcher/minecraft/mod/ResourceFolderModel.cpp Outdated Show resolved Hide resolved
launcher/minecraft/mod/ResourcePackFolderModel.cpp Outdated Show resolved Hide resolved
launcher/minecraft/mod/ResourcePackFolderModel.cpp Outdated Show resolved Hide resolved
@getchoo
Copy link
Member

getchoo commented Feb 14, 2023

works as expected on ntfs, but i think i came across a bit of an issue: currently, if you try to delete the instance that a copy is symbolically linked to, the copied instance is now completely unusable due to the links being unresolved. imo, a good solution here would be to delete the symlinks and copy over the files from the original instance being deleted - or at least add a warning to the deletion dialog ;)

this is awesome though, i love it!

edit: forgor to specify symbolic links

@Ryex
Copy link
Contributor Author

Ryex commented Feb 14, 2023

works as expected on ntfs, but i think i came across a bit of an issue: currently, if you try to delete the instance that a copy is symbolically linked to, the copied instance is now completely unusable due to the links being unresolved. imo, a good solution here would be to delete the symlinks and copy over the files from the original instance being deleted - or at least add a warning to the deletion dialog ;)

this is awesome though, i love it!

edit: forgor to specify symbolic links

I'm not really sure what to do in this situation. It's nigh impossible to detect when a symlink exists without knowing exactly where to look for it. The only way to warn users when they are deleting an instance another instance links files form would be to keep track of all the symlinks an instance has and do a search through all other instances when doing a delete... I'm not sure that would be smart or quick.

@getchoo
Copy link
Member

getchoo commented Feb 14, 2023

works as expected on ntfs, but i think i came across a bit of an issue: currently, if you try to delete the instance that a copy is symbolically linked to, the copied instance is now completely unusable due to the links being unresolved. imo, a good solution here would be to delete the symlinks and copy over the files from the original instance being deleted - or at least add a warning to the deletion dialog ;)

this is awesome though, i love it!

edit: forgor to specify symbolic links

I'm not really sure what to do in this situation. It's nigh impossible to detect when a symlink exists without knowing exactly where to look for it. The only way to warn users when they are deleting an instance another instance links files form would be to keep track of all the symlinks an instance has and do a search through all other instances when doing a delete... I'm not sure that would be smart or quick.

i can understand speed when recursing through all instances, but keeping track of links not being smart? i would have to disagree with that
at bare minimum, warning users that other instances could be effected by deleting another would be an infinitely better solution than having an instance break without warning - and though you could say symbolic links make that self explanatory, a majority of people may not understand how they work or the consequences of deleting the original file.

i think that a great solution here would be to add a key to instance.cfg (if possible) that contains the source of an instance using symbolic links. this would allow the ability to easily recurse through all instances in a performance friendly way, while limiting the more time consuming task of finding symlinks. afterwards, a prompt could be shown explaining something along the lines of "the files of this instance are used by , would you like to delete it as well or copy the files over?"

just an idea though, i'm not sure how hard that would be to implement - but again, at bare minimum, i think there needs to be a warning here or else there's a good chance people using this feature are going to come across this and be very confused about what comes up in the error log

@TayouVR
Copy link
Member

TayouVR commented Feb 14, 2023

maybe give the original a list in the instance.cfg with all symlinked instances (their IDs), and then when deleting there could be a warning popup with
"The Instance(s) %1 might reference files in this instance. Deleting it could break the other instance(s), are you sure?"

@Ryex
Copy link
Contributor Author

Ryex commented Mar 20, 2023

re-based because clean git history is cool.

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i may be able to finish it off in the weekend

i lied

sorry, this PR gives me a lot of brain fatigue (your work is great, but os-specific stuff gives me nightmares) 😵

launcher/CMakeLists.txt Outdated Show resolved Hide resolved
launcher/FileSystem.cpp Outdated Show resolved Hide resolved
launcher/FileSystem.cpp Outdated Show resolved Hide resolved
launcher/FileSystem.cpp Show resolved Hide resolved
launcher/FileSystem.cpp Outdated Show resolved Hide resolved
launcher/FileSystem.cpp Show resolved Hide resolved
launcher/settings/INIFile.cpp Outdated Show resolved Hide resolved
launcher/settings/SettingsObject.cpp Outdated Show resolved Hide resolved
launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
Ryex and others added 2 commits March 31, 2023 18:25
Co-authored-by: flow <flowlnlnln@gmail.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
@Ryex
Copy link
Contributor Author

Ryex commented Apr 1, 2023

i may be able to finish it off in the weekend

i lied

sorry, this PR gives me a lot of brain fatigue (your work is great, but os-specific stuff gives me nightmares) dizzy_face

no worries. don't feel too overwhelmed by it though. when it comes down to it it's pretty simple stuff despite appearances. I documented the parts that need external research to really understand.

Ryex added 5 commits April 3, 2023 17:14
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
@Continous
Copy link

@getchoo @TayouVR I've added a somewhat naive link tracking system. each instance when it is made via copy saves a linkedInstancesList setting with is a json list containing the ID of the original instance. When an instance is asked to be deleted there is now an additional second message box that lists the instances that say they are linked to the deletion candidate, warns that these instances may break, and asks for confirmation.

@DioEgizio I made sure the _filelink.exe was included in the setup exe, I know you mentioned that somewhere.

Is there any possibility we could have an additional option to allow a deletion of the original instance to prompt the user to have the original files migrated to the linked instance? If it's not too difficult, we could even allow linked instances to become "real" instances on command from the user in such case they may want or need it. A good example I can think of is if someone plans on migrating from one version to the next, and has finally decided to remove the previous instance.

@Ryex
Copy link
Contributor Author

Ryex commented Apr 20, 2023

@getchoo @TayouVR I've added a somewhat naive link tracking system. each instance when it is made via copy saves a linkedInstancesList setting with is a json list containing the ID of the original instance. When an instance is asked to be deleted there is now an additional second message box that lists the instances that say they are linked to the deletion candidate, warns that these instances may break, and asks for confirmation.
@DioEgizio I made sure the _filelink.exe was included in the setup exe, I know you mentioned that somewhere.

Is there any possibility we could have an additional option to allow a deletion of the original instance to prompt the user to have the original files migrated to the linked instance? If it's not too difficult, we could even allow linked instances to become "real" instances on command from the user in such case they may want or need it. A good example I can think of is if someone plans on migrating from one version to the next, and has finally decided to remove the previous instance.

perhaps in another PR. that's a fair bit of work and this one is big enough already.

Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i swear we will finish this soon 🥺

launcher/FileSystem.cpp Outdated Show resolved Hide resolved
launcher/settings/INIFile.cpp Outdated Show resolved Hide resolved
launcher/minecraft/mod/ResourceFolderModel.h Show resolved Hide resolved
launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
launcher/ui/dialogs/CopyInstanceDialog.cpp Outdated Show resolved Hide resolved
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
…ce root.

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job 🙂 /gen

@DioEgizio
Copy link
Member

2 approvals 👀

@Scrumplex
Copy link
Member

Let's merge, I guess.

I probably don't have much to add after @flowln's thorough analysis :D

And if the feature has issues, we can hopefully catch them before an eventual release.

@Scrumplex Scrumplex merged commit 64ba5e4 into PrismLauncher:develop May 2, 2023
14 checks passed
@Ryex Ryex mentioned this pull request May 2, 2023
1 task
@Un1q32
Copy link
Contributor

Un1q32 commented May 30, 2023

1.20 prerelease 7 gives a warning when opening worlds that use symlinks, and requires allowed symlinks to be added to a text file to silence the warning. Will this cause problems?

@DioEgizio
Copy link
Member

Make an issue for it please

@Un1q32
Copy link
Contributor

Un1q32 commented May 31, 2023

Make an issue for it please

I haven't actually tested anything so I don't know if its a problem, I just randomly remembered it so figured I would mention it.

@RNanB
Copy link

RNanB commented Jun 1, 2023

1.20 prerelease 7 gives a warning when opening worlds that use symlinks, and requires allowed symlinks to be added to a text file to silence the warning. Will this cause problems?

No, this change only affects linking of individual worlds. Linking the saves folder itself will still work the same

@Ryex
Copy link
Contributor Author

Ryex commented Jun 1, 2023

1.20 prerelease 7 gives a warning when opening worlds that use symlinks, and requires allowed symlinks to be added to a text file to silence the warning. Will this cause problems?

No, this change only affects linking of individual worlds. Linking the saves folder itself will still work the same

This could still have an effect if the user tries to recursively link during a copy. That will need the file created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Linux Issues and PRs related to Linux specifically macOS Issues and PRs related to macOS specifically Windows Issues and PRs related to WIndows specifically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instance duplication doesn't use reflink copying with Btrfs
10 participants