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

Clean up temp file usage and fix #410 #411

Merged
merged 2 commits into from Oct 4, 2019

Conversation

hainesr
Copy link
Member

@hainesr hainesr commented Oct 1, 2019

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.

Rather than using the local folder.

Fixes rubyzip#410
It's not used in the library code.
@coveralls
Copy link

coveralls commented Oct 1, 2019

Coverage Status

Coverage decreased (-0.009%) to 95.443% when pulling e871842 on hainesr:tempfiles into 2825898 on rubyzip:master.

Copy link
Member

@jdleesmiller jdleesmiller left a comment

Choose a reason for hiding this comment

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

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 jdleesmiller merged commit a9a313a into rubyzip:master Oct 4, 2019
jdleesmiller added a commit that referenced this pull request Oct 4, 2019
@GrimmDaddy GrimmDaddy mentioned this pull request Oct 5, 2019
@hainesr hainesr deleted the tempfiles branch October 6, 2019 21:19
@samsonjs
Copy link

samsonjs commented Feb 3, 2020

@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)

@hainesr
Copy link
Member Author

hainesr commented Feb 4, 2020

I think this would be a reasonable option to have. Is it sensible to have the new behaviour the default though?

@samsonjs
Copy link

samsonjs commented Feb 5, 2020

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.

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

Successfully merging this pull request may close these issues.

None yet

5 participants