This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rprichard on Oct 31 2022, 3:56 PM.

Details

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.

Diff Detail

Event Timeline

rprichard created this revision.Oct 31 2022, 3:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 3:56 PM
rprichard requested review of this revision.Oct 31 2022, 3:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 3:56 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Nov 1 2022, 9:02 AM
ldionne added inline comments.
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp
139–142

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

This revision now requires changes to proceed.Nov 1 2022, 9:02 AM
rprichard updated this revision to Diff 480266.Dec 5 2022, 3:50 PM

Re-enable the test for Android.

rprichard updated this revision to Diff 480278.Dec 5 2022, 4:29 PM

Enable the test for Windows.

Adding @mstorsjo for the Windows part of the change.

rprichard retitled this revision from [libc++][Android] temp dir is /data/local/tmp not /tmp to [libc++] Android temp dir is /data/local/tmp, enable Windows test.Dec 5 2022, 4:32 PM
rprichard edited the summary of this revision. (Show Details)

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
144

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 added inline comments.Dec 7 2022, 10:36 AM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp
144

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 added inline comments.Dec 7 2022, 12:37 PM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp
144

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 requested changes to this revision.Dec 20 2022, 4:44 PM

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

This revision now requires changes to proceed.Dec 20 2022, 4:44 PM

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

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

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 requested review of this revision.Dec 21 2022, 2:14 PM
rprichard updated this revision to Diff 528668.Jun 5 2023, 8:26 PM
rprichard edited the summary of this revision. (Show Details)

Rebase this commit.

rprichard added a subscriber: enh.Jun 5 2023, 10:19 PM

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 accepted this revision.Jun 7 2023, 1:09 PM

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 :-) )

rprichard edited the summary of this revision. (Show Details)Jun 7 2023, 4:30 PM
rprichard marked an inline comment as done and an inline comment as not done.Jun 8 2023, 12:52 PM
ldionne accepted this revision.Jun 12 2023, 8:31 AM
This revision is now accepted and ready to land.Jun 12 2023, 8:31 AM