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

Fix creating temp directory to support multiple users #178

Merged
merged 2 commits into from Sep 17, 2019

Conversation

r6eve
Copy link
Contributor

@r6eve r6eve commented Sep 10, 2019

In the current implementation of creating temporary user's directory, there are two problems: always creates the directory of the same name cljam, and doesn't delete it after all is done. That might cause Permission denied if multiple users run cljam on the same machine.

This PR fixes the behavior; with-temp-dir macro creates an unique directory, and finally deletes it. Also, fixes a path to temporary directory such that the test code creates user's directory instead of test's directory.

@r6eve r6eve force-pushed the fix/temp-directory-for-multiple-users branch from 3c22c22 to a9bba94 Compare September 11, 2019 08:03
@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

Merging #178 into master will increase coverage by 0.05%.
The diff coverage is 82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage   86.86%   86.92%   +0.05%     
==========================================
  Files          74       74              
  Lines        5750     5765      +15     
  Branches      492      490       -2     
==========================================
+ Hits         4995     5011      +16     
- Misses        263      264       +1     
+ Partials      492      490       -2
Impacted Files Coverage Δ
src/cljam/algo/level.clj 85.1% <80%> (+2.12%) ⬆️
src/cljam/util.clj 91.22% <81.81%> (-3.65%) ⬇️
src/cljam/algo/sorter.clj 92.23% <84.61%> (+2.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c9f787...28fb728. Read the comment docs.

@r6eve r6eve marked this pull request as ready for review September 11, 2019 08:20
@r6eve r6eve requested a review from alumi as a code owner September 11, 2019 08:20
@r6eve r6eve requested a review from a team September 11, 2019 08:20
@ghost ghost requested review from athos and removed request for a team September 11, 2019 08:20
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thank you for working on this feature! Added a comment about the deletion.

src/cljam/util.clj Outdated Show resolved Hide resolved
@r6eve
Copy link
Contributor Author

r6eve commented Sep 12, 2019

Thanks for the speedy review! I missed out recursive deletions.

I've added missing features of recursive deletions, and the tests.

@r6eve
Copy link
Contributor Author

r6eve commented Sep 12, 2019

Sorry for my late push. I fixed miss test's directory a little bit more.

@r6eve
Copy link
Contributor Author

r6eve commented Sep 12, 2019

Sorry, I deleted extra spacing.

Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good other than indentation 👍

test/cljam/util_test.clj Outdated Show resolved Hide resolved
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update! 👍

dir-path (.getPath (file system-tmp-dir-path "cljam"))]
(.mkdirs (file dir-path))
dir-path))
(defmacro with-temp-dir
Copy link
Member

Choose a reason for hiding this comment

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

The expanded form of this macro looks somewhat large. More importantly, it contains a reify form in it, which would redundantly generate a dedicated anonymous class for each macro callsite.

How about breaking the implementation down into some functions like create-temp-dir and delete-temp-dir, and using them in the expanded form?

Copy link
Member

@athos athos left a comment

Choose a reason for hiding this comment

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

Hi, thanks for another improvement!

Just added a comment on the implementation of the macro.

@r6eve
Copy link
Contributor Author

r6eve commented Sep 13, 2019

Thank you for the helpful suggestion! I didn't know that.
To avoid the costs, I decided to make two functions and use them in expanded forms. 4820564

@athos
Copy link
Member

athos commented Sep 13, 2019

Ah, sorry if I confused you 🙇

I think providing with-temp-dir as an internal utility is completely fine. By "using them (the functions) in the expanded form", I just meant something like:

(defmacro with-temp-dir [bindings & body]
  (cond ...
        (symbol? (bindings 0))
        (let [[dir prefix] bindings]
          `(let [~dir (create-temp-dir ~prefix)]
             (try
               (with-temp-dir ~(subvec bindings 2) ~@body)
               (finally
                 (delete-temp-dir! ~dir)))))
        ...))

src/cljam/algo/level.clj Outdated Show resolved Hide resolved
src/cljam/algo/sorter.clj Outdated Show resolved Hide resolved
@r6eve r6eve force-pushed the fix/temp-directory-for-multiple-users branch from 4820564 to 28fb728 Compare September 16, 2019 04:11
@r6eve
Copy link
Contributor Author

r6eve commented Sep 16, 2019

Thank you for reviewing and I apologize for my poor understanding.
I fixed them and squashed the commits into two commits.

Copy link
Member

@athos athos 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 will merge it, thanks!

@athos athos merged commit 2db7fe7 into master Sep 17, 2019
@athos athos deleted the fix/temp-directory-for-multiple-users branch September 17, 2019 03:10
@r6eve
Copy link
Contributor Author

r6eve commented Sep 17, 2019

Thank you for correcting me and merging!

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

3 participants