Build the new FileSystem module for Android #2660
Conversation
| options: options, | ||
| permissions: permissions, | ||
| executor: executor, | ||
| useTemporaryFileIfPossible: false |
There was a problem hiding this comment.
|
5.8 test run timed out for some reason, while nightly CI has been broken for awhile. |
| import Atomics | ||
| import NIOCore | ||
| @preconcurrency import SystemPackage | ||
| import class Foundation.FileManager |
There was a problem hiding this comment.
The NIOFilsystem module is intentionally not depending on Foundation. Is there a way to solve this without using Foundation here?
There was a problem hiding this comment.
I didn't know that, will revert this.
FranzBusch
left a comment
FranzBusch
left a comment
There was a problem hiding this comment.
LGTM but I would like @glbrntt to also take a quick look before we merge this
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Looks good, left a couple of nits.
| #if os(Android) | ||
| return Self.syncOpenWithMaterialization( |
There was a problem hiding this comment.
Rather than duping the whole call, can we conditionalise just useTemporaryFileIfPossible? i.e.
#if os(Android)
let useTemporaryFileIfPossible = false
#else
let useTemporaryFileIfPossible = true
#endif
Makes it a little bit easier to see what the differences are.| _ execute: @Sendable (SystemFileHandle) async throws -> Void | ||
| ) async throws { | ||
| let path = FilePath("/tmp/\(Self.temporaryFileName())") | ||
| let path = FilePath("\(FileManager.default.temporaryDirectory.path)/\(Self.temporaryFileName())") |
There was a problem hiding this comment.
Can we use FileSystem.shared.temporaryDirectory instead of FileManager?
There was a problem hiding this comment.
Unfortunately not, as that uses a hard-coded /tmp, which doesn't exist on Android. I had originally modified that method to use Foundation also, but removed it when asked above. I figured it was fine here in the tests, since they're already using Foundation, which is why I didn't have to add an import here.
There was a problem hiding this comment.
So FileSystem.shared.temporaryDirectory returns a bogus path on Android, we should fix that instead of resorting on FileManager here if possible. How does Foundation get the right tmp path on Android?
There was a problem hiding this comment.
we should fix that instead of resorting on FileManager here if possible.
Something wrong with using Foundation methods here in the tests? It is already a dependency.
How does Foundation get the right tmp path on Android?
The logic is a bit convoluted, checking TMPDIR in the Termux app that runs natively on Android and defaulting to /data/local/tmp/ otherwise. I figured it's better to just invoke that Foundation method here, as I've done for other NIO test functions before.
There was a problem hiding this comment.
Can we just default to "/data/local/tmp" on Android then? The comment suggests TMPDIR is rarely set, so this would at least produce a less wrong answer on Android than "/tmp".
Motivation: Get the new module building and tested on my Android CI again Modifications: - Add force unwraps where needed - Update C types and account for nullability annotations in NDK 26 - Use /data/local/tmp for the `temporaryDirectory` and invoke it in the tests, instead of /tmp directly - Don't copy over extended attributes - Android doesn't support hard-linking on most partitions, so don't use it for materializing temporary files Result: Everything works again on Android
|
@swift-server-bot test this please |
I added this changed function call for Android in #2660 earlier this year, because of [an incorrect Bionic function signature as explained recently](#3009 (comment)), but now that it has been worked around through a new API note as mentioned there, this change is no longer needed. I simply did not notice this earlier because it still compiles and merely gives a warning, which removing this call now silences.
3 participants
Motivation:
Get the new module building and tested on my Android CI again (I had to make a few extra modifications there to get this working with the older Android API 24 that I test on my CI)
Modifications:
temporaryDirectoryand invoke it in the tests, instead of /tmp directlyResult:
Everything works again on Android
@glbrntt, please review.