sys::path::system_temp_directory vs. sys::fs::createTemporaryFile

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

This makes sense to me.

I have a few concerns:

  1. GetTempPathW is buggy, we should be manually using GetEnvironmentVariableW with TMP, TEMP and USERPROFILE arguments and use the first that exists.
  2. We should make sure that the platform specific pieces live in their respective Path.inc files.

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. :slight_smile:

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)

Ping2.

Rebased patch 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