-
Notifications
You must be signed in to change notification settings - Fork 7
Store pull files in temp directory #253
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
base: master
Are you sure you want to change the base?
Conversation
In theory this could be a new function in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check also the initial download of projects - currently we download the temporary data to the project directory (!) - e.g. photo.jpg
will be first downloaded as photo.jpg.0
to the project directory (possibly also photo.jpg.1
, photo.jpg.2
... if the file is large and gets downloaded in multiple chunks), then it gets renamed / merged from chunks to the final photo.jpg
. I think those temporary files should also go to a temporary directory first. (See _download_items()
and UpdateTask.apply()
)
mergin/client_pull.py
Outdated
@@ -413,7 +413,8 @@ def pull_project_async(mc, directory): | |||
# then we just download the whole file | |||
_pulling_file_with_diffs = lambda f: "diffs" in f and len(f["diffs"]) != 0 | |||
|
|||
temp_dir = mp.fpath_meta(f"fetch_{local_version}-{server_version}") | |||
temp_dir_mergin = os.path.join(tempfile.gettempdir(), f"mergin-{mp.project_id()}") | |||
temp_dir = os.path.join(temp_dir_mergin, f"fetch_{local_version}-{server_version}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about using e.g. tempfile.TemporaryDirectory(prefix="mm-pull-")
here and keeping the tmp dir reference in the pull job? That should be safer and always unique. We could also use it with delete=True
flag and even ignore_cleanup_errors=True
- and we don't need to worry about its cleanup, even in case of exceptions etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good alternative to mkdtemp
. I think we can not use with
context here.
I think we can manually cleanup temp dir on the finish of pull (rmtree(temp_dir, ignore_errors=True) string
alternatively use cleanup()
method of TemporaryDirectory
). But sure, just 🍒
there is qustionable also "unfinish_pull_dir" (where also some pull related staff is stored) and ".cache" dir (which is ok I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just question about using different function shortcut for creating temp dir somehwere :)
Pull Request Test Coverage Report for Build 16467542402Details
💛 - Coveralls |
Change pull process so that downloaded files and file parts are stored in
temporary directory
(based on system) and not in.mergin
folder in project. This should solve issue with external sync tools (ie. Dropbox, OneDrive, GDrive and others) where those files might be selected for sync and thus cannot be remove once MM pull job ends.Fixes MerginMaps/qgis-plugin#464