[libc++] Android temp dir is /data/local/tmp, enable Windows test

[rprichard]

Summary

[libc++] Android temp dir is /data/local/tmp, enable Windows test

On Android, std::filesystem::temp_directory_path() should fall back to
/data/local/tmp when no environment variable is set. There is no /tmp
directory. Most apps can't access /data/local/tmp, but they do have a
"cache dir" (Context#getCacheDir()) that is usable for temporary files.
However, there is no obvious and reliable way for libc++ to query this
directory in contexts where it is available. The global fallback
/data/local/tmp is available for "adb shell", making it useful for test
suites.

On Windows, temp_directory_path falls back to the Windows directory
(e.g. "C:\Windows"), so call GetWindowsDirectoryW to do the test.

[ldionne]

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp

139–144

Why not this? Otherwise, we're losing coverage.

[rprichard]

Re-enable the test for Android.

[rprichard]

Enable the test for Windows.

[rprichard]

Adding @mstorsjo for the Windows part of the change.

[mstorsjo]

Ok with me from the Windows perspective, as long as CI is passing. (Currently it seems to have failed for some unrelated reason?)

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp

146

I don't quite see why this specific line is needed here (I do kinda get what it hints at, based on the docs for GetWindowsDirectoryW though), but this should be effectively covered by the ret == win_dir check anyway, right? (I.e. the purpose of this test is to test the stdlib's temp path generation, not check GetWindowsDirectoryW.

[rprichard]

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp

146

I suppose I was trying to verify that ret and win_dir both do not end with a backslash. I can remove this line and rely on the MSDN docs if you'd like.

Aside, the MSDN docs contradict themselves in the (hypothetical) case that the Windows directory is the root:

https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getwindowsdirectoryw:

This path does not end with a backslash unless the Windows directory is the root directory. For example, if the Windows directory is named Windows on drive C, the path of the Windows directory retrieved by this function is C:\Windows. If the system was installed in the root directory of drive C, the path retrieved is C:.

Realistically, I think that never happens?

[mstorsjo]

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp

146

Ah, now I see the angle where you're coming from, that it'd be good to verify that the returned path has a specific form.

With that in mind, I guess it's fine to keep the test - it probably doesn't matter much either way.

Yeah I noted that contradiction in the docs - I would believe it's a simple typo and just was meant to say "If the system was installed in the root directory of drive C, the path retrieved is C:\."

[ldionne]

Requesting changes for the queue. Is this ready to be re-reviewed?

[rprichard]

In D137131#4009498, @ldionne wrote:

Requesting changes for the queue. Is this ready to be re-reviewed?

Yeah, I think this change is ready to be re-reviewed.

[mstorsjo]

In D137131#4009595, @rprichard wrote:

In D137131#4009498, @ldionne wrote:

Requesting changes for the queue. Is this ready to be re-reviewed?

Yeah, I think this change is ready to be re-reviewed.

IIRC you need to select the action "Request Review" again then (which is implied if you upload a new patch, although that's not needed here), for it to show up in the right queue for @ldionne to pick up on it again.

[rprichard]

Rebase this commit.

[rprichard]

Android apps aren't generally allowed to access /data/local/tmp, but they are able to access the app-specific cache dir. (/data/data/${APP_ID}/cache or /data/user/0/$APP_ID}/cache). New-enough versions of Android set TMPDIR to this cache dir for apps. I wonder if it'd make sense for libc++ to use the app cache as fallback for std::filesystem::temp_directory_path(), when running in an app context (app UID?). I suspect the answer is "no", because it's not feasible to determine the path to the app cache. Not sure though.

It's probably useful for std::filesystem::temp_directory_path() to work for the shell user even if it doesn't work for app users. That makes it useful for many test suites. I think this patch is an improvement as-is.

For libc++ filesystem testing, adb_run.py could set TMPDIR to /data/local/tmp. On new-enough versions of Android, that happens automatically in the initial adb shell environment, but su and run-as clear TMPDIR.

Bionic has similar behavior with respect to the temp dir. e.g. https://android-review.git.corp.google.com/c/platform/bionic/+/2545855

[enh]

i'd probably change the commit message to make it clear that Android's _global fallback_ temporary directory is /data/local/tmp. (for most apps, it's an app-specific directory that [on new enough versions of Android] you can get from $TMPDIR). but i see the _code_ works like that anyway --- prefer $TMPDIR, but fall back to /data/local/tmp. so, yes, that's correct. (i know nothing about Windows though :-) )

libcxx/src/filesystem/operations.cpp

#if defined(_LIBCPP_WIN32API)
   // GetTempPathW returns a path with a trailing slash, which we
   // shouldn't include for consistency.
   if (buf[retval-1] == L'\\')
     buf[retval-1] = L'\0';
   path p(buf);
 #else
   const char* env_paths[] = {"TMPDIR", "TMP", "TEMP", "TEMPDIR"};
   const char* ret = nullptr;
-
   for (auto& ep : env_paths)
     if ((ret = getenv(ep)))
       break;
-  if (ret == nullptr)
+  if (ret == nullptr) {
+#if defined(__ANDROID__)
+    ret = "/data/local/tmp";
+#else
     ret = "/tmp";
+#endif
+  }
 
   path p(ret);
 #endif
   error_code m_ec;
   file_status st = detail::posix_stat(p, &m_ec);
   if (!status_known(st))
     return err.report(m_ec, "cannot access path " PATH_CSTR_FMT, p.c_str());
 

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp

for (auto& TC : cases) {
         UnsetEnv(TC.name);
     }
     // No env variables are defined
     path fallback;
     {
         std::error_code ec = GetTestEC();
         path ret = temp_directory_path(ec);
         TEST_CHECK(!ec);
-#ifndef _WIN32
+#if defined(__ANDROID__)
+        TEST_CHECK(ret == "/data/local/tmp");
+#elif !defined(_WIN32)
         // On Windows, the function falls back to the Windows folder.
         TEST_CHECK(ret == "/tmp");
 #endif
         TEST_CHECK(is_directory(ret));
         fallback = ret;
     }
     for (auto& TC : ignored_cases) {
         // Check that certain variables are ignored
         PutEnv(TC.name, TC.p);
         std::error_code ec = GetTestEC();
         path ret = temp_directory_path(ec);
         TEST_CHECK(!ec);