-
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
Conversation
@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.
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.
This change can still be made.
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.
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.
Instead of setting this separately here, can you just add TARGET_OS_ANDROID
to the list below? That's how I do it on my CI.
Ping @hyp, I've been using a version of this pull along with my own modifications on my Android CI. I think we should go ahead with some version of this. If you're unable to work on this pull further, I can rebase this pull, add my own mods, and submit it in another pull. Let us know what you'd like to do. |
Working on merging this in |
@swift-ci please test |
testing a merge first, need to rebase it again though |
5ccdee3
to
5f40cce
Compare
@swift-ci please test |
@swift-ci please test |
@swift-ci please test windows platform |
The windows host builds with the old driver for now, so it doesn't yet support wmo
@swift-ci please test |
There was a change missing related to turning off WMO for a windows host but now it's fixed too. Should be ready to merge (pending windows CI too 🤞 ) |
@swift-ci please test windows platform |
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.
Mostly good, with some small tweaks pointed out.
#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.
Instead of setting this separately here, can you just add TARGET_OS_ANDROID
to the list below? That's how I do it on my CI.
@@ -940,6 +942,13 @@ open class Process: NSObject, @unchecked Sendable { | |||
var spawnAttrs: posix_spawnattr_t? = nil | |||
#else | |||
var spawnAttrs: posix_spawnattr_t = posix_spawnattr_t() | |||
#endif | |||
#if os(Android) | |||
guard var spawnAttrs else { |
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.
This doesn't make any sense, as it is set to nil
just 5 lines above, so this causes this code to always fail.
Maybe you needed this guard
in some previous iteration of this patch, but it is no longer needed.
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.
Thanks, you're correct! It looks like I can't get rid of it yet, but the nullability on the NDK is broken for posix_spawnattr_init
. I will follow-up with a nullability annotation change and with a change that removes that then.
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.
OK, let me know when you get that annotation change up, as I will need it too.
I will follow this pull up with my own changes later today, mostly minor stuff like importing Bionic
instead where possible.
@@ -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.
This change can still be made.
@swift-ci please test |
@swift-ci please test windows platform |
de4bb96
to
1da58eb
Compare
@swift-ci please test |
@swift-ci please test windows platform |
1 similar comment
@swift-ci please test windows platform |
Great to see this land! @finagolfin, does this mean that we'll be able to drop swift-android.patch for trunk? Or will it need to be split up into separate patches, since the modifications to, e.g., |
The latter, because this is applied to all three Swift 6 release branches on my CI and this pull only got it into trunk. |
Requires swiftlang/swift-foundation#757