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

Choose USER-specific tmpdir #656

Merged
merged 1 commit into from
May 27, 2019
Merged

Choose USER-specific tmpdir #656

merged 1 commit into from
May 27, 2019

Conversation

cdunn2001
Copy link
Contributor

re: #80

The question is what to do on Windows. Here, we check for $USER in the environment, and use it iff set.

The other question is how to find /tmp. It's more common to check $TMPDIR on Linux and OSX, and getTempDir() is discouraged in Nim.

This will use $TMPDIR/$USER/nimble-$PID/nimblecache if possible, and getTempDir()/nimble-$PID/nimblecache otherwise. That way current behavior can (almost) be maintained on systems which lack USER or TMPDIR, but people have ways around this when needed.

Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

IMO Nim's getTempDir should be fixed to use $TMPDIR. We can work around it here for now but in the long run I don't want to have code that works around the stdlib's bugs, the stdlib should be fixed.

Anyway, thanks for creating this PR. We need a different getNimbleTempDir though, one which doesn't create a directory based on the PID.

@@ -382,7 +382,7 @@ proc execScript(scriptName: string, flags: Flags, options: Options): PSym =

# Ensure that "nimblepkg/nimscriptapi" is in the PATH.
block:
let t = getTempDir() / "nimblecache"
let t = getNimbleTempDir() / "nimblecache"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This defeats the purpose of the cache, getNimbleTempDir will return a different directory for each Nimble invokation...

Choose a reason for hiding this comment

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

Oh! Duh. Ok.

Choose a reason for hiding this comment

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

How about now?

@dom96
Copy link
Collaborator

dom96 commented May 27, 2019

I would change this to:

  • Use the result var
  • append the $USER to the result of getTempDir too

But this will do.

@dom96 dom96 merged commit a4eaa5d into nim-lang:master May 27, 2019
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.

3 participants