Skip to content
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

Fix the Android build #492

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Fix the Android build #492

merged 1 commit into from
Nov 21, 2024

Conversation

jakepetroules
Copy link
Contributor

No description provided.

@@ -328,6 +328,18 @@ extension SystemError: CustomStringConvertible {
while cap <= 16 * 1024 {
var buf = [Int8](repeating: 0, count: cap)
let err = TSCLibc.strerror_r(errno, &buf, buf.count)
#if os(Android)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@finagolfin Does this look reasonable?

@jakepetroules
Copy link
Contributor Author

@swift-ci please smoke test

@finagolfin
Copy link
Member

What is this about, are you getting compilation issues all of a sudden? Because my Android CI just cross-compiled the latest Nov. 14 trunk snapshot of SwiftPM and TSC yesterday without a problem, after applying these basic patches that were mostly just upstreamed with your approval.

I didn't know what to make of that linked Android overlay patch for TSC, as I didn't need it when building natively on Android last month or when cross-compiling the latest Termux 6.1 toolchain.

@@ -328,6 +328,18 @@ extension SystemError: CustomStringConvertible {
while cap <= 16 * 1024 {
var buf = [Int8](repeating: 0, count: cap)
let err = TSCLibc.strerror_r(errno, &buf, buf.count)
#if os(Android)
if String(cString: err) == strerror(EINVAL) {
return "Unknown error \(errno)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an unknown error or is it as the name suggests, an invalid parameter?

cap *= 2
continue
}
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does if let err seem better?

@jakepetroules
Copy link
Contributor Author

What is this about, are you getting compilation issues all of a sudden? Because my Android CI just cross-compiled the latest Nov. 14 trunk snapshot of SwiftPM and TSC yesterday without a problem, after applying these basic patches that were mostly just upstreamed with your approval.

I didn't know what to make of that linked Android overlay patch for TSC, as I didn't need it when building natively on Android last month or when cross-compiling the latest Termux 6.1 toolchain.

Yeah, Android builds are using the GNU-specific definition of strerror_r so I got a compilation failure here.

This was with the Swift 6.0.2 release and your Android SDK from https://github.com/finagolfin/swift-android-sdk/releases/tag/6.0.2, with top-of-tree main branch of SwiftTSC.

Using:

/Library/Developer/Toolchains/swift-6.0.2-RELEASE.xctoolchain/usr/bin/swift build --swift-sdk aarch64-unknown-linux-android24

@finagolfin
Copy link
Member

Android builds are using the GNU-specific definition of strerror_r so I got a compilation failure here.

OK, this is a well-known issue with the Android build of this repo, which is remedied by passing in -Xswiftc -Xcc -Xswiftc -U_GNU_SOURCE to SwiftPM, as you can see in the CMake config here too.

Unfortunately, while this SwiftPM package manifest can add such defines easily, it could not undefine them without resorting to unsafe flags: maybe that should be fixed in SwiftPM.

After adding those flags and my linked Android overlay patch for this repo, I was able to cross-compile this repo for Android, then adding --build-tests -Xlinker -landroid-spawn allowed me to cross-compile its tests too (Android API 24 did not support the posix_spawn APIs yet, so I ship a compatibility library in my SDK bundle for that).

If you have any ideas on how to better handle these matters, I'm all ears.

@jakepetroules
Copy link
Contributor Author

Do you know where that _GNU_SOURCE definition is originating from?

@jakepetroules
Copy link
Contributor Author

I reverted the misc.swift changes for now, but I think we do still need the added import in Tracing.swift as I get a failure to find getpid otherwise.

@jakepetroules
Copy link
Contributor Author

@swift-ci please smoke test

@finagolfin
Copy link
Member

Do you know where that _GNU_SOURCE definition is originating from?

The Swift compiler started defining it for Android for some extra math function declarations six years ago. We briefly discussed removing it years ago, but figured there were likely already Swift codebases relying on it, so left it in.

Copy link
Member

@finagolfin finagolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I confirmed that this fixed cross-compiling for Android.

@jakepetroules jakepetroules requested a review from owenv November 20, 2024 05:33
@jakepetroules
Copy link
Contributor Author

@swift-ci please smoke test

@jakepetroules jakepetroules enabled auto-merge (rebase) November 20, 2024 05:33
@owenv
Copy link
Contributor

owenv commented Nov 20, 2024

Looks reasonable, especially now that it's just adding the missing import

@jakepetroules
Copy link
Contributor Author

@swift-ci please test macOS

@jakepetroules
Copy link
Contributor Author

@swift-ci please test Linux

@jakepetroules jakepetroules merged commit b464fcd into main Nov 21, 2024
3 checks passed
@jakepetroules jakepetroules deleted the android branch November 21, 2024 07:29
@jakepetroules
Copy link
Contributor Author

Here's a better attempt at the strerror thing which allows the package build to work regardless of whether _GNU_SOURCE is given on the command line: #497

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants