add Android shims for spawnattr#5301
Conversation
|
@swift-ci please smoke test |
parkera
left a comment
There was a problem hiding this comment.
This is probably fine, but should we be looking at improving swift-subprocess instead, or in addition to this?
|
@parkera This PR is mostly motivated by being able to lower the recent Android SDK to API 24 instead of 28, which would currently fail to compile, because of these missing symbols on API 27 and below. I don't think people are going to use Process or subprocess a lot on Android, but at least this is better than removing it by "!os(Android)" You can read more about this discussion here: https://forums.swift.org/t/android-api-minimum-for-the-swift-sdk-for-android/82874 |
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
@ktoso I am just trying to get building in my fork with Github Actions and the Android SDK, so no need to run the smoke tests. But thanks! |
|
I see lol, I'll stop then :-) |
|
So after a lot of debugging, I also found that the existing polyfills did not work on API 27 devices and below. After fixing those, I built a API 24 SDK with this PR, and the below code now correctly runs with no errors on both a API 24 device and API 28 @main
struct MyExecutable {
static func main() {
let executablePath = "/system/bin/ls"
let arguments = ["-l"]
let process = Process()
process.executableURL = URL(fileURLWithPath: executablePath)
process.arguments = arguments
let pipe = Pipe()
process.standardOutput = pipe
process.standardError = pipe
do {
try process.run()
let data = pipe.fileHandleForReading.readDataToEndOfFile()
if let output = String(data: data, encoding: .utf8) {
print("--- Subprocess Output ---")
print(output)
print("-------------------------")
}
process.waitUntilExit()
print("Process exited with status: \(process.terminationStatus)")
} catch {
print("Failed to spawn process: \(error)")
}
}
} |
|
I applied this pull natively on Android, where I periodically run these Foundation tests, enabled the polyfills on newer Android, and ran the tests: no regressions, so this appears to work. @drodriguez, could you take a look after the winter break, since you wrote a lot of these polyfills? |
drodriguez
left a comment
There was a problem hiding this comment.
Looks fine to me, with that small feedback.
I cannot vouch for the correctness of the implementation matching the POSIX documentation/Bionic particulars, but it looks fine from a quick read.
|
@swift-ci test |
|
@compnerd and @Steelskin, this pull touches code that affected your TBC CI in the past, so pinging you guys again to see what you think, ie if this passes your CI. @swift-ci test |
Our CI is currently broken due to a SIL verification failure. You can follow here: swiftlang/swift#86288 |
|
@Steelskin, the linked issue was fixed last week, is the TBC CI still broken? If not, can you run this through it? |
My apologies, we had a few other failures in between but we should be back to green now. I am starting a new build right now. |
|
No apology needed, as I know what it's like when the CI starts failing 😉 and was fine with waiting, then saw that Saleem mentioned failures again for the swift-driver pull, so I figured more was going on and simply checked with you again now. Can you run that swift-driver pull through again separately? I think he is worried it may not still pass, as it did when you tried with your TBC CI a couple months ago. |
|
@finagolfin This passed on our CI, including the Android smoke test. |
|
Alright, this passed all Android-related CI, just need some Foundation reviewer like @jmschonfeld to approve and we'll get it in. |
|
@iCharlesHu is a good Foundation reviewer to take a look at this one |
|
@iCharlesHu Any chance you could take a look at this soon? This is the only thing blocking us from building an API 24 SDK instead of 28. And we would like to get this in before 6.3 and have some time for testing as well. |
|
Going ahead and merging since most of this pull only affects Android, that too only when compiled against a pre-28 Android API, with the only change to cross-platform code coming at the call sites and cleaning up errors a bit in All the CI runs show that this pull works fine on all platforms, but just asked the Foundation team for review so they can sign off, but Charles has a history of not responding for months at a time, swiftlang/swift-foundation-icu#53, so better to get the Foundation team's input, if any, on the 6.3 cherry-pick of this pull that Mads just submitted, #5398. |
Adds shims for the posix_spawnattr added in #2928 for Android 27 and below.