-
Notifications
You must be signed in to change notification settings - Fork 164
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
[Android] Enable more code and tests #871
Conversation
@@ -173,7 +173,7 @@ struct TimeZoneCache : Sendable { | |||
} | |||
} | |||
|
|||
#if os(Linux) && !os(WASI) | |||
#if os(Linux) |
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.
@kateinoigakukun, I assume the os
can't be both Linux
and WASI
?
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.
Right, just a os(Linux)
should be enough.
Updated to allow reading from zero-sized system files on Android too and imported the Android overlay in a test file that now needs it and rebased, tested with the Sep. 4 trunk source snapshot natively on Android without a problem. I was hoping to get confirmation from TBC guys, who are preparing their own Android SDK, but no word from them for the last month on this pull, so might as well move ahead. @parkera, just need someone to review and get this in, then I'd like to get this into release/6.0 next. Should be perfectly safe as this only affects Android, touches no code on any other platform. |
@swift-ci test |
@@ -23,6 +23,10 @@ import TestSupport | |||
@testable import Foundation | |||
#endif | |||
|
|||
#if canImport(Android) | |||
import Android | |||
#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'm curious why we need this import here.
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 is the compilation error I get without it:
Tests/FoundationEssentialsTests/FileManager/FileManagerTests.swift:622:15: error: cannot find 'getuid' in scope
620 | func testFileAccessAtPath() throws {
621 | #if !os(Windows)
622 | guard getuid() != 0 else {
| `- error: cannot find 'getuid' in scope
There are some strange issues with these platform overlays, which I investigated a bit last month. Since nobody I asked seems to have a good idea of what's going on, I chalked it up to some Swift module leak that can be fixed later and am just adding these imports now to shut the compiler up.
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 fine to me, but would like a second opinion from @jmschonfeld
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.
Overall LGTM, but a few small comments
Tests/FoundationEssentialsTests/FileManager/FileManagerTests.swift
Outdated
Show resolved
Hide resolved
while disabling setting extended file attributes and a test creating a hard link, features not normally allowed on Android.
Addressed all review feedback and updated, one last CI run and this can go in. |
@swift-ci please test |
Passed CI, ready for merge. I will submit for 6.0 next. |
* [Android] Enable more code and tests while disabling setting extended file attributes and a test creating a hard link, features not normally allowed on Android. * Remove incorrect WASI check
* [Android] Enable more code and tests while disabling setting extended file attributes and a test creating a hard link, features not normally allowed on Android. * Remove incorrect WASI check
while disabling setting extended file attributes and a test creating a hard link, features not normally allowed on Android.
This got all the tests passing natively on Android 13 AArch64.
@hyp and @compnerd, try it out yourself and please review.