Hi, all. I noticed recently that llvm::sys::fs::createTemporaryFile does not use llvm::sys::path::system_temp_directory, instead relying on its platform-specific helper TempFile. Is there any reason for this disparity?
The two implementations are not in sync, either:
- TempDir honors TMPDIR, TMP, TEMP, TEMPDIR, and a configuration-settable P_tmpdir on Unix-y systems. system_temp_directory just honors TMPDIR.
- TempDir calls ::GetTempPathW on Windows, while system_temp_directory honors TEMP and falls back to C:\\TEMP.
- system_temp_directory prefers DARWIN_USER_TEMP_DIR on Darwin.
- system_temp_directory has an option for "temporary directory that persists past reboot", while TempDir is never used for anything but transient files and directories
Does anyone mind if I merge the platform-specific bits of TempDir into system_temp_directory? Does anyone have other opinions here?
Jordan
But that is orthogonal, no?
If I understand correctly, Jordan's question is "do we need both
system_temp_directory and TempDir"? I doesn't look like it, it seems
that every user of TempDir could be replaced with
system_temp_directory.
Cheers,
Rafael
Well, part of my original point is that TempDir is smarter than system_temp_directory in a few ways. I suppose the Windows implementation isn't one of them.
To be clear, I don’t think we need both; I’d like for us to drop one of them. I just want to make sure that that the final state, post merge, works well on all of our platforms. 
Cool. The attached patch removes TempDir. It passes all tests, but the
OS X case looks a bit suspicious. I first implemented it by given
priority to confstr, but we depend on the env variables taking
priority for testing the crash report. Is there some way to change
what confstr returns?
Should we just add a -cc1 command line option for use in
test/Driver/crash-report.c?
Cheers,
Rafael
t.patch (7.31 KB)
Thanks for doing this, Rafael!
It looks like OS X always has TMPDIR set these days (to the value of _CS_DARWIN_USER_TEMP_DIR), so I think we should just go with TMPDIR if it’s set and non-empty, and then fall back to confstr. However, we should not be using TMPDIR if ErasedOnReboot is false (i.e. if we’d prefer _CS_DARWIN_USER_CACHE_DIR). Unfortunately there’s no canonical environment variable for that.
CCing Greg to see if that makes sense.
Jordan
It looks like OS X always has TMPDIR set these days (to the value of
_CS_DARWIN_USER_TEMP_DIR), so I think we should just go with TMPDIR if it's
set and non-empty, and then fall back to confstr. However, we should not be
using TMPDIR if ErasedOnReboot is false (i.e. if we'd prefer
_CS_DARWIN_USER_CACHE_DIR). Unfortunately there's no canonical environment
variable for that.
CCing Greg to see if that makes sense.
OK, so I guess something like this?
Cheers,
Rafael
t.patch (7.4 KB)
Well, that changes behavior on Linux; previously it did honor TMPDIR when looking for a cache directory. Is that okay? (The Windows version is also honoring TEMP in this case.)
What are we using the cache dir for besides the module cache?
Jordan
Well, that changes behavior on Linux; previously it did honor TMPDIR when
looking for a cache directory. Is that okay? (The Windows version is also
honoring TEMP in this case.)
I *think* that is a bug fix. I don't have those env variables set on
my linux machine (fedora 20), but I don't remember seeing them
pointing to anything but /tmp in the past and that is even in tmpfs
these days.
What are we using the cache dir for besides the module cache?
Only that. There is one call in lvm and 2 in clang-tools-extra, but
they both pass true to erasedOnReboot.
Jordan
Cheers,
Rafael
ping. Rebased version attached.
t.patch (7.4 KB)
Sorry, I've been meaning to double-check that this does the same thing for sandboxed apps. Let me get back to you tonight or tomorrow.
Okay, confirmed that this is reasonable on OS X. If we're okay with not having an env var for the cache dir, I'd say go ahead. Thanks, Rafael!
Jordan