[PATCH] D14231: [Support] Use GetTempDir to get the temporary dir path on Windows.

[Aaron Ballman via llvm-commits]

aaron.ballman added inline comments.

================
Comment at: unittests/Support/Path.cpp:366
@@ +365,3 @@
+
+  _wputenv_s(L"TMP", L"C:\\OtherFolder");
+  path::system_temp_directory(true, TempDir);
----------------
chfast wrote:
> aaron.ballman wrote:
> > I am slightly worried about this for systems that don't mount a C drive, but I suspect those are passingly rare. ;-)
> > 
> > What has me way more worried is that failing these tests will not restore the TMP (and TEMP, below) environment variables. So if any of these tests fail, we leave the user's machine in a broken state. This can be solved with some RAII wrappers.
> > 
> > Further, changing the temp directory out from under the user affects *every* application on their machine. This means there's a non-zero chance that temp files start going to incorrect locations for other applications. I don't think there's a way to solve this, and I'm really not comfortable with that because it can lead to hard-to-trace problems for developers. Ideas most certainly welcome.
> https://msdn.microsoft.com/en-us/library/83zh4e6k.aspx
> > _putenv and _wputenv affect only the environment that is local to the current process; you cannot use them to modify the command-level environment
> 
> So, changing env vars can affect unit tests running in other threads. RAII for restoring original values is worth considering.
That is awesome, and also something I totally did not know. Thank you for pointing that out!

However, a simple test shows there's still a problem -- we have ExecuteAndWait (and other functions) that create subprocesses (possibly in parallel to these tests), almost all of which pass nullptr as the environment variable. When CreateProcess is eventually called with no environment, it inherits the changes made by _wputenv_s(). So this has the possibility to change the environment out from under our own tests, and those tests may in turn try to create temporary files in the wrong place. :-(

My very simple test code was:
```
#include <iostream>
#include <Windows.h>

void f(const wchar_t *M) {
  wchar_t B[MAX_PATH + 2];
  ::GetTempPathW(MAX_PATH + 2, B);
  std::wcout << M << B << std::endl;
}

int main(int argc, const char *argv[]) {
  if (argc == 1) {
    f(L"Original: ");
    _wputenv_s(L"TMP", L"C:\\OtherFolder");
    STARTUPINFOA si = { 0 };
    si.cb = sizeof(si);
    PROCESS_INFORMATION pi = { 0 };
    ::CreateProcessA(argv[0], "test test test", nullptr, nullptr, TRUE,
                     CREATE_UNICODE_ENVIRONMENT, nullptr, nullptr, &si, &pi);
  } else {
    f(L"Subprocess: ");
  }
}
```
While this is hopefully not really an issue in reality, I still worry about giving ourselves head-scratching debugging problems that are nondeterministic, as well as cluttering up someone's hard drive with temp files in the wrong locations.

Do these tests currently run serially with regards to other tests (even from lit)? If so, then I think it's a risk we should document (probably around these new tests as well as Execute() in Program.inc for Win32) and just move on despite my discomfort. This seems about as good as we're going to be able to get for testing. If we run in parallel, then I'm a bit more worried.

================
Comment at: unittests/Support/Path.cpp:386
@@ +385,3 @@
+  _putenv_s("TMP", Expected.c_str());
+  path::system_temp_directory(true, TempDir);
+  EXPECT_EQ(Expected, TempDir);
----------------
chfast wrote:
> Here is the test that checks system_temp_directory() in case $TMP is 260 chars long.
Does this test pass on Windows 7 (if possible, with no service packs installed)?


Repository:
  rL LLVM

http://reviews.llvm.org/D14231