TestFoundation: change CWD to TMPDIR and restore previous CWD when test finishes

[finagolfin]

The working directory was changed to /data/local/tmp on Android assuming that adb is being used, but this isn't necessary in the Termux app. Additionally the working directory was not reset back, so later tests were affected by the change.

Along with the still-unmerged #2145, this finally gets all the TestFoundation tests passing for me on Android.

Pinging @drodriguez, who did a lot of this Android porting work.

[drodriguez]

Does Termux sets a TMPDIR that's writtable? It would be better to use that and not hard coded special paths for one "distribution". TMPDIR in any Android should point to a writable directory (or one can ask that to people that execute the test in their devices). I am sure that Termux can be execute with an environment variable pointing to the right place.

[finagolfin]

Does Termux sets a TMPDIR that's writtable?

Yes, I've modified a libdispatch test before to use it, swiftlang/swift-corelibs-libdispatch@06e4da811.

It would be better to use that and not hard coded special paths for one "distribution". TMPDIR in any Android should point to a writable directory (or one can ask that to people that execute the test in their devices). I am sure that Termux can be execute with an environment variable pointing to the right place.

Well, after this change, there's no hard-coded path used in Termux: I'm just not changing the working directory to /data/local/tmp on Termux. Moving to that directory surprisingly makes no difference to all subsequent tests but one, TestFileHandle.test_readToEndOfFileAndNotify_readError(), which I mentioned before causes TestFoundation to segfault in Termux, turns out because of this one prior line.

I'm unsure exactly why this change of directory to /data/local/tmp was made before, so I just avoid it in Termux, by checking to see if the PREFIX variable is set and that it's the Termux app. I can check for some other environment variable or use TMPDIR instead if you let me know what exactly you need on your end, as on my end, I'm simply avoiding this unnecessary change of directory to /data/local/tmp.

[spevans]

If /data/local/tmp doesnt exist with Termux, will this cause issues for NSTemporaryDirectory() which return /data/local/tmp on Android?

[finagolfin]

/data/local/tmp exists, but Android app users are limited in how they can use it, eg NativeScript/android#828. Adb doesn't have the same limitations.

As for NSTemporaryDirectory(), Termux sets TMPDIR to a path in the app, so I think Foundation picks that up.

[drodriguez]

I am not the original author of the changes, but it looks to me that TestURL needed a current working directory that was writable. For Android, using ADB, using /data/local/tmp seems like a good idea.

As I tried to explain (badly) in my previous comment, I would love to not have "flavors" of Android in the code, so finding a solution that works for everyone would be good. My proposal was changing the current code for something like:

#if os(Android)
FileManager.default.changeCurrentDirectoryPath(NSTemporaryDirectory())
#endif

Which should work for people using ADB to test and for people coding directly in their devices (like Termux, but maybe also solutions like Crostini or GNURoot), and does not mention any of the flavors by name anywhere.

Additionally, from the clue that a test in TestURL affects TestFileHandle.test_readToEndOfFileAndNotify_readError it is obvious that this change leaks into the later tests. Reading the code is obvious why, because the the changeCurrentDirectoryPath is not reverted as it is in TestFileManager.test_creatingDirectoryWithShortIntermediatePath, for example. The code of this test is actually quite unfriendly with static globals used all over the place. One solution will be writing a better setup_test_paths that undoes the changes (or a sibling function that undoes the changes).

@spevans, since you are already here: #2521 should also avoid the environment of one test method from leaking into the next, and the problem that we are experiencing here. I didn't received any concern or thumbs up about merging that one.

[finagolfin]

it is obvious that this change leaks into the later tests.

I was unsure if this was intentional for Android/adb or not, ie if it wasn't reverted because several other tests needed that directory change too on Android with adb.

I'll look into whether this change is even necessary anymore, but I don't use adb, so you'll have to try any pull I come up with on Android/adb and let me know.

[spevans]

If termux sets TMPDIR and NSTemporaryDirectory picks that up, could we just change this code to be:

FileManager.default.changeCurrentDirectoryPath(NSTemporaryDirectory()) and even drop the #if os(Android) conditional ?

I was thinking about how we use temporary directories in other tests and there is inconsistent setup code uses all over the place. I think in a future PR we could change it to something like:

Add the following into TestUtils.swift

func withTemporaryDirectory<T>(block: (_ dir: String, _ url: URL) throws -> T) throws -> T {
    let tmpDir = NSTemporaryDirectory() + "testFoundation/" + NSUUID().uuidString
    let fm = FileManager.default
    try? fm.removeItem(atPath: tmpDir)
    try fm.createDirectory(atPath: tmpDir, withIntermediateDirectories: true, attributes: nil)
    defer { try fm.removeItem(atPath: tmpDir) }
    let tmpURL = URL(fileURLWithPath: tmpDir, isDirectory: true)
    return try block(tmpDir, tmpURL)
}

and then each test that needs a temporary directory can simply do:

try withTemporaryDirectory { (dir, url) in
    print("dir:", dir)
    print("url:", url)
}

[finagolfin]

could we just change this code to be: FileManager.default.changeCurrentDirectoryPath(NSTemporaryDirectory()) and even drop the #if os(Android) conditional ?

I went further and just dropped the path change completely, as that was part of a really old pull, #1189, and may not be needed anymore. If I keep it and simply change the setup for TestFileHandle instead, so that it doesn't try to open a file handle to the working directory, ie /data/local/tmp:

diff --git a/TestFoundation/TestFileHandle.swift b/TestFoundation/TestFileHandle.swift
index 9932d3a0..8d264c52 100644
--- a/TestFoundation/TestFileHandle.swift
+++ b/TestFoundation/TestFileHandle.swift
@@ -146,7 +146,7 @@ class TestFileHandle : XCTestCase {
         }
         let fh = FileHandle(fileDescriptor: fd, closeOnDealloc: true)
 #else
-        let fd = open(".", O_RDONLY)
+        let fd = open(NSTemporaryDirectory(), O_RDONLY)
         expectTrue(fd > 0, "We must be able to open a fd to the current directory (.)")
         let fh = FileHandle(fileDescriptor: fd, closeOnDealloc: true)
 #endif

then the tests all pass for me again, even though I can't write to /data/local/tmp from Termux.

I think in a future PR we could change it to something like: Add the following into TestUtils.swift

Sounds like a good idea, but there's only two other tests where the current directory is changed to a valid path, the one Daniel mentioned above that is reset and this seemingly spurious one in TestProcess.test_run(). Everywhere else, the tests seem to properly call NSTemporaryDirectory() instead, as in TestFileHandle.createTemporaryFile.

@drodriguez, let me know if this pull now causes any problems for you in Android with adb, and if the pasted patch above or other patches are needed. By simply removing this directory change, it should shake out what other tests were inadvertently relying on it in Android/adb.

[spevans]

@buttaface So are you saying the tests pass on termux with this PR (which removes the chdir) and the above patch which uses let fd = open(NSTemporaryDirectory(), O_RDONLY) instead of let fd = open(".", O_RDONLY) ?

I dont see an issue with changing the filehandle test as it still opening a directory so future reads fail. So just to confirm, under Termux, /data/local/tmp exists but is not writeable? and let fd = open(".", O_RDONLY) fails because the current directory is /data/local/tmp ?

[finagolfin]

I'm saying the tests pass with either this pull alone or the TestFileHandle change alone in Termux, or obviously with both. But I'm unsure what would be needed in Android/adb after this pull, so Daniel will need to tell us.

under Termux, /data/local/tmp exists but is not writeable? and let fd = open(".", O_RDONLY) fails because the current directory is /data/local/tmp ?

Yes and yes.

[drodriguez]

I will need to check again, but I am afraid that the current working directory for a command line invocation (like TestFoundation) through adb will be the root directory / (it is not setup in Foundation, but one can see the usual way, which is just invoking adb shell /path/to/executable like in https://github.com/apple/swift/blob/1010d1e1a3d517af755f24011217c46f5c3bf32f/utils/android/adb/commands.py#L31-L39).

The problem with the current working directory being / is that many of the calculations in this file relies on two things: not having a current working directory path ending in / and having some more content than a path separator.

One can check that this also happens in other Unix systems by cding into / and running the TestFoundation.TestURL/test_fileURLWithPath from there.

I am afraid that the trick needs to stay for most adb Android setups (and hopefully it doesn't affect a lot other Android setups like Termux).

[finagolfin]

I am afraid that the current working directory for a command line invocation (like TestFoundation) through adb will be the root directory /

Are you sure? At least in the case of that Python script, it runs executables in /data/local/tmp, so that should be the working directory already.

The problem with the current working directory being / is that many of the calculations in this file relies on two things

If you're worried about it possibly being / under some other Android/adb invocations, I can change it to NSTemporaryDirectory() in TestURL and then reset it like in TestFileManager.test_creatingDirectoryWithShortIntermediatePath when these two TestURL tests are done, which would mean /data/local/tmp might not be the working directory for the rest of the tests in TestFoundation, depending on the Android/adb invocation.

Let me know what you prefer.

[drodriguez]

Are you sure? At least in the case of that Python script, it runs executables in /data/local/tmp, so that should be the working directory already.

Yes, the executables are created in subdirectories of /data/local/tmp, but the scripts do not change the current working directory, and the final invocation is something like adb shell /data/local/tmp/[uuid]/__executable_with_arguments. The current working directory for that is /, not where the executable sits.

If you're worried about it possibly being / under some other Android/adb invocations, I can change it to NSTemporaryDirectory() in TestURL and then reset it like in TestFileManager.test_creatingDirectoryWithShortIntermediatePath when these two TestURL tests are done, which would mean /data/local/tmp might not be the working directory for the rest of the tests in TestFoundation, depending on the Android/adb invocation.

If you don’t mind doing the necessary changes, I would prefer that solution, and I would be glad to review.

[finagolfin]

If you don’t mind doing the necessary changes, I would prefer that solution

Done.

[finagolfin]

Not necessary since it's reset in the next test, but keeps these tests independent so if someone shuffles the order of the tests someday, that won't matter.

[drodriguez]

I would recommend putting the added lines into a defer block just after the call to setup_test_paths, but I think this will work in most cases.

@swift-ci please test Linux platform

[finagolfin]

I would recommend putting the added lines into a defer block just after the call to setup_test_paths

Done, CI failed at git checkout because of unrelated ICU issue.

[spevans]

@swift-ci test linux

[drodriguez]

@swift-ci please test Linux platform

[drodriguez]

Updated the title and the content to reflect better the final solution. Merging.