-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add android support, that uses new swift-foundation for Android #5024
base: main
Are you sure you want to change the base?
Conversation
The newer level introduces APIs with additional nullability attribution. This causes issues as the attribution sometimes collides with expectations. Unwrap more cases to appease the nullability attributes. One problematic area of this change is the handling for `Process.run()`. The posix spawn APIs are described as: ``` typedef struct __posix_spawnattr* posix_spawnattr_t; int posix_spawnattr_init(posix_spawnattr_t _Nonnull * _Nonnull __attr); ``` As the inner `_Nonnull` appertains to `posix_spawnattr_t`, it expects a non-null pointer to a non-null pointer. However, as `struct __posix_spawnattr` is opaque, we cannot allocate space for it and thus must be acceptable to take a pointer to null to permit the allocation.
This allows us to prevent the ambiguity for various STATX defines from linux/stat.h when building for Android, as those are defined in the Android platform module
@swift-ci please test |
Supersedes #4994 |
CC @parkera |
@hyp Android support is not part of Swift 6, correct? |
41466f4
to
94dc57f
Compare
This depends on swiftlang/swift-foundation#714 now. |
@parkera Sorry for the late reply! Android overlay support is included in Swift 6, so this should be taken into Swift 6 as well. |
@swift-ci please test |
|
||
switch Int32(current.pointee.fts_info) { | ||
case FTS_D: | ||
let (showFile, skipDescendants) = match(filename: filename, to: _options, isDir: true) | ||
if skipDescendants { | ||
fts_set(_stream, _current, FTS_SKIP) | ||
fts_set(stream, _current!, FTS_SKIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use current
instead.
@swift-ci please test |
@@ -19,6 +19,8 @@ import FoundationNetworking | |||
#endif | |||
#if os(Windows) | |||
import WinSDK | |||
#elseif os(Android) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
canImport(Android)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I modified this in my pull hyp#1 instead.
Which swift-foundation change does this rely on now? |
None, as the one he opened is now obsolete. However, I have proposed further changes to this pull in hyp#1 and haven't gotten any response. |
@compnerd can you help with this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me!
#if TARGET_OS_ANDROID | ||
#define HAVE_STRLCPY 1 | ||
#define HAVE_STRLCAT 1 | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should consider doing this via the build configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've been doing this in the headers because build configuration is spread across several systems (cmake, SPM), and they do not all support the same features.
Requires swiftlang/swift-foundation#757