[android] Implement Process and modify TestProcess to pass. #2228

drodriguez · 2019-05-08T21:12:52Z

Until recent API levels, Android haven't provide posix_spawn and other
functions from that family, so Process was not going to work with the
branches used for other POSIX OS.

Instead of creating an specific path for Android, which will probably be
forgotten by other changes, all the POSIX OS will use private CF
functions that wrap the posix_spawn functions. For Android, we try to
figure out if posix_spawn exist, and use it in that case, but we
implement a subset of the posix_spawn family of functions for lower API
levels, using fork and exec underneath, and other functions from the
POSIX APIs. The already code in Process is only modified slightly to
adapt to the new names have to be modified.

The tests have to be modified slightly for working on Android. Many
changes are related to the environment needing the key LD_LIBRARY_PATH,
or the xdgTestHelper will not be able to start in some of the tests. The
other change is using NSTemporaryDirectory() instead of a hardcoded
/tmp, which doesn't exist in Android (the code also removes the last
slash, to match the original code expectations).

It also includes a small fix for when launch() is used and a error is
thrown which is not related to the current working directory. Before the
error was ignored silently, and now it will fatalError.

Other small improvement is reporting the actual line in the helper for
the returned POSIX error codes, instead of always reporting the line of
the helper itself.

Special thanks to @spevans because without the recent changes to use exclusively xdgTestHelper, the changes in the tests would have been more complicated.

drodriguez · 2019-05-08T21:14:57Z

@swift-ci please test

spevans · 2019-05-08T21:22:06Z

TestFoundation/TestProcess.swift

+        if let ldLibraryPath = ProcessInfo.processInfo.environment["LD_LIBRARY_PATH"] {
+            process.environment?["LD_LIBRARY_PATH"] = ldLibraryPath
+        }
+#endif
        process.launch()


Is the LD_LIBRARY_PATH still needed? When I updated xdgTestHelper to contain all of the utilities I updated the CMakeLists.txt to add an rpath to the build directory which should apply to both Linux and Android:
set(XDG_TEST_HELPER_RPATH -Xlinker;-rpath;-Xlinker;${CMAKE_CURRENT_BINARY_DIR})

This was needed because one of the tests clears the environment before calling xdgTestHelper --env and before adding in the rpath it was failing as the LD_LIBRARY_PATH was not set anymore.

${CMAKE_CURRENT_BINARY_DIR} is a path in the build machine, but it doesn’t mean anything when moving the executable to the device for testing. That's also why this pieces are restricted to Android (they should not be needed for other targets).

Foundation CMakeLists is not prepared to test in Android, so the testing has to done manually, and while I always use the same directory, I would prefer not to hardcode anything in the CMakeLists for Android (I have to figure out how to simplify testing for Android).

millenomi · 2019-05-10T17:22:09Z

Foundation/Process.swift

@@ -978,4 +979,127 @@ private func posix(_ code: Int32) {
    default: fatalError("POSIX command failed with error: \(code)")
    }
 }
+
+#if os(Android)
+// Android doesn't provide posix_spawn APIs until recent API level, so we cannot


Do you want to delegate to the real API if they exist?

I don’t know if one can do that.

The availability of the symbol depends on the Android API level. By default, if nobody sets a different __ANDROID_API__ while compiling, the latest API level is used. Swift sets API level 21, but Foundation doesn't set anything, so at the moment, Foundation is being compiled with access to every available symbol in the Android NDK (I'm not 100% sure about that, though, because I don’t know if the module cache remembers the setting from Swift). So the symbol will be available at compile time, but it might not be at runtime (and the binary will probably fail to start).

I don’t think #available/@available works for Android, which might be a good way, so I think this is the best I can come up now.

I think that it makes sense to pass along the __ANDROID_API__ when building for Android. Using posix_spawn is preferable to rolling our own.

I don't understand where you exactly want to pass __ANDROID_API__. Swift definitions are just booleans, and there's no support for something like #if __ANDROID_API__ >= 21, IIRC.

We might be able to do those checks in CoreFoundation, but again __ANDROID_API__ is not a runtime check, but a "minimum deployment target" compilation check. When compiling you are deciding which is the minimal API you want to deploy to. As the code is written, we are supporting API levels before 26. If I use posix_spawn (and link against it), then we are not supporting anything before level 26.

That's my understanding, unless someone has another theory.

millenomi · 2019-05-10T17:23:06Z

I'm okay with this patch otherwise.

Foundation/Process.swift

drodriguez · 2019-05-10T21:32:47Z

Added the feedback from Simon.

@swift-ci please test

finagolfin · 2019-05-15T18:12:03Z

Tested this with the 5/12 snapshot, Process tests all passing now, whereas before I had to put in more workarounds just to avoid those tests on Android AArch64:

diff --git a/TestFoundation/TestProcess.swift b/TestFoundation/TestProcess.swift
index f3ba630f..b38d8b26 100644
--- a/TestFoundation/TestProcess.swift
+++ b/TestFoundation/TestProcess.swift
@@ -514,6 +514,7 @@ private enum Error: Swift.Error {
     case InvalidEnvironmentVariable(String)
 }
 
+#if !os(Android)
 // Run xdgTestHelper, wait for 'Ready' from the sub-process, then signal a semaphore.
 // Read lines from a pipe and store in a queue.
 class _SignalHelperRunner {
@@ -591,7 +592,7 @@ class _SignalHelperRunner {
         return true
     }
 }
-
+#endif
 #if !os(Android)
 internal func runTask(_ arguments: [String], environment: [String: String]? = nil, currentDirectoryPath: String? = nil) throws -> (String, String) {
     let process = Process()
@@ -669,5 +670,9 @@ private func parseEnv(_ env: String) throws -> [String: String] {
     }
     return result
 }
+#else
+internal func runTask(_ arguments: [String], environment: [String: String]? = nil, currentDirectoryPath: String? = nil) throws -> (String, String) {
+return ("", "")
+}
 #endif

drodriguez · 2019-05-16T20:32:29Z

Rebased on top of master and removed the conflict. This change do not include any changes to the RPATH for Android (it still uses LD_LIBRARY_PATH tricks instead which should not affect other platforms).

@swift-ci please test

drodriguez · 2019-05-17T20:48:50Z

@swift-ci please test

compnerd · 2019-06-16T18:16:18Z

Foundation/Process.swift

@@ -978,4 +979,127 @@ private func posix(_ code: Int32) {
    default: fatalError("POSIX command failed with error: \(code)")
    }
 }
+
+#if os(Android)
+// Android doesn't provide posix_spawn APIs until recent API level, so we cannot


I think that it makes sense to pass along the __ANDROID_API__ when building for Android. Using posix_spawn is preferable to rolling our own.

Foundation/Process.swift

compnerd · 2019-06-16T18:19:16Z

Foundation/Process.swift

+
+    // Clean up the parent signal handlers
+    for idx in 1..<(NSIG as Int32) {
+        // Seems that SIGKILL/SIGSTOP are sometimes silently ignored, and


Odd, is that a documented issue on android?

Not that I know of. I found it doing trial and error. Some device was fine, some other was crashing here.

In https://pubs.opengroup.org/onlinepubs/009695399/functions/sigaction.html there's discussion that they should be ignored silently, but it also talks about SysV and BSD returning EINVAL. I think the problem is that, in the code I found for Bionic, it seems to delegate to Linux rt_sigaction, which is documented to fail for those two signals.

In any case, since SIGKILL and SIGSTOP cannot be ignored or caught, they are already set to SIG_DFL, which is what this code will try to do. Skipping them should not have ill effects.

drodriguez · 2019-06-17T21:55:42Z

@swift-ci please test linux platform

drodriguez · 2019-07-12T18:56:42Z

All right. I rewrote all the code in a way I hope matches the requests above. Instead always using a custom posix_spawn for Android, if the real one is available during runtime (checking with dlsym), it will be used instead. This meant a little bit of wrangling in the code for other POSIX OS, but the code path for them is mostly a thin wrapper around the real functions.

@compnerd: can you review if this is what you wanted?

Until recent API levels, Android haven't provide posix_spawn and other functions from that family, so Process was not going to work with the branches used for other POSIX OS. Instead of creating an specific path for Android, which will probably be forgotten by other changes, all the POSIX OS will use private CF functions that wrap the posix_spawn functions. For Android, we try to figure out if posix_spawn exist, and use it in that case, but we implement a subset of the posix_spawn family of functions for lower API levels, using fork and exec underneath, and other functions from the POSIX APIs. The already code in Process is only modified slightly to adapt to the new names have to be modified. The tests have to be modified slightly for working on Android. Many changes are related to the environment needing the key LD_LIBRARY_PATH, or the xdgTestHelper will not be able to start in some of the tests. The other change is using NSTemporaryDirectory() instead of a hardcoded `/tmp`, which doesn't exist in Android (the code also removes the last slash, to match the original code expectations). It also includes a small fix for when launch() is used and a error is thrown which is not related to the current working directory. Before the error was ignored silently, and now it will fatalError. Other small improvement is reporting the actual line in the helper for the returned POSIX error codes, instead of always reporting the line of the helper itself.

drodriguez · 2019-07-16T22:33:11Z

Rebased to avoid conflicts with master. Ready for review.

@swift-ci please test Linux platform

drodriguez · 2019-07-16T23:58:52Z

Failures in LLDB are unrelated.

@swift-ci please test Linux platform

drodriguez requested review from compnerd and spevans May 8, 2019 21:12

spevans reviewed May 8, 2019

View reviewed changes

millenomi reviewed May 10, 2019

View reviewed changes

spevans reviewed May 10, 2019

View reviewed changes

Foundation/Process.swift Outdated Show resolved Hide resolved

Foundation/Process.swift Outdated Show resolved Hide resolved

drodriguez force-pushed the android-process branch from 4f9a3bf to 4556534 Compare May 10, 2019 21:31

drodriguez force-pushed the android-process branch from 4556534 to 4793d93 Compare May 16, 2019 20:31

finagolfin mentioned this pull request Jun 16, 2019

Foundation: Process is unavailable on android #2356

Merged

compnerd reviewed Jun 16, 2019

View reviewed changes

drodriguez force-pushed the android-process branch from 4793d93 to 14aeae5 Compare June 17, 2019 21:55

drodriguez force-pushed the android-process branch from 14aeae5 to 9e365ad Compare July 12, 2019 18:50

drodriguez requested a review from compnerd July 15, 2019 23:03

drodriguez force-pushed the android-process branch from 9e365ad to 79019d2 Compare July 16, 2019 22:32

millenomi merged commit c5c35c1 into swiftlang:master Jul 23, 2019

drodriguez deleted the android-process branch July 23, 2019 22:58

finagolfin mentioned this pull request Jul 29, 2019

[android] Add support for natively building on Android swiftlang/swift#23208

Merged

ephemer mentioned this pull request Jul 31, 2019

Make Host NSUnimplemented for Android #2439

Closed

[android] Implement Process and modify TestProcess to pass. #2228

[android] Implement Process and modify TestProcess to pass. #2228

Uh oh!

Conversation

drodriguez commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguez commented May 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millenomi commented May 10, 2019

Uh oh!

Uh oh!

Uh oh!

drodriguez commented May 10, 2019

Uh oh!

finagolfin commented May 15, 2019

Uh oh!

drodriguez commented May 16, 2019

Uh oh!

drodriguez commented May 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drodriguez commented Jun 17, 2019

Uh oh!

drodriguez commented Jul 12, 2019

Uh oh!

drodriguez commented Jul 16, 2019

Uh oh!

drodriguez commented Jul 16, 2019

Uh oh!

Uh oh!

drodriguez commented May 8, 2019 •

edited

Loading