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

rewrite data persisting #2897

Merged
merged 4 commits into from
Jan 17, 2019
Merged

rewrite data persisting #2897

merged 4 commits into from
Jan 17, 2019

Conversation

chawyehsu
Copy link
Member

@chawyehsu chawyehsu commented Dec 14, 2018

Data matters, please review

This is a rework of #2891 #2890 and #2882 . This will also fix #2724 and fix #2900 , close #2779

Introduction

I add a function called unlink_persist_data($dir), which recursively unlink all junction/hard link in the given directory. This affects some sub-commands which have interaction with junction/hard link:

  • scoop-install: the persisting logic has been improved as follow:
    1. if there are data in the persist folder, also in app(source) folder, rename that one located in app folder (e.g. app's default setting files) with a original suffix, then create link from persist to app
    2. if there are data in the persist folder, but no in app(source) folder, just create link from persist to app
    3. if there is no data in the persist folder (e.g. fresh install), but there are data in app(source) folder (e.g. app's default setting files), we will just use that default setting files as the first-time persisting. So move that files from app folder to persist folder, then create link from persist to app
    4. But what if if there is neither data in the persist folder (e.g. fresh install), nor in the app(source) folder (e.g. setting files will be created after first startup, like Everthing.db). We need to create empty persisting target in the persist folder. But by default we can't know if a persisting target is a file or a directory (e.g. conf.d). So we create a directory by default, and to avoid this, manifest maintainers should use pre_install to manually create the source file before persisting.
  • scoop-reset: reset command uses the logic of install, but there is a problem: before re-persisting data, there have been junction/hard link in the app(source) folder. It will throw an exception of file exists Persisting bug with already existing folder #2724 . To fix this, we should unlink all old link before re-persisting, using unlink_persist_data.
  • scoop-uninstall: Remove-Item can not remove NTFS junction, we need to unlink all persistting data, before uninstalling/deleting an app, but keeping persisting data.
  • scoop-cleanup: like uninstall, Remove-Item can not remove NTFS junction, we need to unlink all persistting data, before deleting old versions of an app. Before PR scoop-cleanup: handle with junction better #2882 , it use fsutil.exe to unlink junction, which is not friendly to restricted users (scoop-cleanup is not friendly to restricted user #2832 and scoop-cleanup: Remove-Item : Index (zero based) must be greater than or equal to zero... #2881 ).

Beyond the logic improvement, there is a new feature now: it supports sub-folder data persisting, like:

{
    "homepage": "https://scoop.sh",
    "description": "A dummy manifest for scoop tests.",
    "license": "Freeware",
    "version": "1.1.0",
    "url": "https://get.scoop.sh",
    "pre_install": [
        "if (!(test-path \"$dir\\dummy.txt\")) { new-item -force \"$dir\\dummy.txt\" -itemtype file | out-null }",
        "if (!(test-path \"$dir\\dummydir\")) { new-item \"$dir\\dummydir\" -itemtype directory | out-null }",
        "if (!(test-path \"$dir\\subdir\")) { new-item \"$dir\\subdir\" -itemtype directory | out-null }",
        "if (!(test-path \"$dir\\subdir\\subdummydir\")) { new-item \"$dir\\subdir\\subdummydir\" -itemtype directory | out-null }",
        "if (!(test-path \"$dir\\subdir\\subdummy.txt\")) { new-item -force \"$dir\\subdir\\subdummy.txt\" -itemtype file | out-null }",
    ],
    "persist": [
        "dummy.txt",
        "dummydir",
        "subdir\\subdummydir",
        "subdir\\subdummy.txt"
    ]
}

So no need to strip directories of source for target anymore.

To participate in the code review and tests, go visit https://github.com/h404bi/scoop-persist-test for test cases.

@chawyehsu chawyehsu changed the title totally rewritten on data persisting rewrite data persisting Dec 16, 2018
@r15ch13
Copy link
Member

r15ch13 commented Dec 16, 2018

About those hard-links:

Not all of those hard-linked files work. Like Witch Blast for example. (easy way to reproduce this problem)
config.dat and game.sav get created on the first start and first new game.

The pre_install creates an empty game.sav file, but the game removes it on the first start without unlinking it (probably because the format is wrong). Saving the game will write a new game.sav but the file in $persist_dir is still 0kb.
For these cases, my idea was, that all hard-linked files get copied over to $persist_dir if the directory is modified in some way (like uninstall/update/reset).
This could also solve the problem with files that are not created on install like the aforementioned Everything.db.

Persist feature in general

Maybe adding more information about how to handle different data could be added to the persist property. Which would make it more complex, and may break backward compatible, but could reduce the guessing game about file/directory type. (e.g. config files without extension, directories with extension or starting with a dot)
Also we could specifiy if a directory should be merged or linked, which would make it better with handling plugin folders.

Example which just came to my mind:

    "persist": [
        {
            "type": "file",
            "name": "config.dat",
            "content": "[Config]`nlanguage=eng",
            "encoding": "UTF-8",
            "method": "copy"
        },
        {
            "type": "directory",
            "name": "data",
            "method": "link"
        },
        {
            "type": "directory",
            "name": "plugins",
            "method": "merge"
        }
    ]

@chawyehsu
Copy link
Member Author

It looks like Everything also has the problem the same as Witch Blast.
I like your idea (especially the merge feature), but that's really a breaking change. So I prefer to finish this pull-request firstly. After that, consider a better way to handle with first-time data persisting.

@chawyehsu
Copy link
Member Author

Added the trigger for closing issue #2900 , there is no further changes I can do for this pull-request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup is not $dir aware Persisting bug with already existing folder
3 participants