Clean up temp file usage and fix #410 #411
Conversation
Rather than using the local folder. Fixes rubyzip#410
It's not used in the library code.
There was a problem hiding this comment.
LGTM 👍
I guess this would cause a problem if there was not a writeable temporary directory, but that seems less likely than a situation where the folder with the zip file is not writable.
It seems to have been introduced a long time ago, in cd038ae (with minor variations since then, notably #325).
Good find on tmpdir.
|
@jdleesmiller @hainesr This is a problem for me as I have a setup where the root volume is small and there's a second, larger volume dedicated for zipping files. If I were to submit a PR that adds some kind of flag or option to control whether the zip file is created in the system temp directory or the zip directory is it likely to be accepted? (defaulting to the new behaviour) |
|
I think this would be a reasonable option to have. Is it sensible to have the new behaviour the default though? |
|
Ok great! I'll work on it and should have a PR for you fairly soon. Now that it's changed I feel like it makes sense to keep it this way. It seems like good behaviour because if zip operation is interrupted for some reason any leftover files are more likely to be cleaned up from /tmp automatically. I'm happy to code it up however you like though. For our use-case it won't make an appreciable difference either way. |
There didn't seem to be any particular reason why we weren't using the OS temp directory to store temporary files, and this was reported as an issue on Windows in #410.
This PR fixes that, and also cleans up the usage of
tmpdir- which isn't needed in the library code, but is used in the tests.