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.

Sign up for GitHub

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 support for Android #183

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Add support for Android #183

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Sources/X509/Verifier/RFC5280/URIConstraints.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import Musl
import CoreFoundation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit confused by whether these imports need to be there at all, as I've been seeing changes in what the trunk compiler I build natively on Android expects here in the last month.

To compare to other platforms, I commented out the imports of CoreFoundation and the three linux C libraries in this file- Bionic, Musl, and Glibc- and tried building the latter two on Fedora 40, where both compiled fine without these imports (Musl was built with the latest July 2 SDK snapshot, Glibc starting from the latest trunk snapshot toolchain back to 5.9.2). This file uses the inet_pton() C function, which clearly comes from those C libraries.

The issue is that a trunk source snapshot of the compiler from June and July built natively on Android worked the same, ie these two imports were unnecessary, but an August trunk snapshot now requires import Android.

I don't know if something changed in the compiler's ClangImporter or LLVM in the last month, but the Android module map is identical from the July trunk snapshot to August.

Anyone know what is going on here, if not with my Android issue, at least why these imports seem unneeded for Glibc/Musl?

@ian-twilightcoder, @compnerd, @etcwilde, @al45tair, perhaps one of you knows what's going on with these overlay imports.

Copy link
Member

Choose a reason for hiding this comment

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

The glibc modulemap/header thing that we have today isn't properly modularizing glibc. Instead of using that mega header, each of the top-level C headers should end up in its own top-level module so if that header is seen through a transitive include, that's the module definition that will get used. I don't know what changed specifically, but what we have was technically undefined, so we're still in the wrong with glibc. It just means that something pulled in something from CoreFoundation that pulled in the necessary header before something else pulled in the bit of glibc/bionic that pulled in that same header, which ends up with CoreFoundation saying that it owns the canonical definition of that thing rather than the libc. We ran into this in corelibs-foundation as well: swiftlang/swift-corelibs-foundation#5005

The musl modulemap should be better behaved though as I seem to remember Alastair putting quite a lot of work to modularize it correctly. @ian-twilightcoder can probably give you more details though on how to proceed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a little experiment to see if these libc imports are needed at all, deleting every import Musl and import Glibc in the swift-nio repo and then trying to build with each libc on linux with the July 2 trunk snapshot toolchain, as that's the latest Musl SDK available.

The good news is that caused a bunch of errors, so I then put back each import one by one until I was left with this cleaned-up patch, ie it still built fine with these imports deleted, even running and passing all the same tests for the Glibc build:

diff --git a/Sources/NIOFileSystem/FileSystem.swift b/Sources/NIOFileSystem/FileSystem.swift
index 291102d0..d158c41d 100644
--- a/Sources/NIOFileSystem/FileSystem.swift
+++ b/Sources/NIOFileSystem/FileSystem.swift
@@ -21,10 +21,6 @@ import NIOPosix
 
 #if canImport(Darwin)
 import Darwin
-#elseif canImport(Glibc)
-import Glibc
-#elseif canImport(Musl)
-import Musl
 #elseif canImport(Bionic)
 import Bionic
 #endif
diff --git a/Sources/NIOFileSystem/FileSystemError+Syscall.swift b/Sources/NIOFileSystem/FileSystemError+Syscall.swift
index 633b6dc7..377203d3 100644
--- a/Sources/NIOFileSystem/FileSystemError+Syscall.swift
+++ b/Sources/NIOFileSystem/FileSystemError+Syscall.swift
@@ -17,10 +17,6 @@ import SystemPackage
 
 #if canImport(Darwin)
 import Darwin
-#elseif canImport(Glibc)
-import Glibc
-#elseif canImport(Musl)
-import Musl
 #endif
 
 extension FileSystemError {
diff --git a/Sources/NIOFileSystem/Internal/System Calls/FileDescriptor+Syscalls.swift b/Sources/NIOFileSystem/Internal/System Calls/FileDescriptor+Syscalls.swift
index 9212839c..be4ba011 100644
--- a/Sources/NIOFileSystem/Internal/System Calls/FileDescriptor+Syscalls.swift	
+++ b/Sources/NIOFileSystem/Internal/System Calls/FileDescriptor+Syscalls.swift	
@@ -18,11 +18,7 @@ import SystemPackage
 
 #if canImport(Darwin)
 import Darwin
-#elseif canImport(Glibc)
-import Glibc
-import CNIOLinux
-#elseif canImport(Musl)
-import Musl
+#elseif canImport(Glibc) || canImport(Musl)
 import CNIOLinux
 #elseif canImport(Bionic)
 import Bionic
diff --git a/Sources/NIOFileSystem/Internal/System Calls/Syscall.swift b/Sources/NIOFileSystem/Internal/System Calls/Syscall.swift
index 0a80b9f1..d738d856 100644
--- a/Sources/NIOFileSystem/Internal/System Calls/Syscall.swift	
+++ b/Sources/NIOFileSystem/Internal/System Calls/Syscall.swift	
@@ -18,11 +18,7 @@ import SystemPackage
 #if canImport(Darwin)
 import Darwin
 import CNIODarwin
-#elseif canImport(Glibc)
-import Glibc
-import CNIOLinux
-#elseif canImport(Musl)
-import Musl
+#elseif canImport(Glibc) || canImport(Musl)
 import CNIOLinux
 #elseif canImport(Bionic)
 import Bionic
diff --git a/Sources/NIOFileSystem/Internal/SystemFileHandle.swift b/Sources/NIOFileSystem/Internal/SystemFileHandle.swift
index 41406573..83cc013b 100644
--- a/Sources/NIOFileSystem/Internal/SystemFileHandle.swift
+++ b/Sources/NIOFileSystem/Internal/SystemFileHandle.swift
@@ -20,11 +20,7 @@ import NIOPosix
 
 #if canImport(Darwin)
 import Darwin
-#elseif canImport(Glibc)
-import Glibc
-import CNIOLinux
-#elseif canImport(Musl)
-import Musl
+#elseif canImport(Glibc) || canImport(Musl)
 import CNIOLinux
 #elseif canImport(Bionic)
 import Bionic
diff --git a/Sources/NIOPosix/VsockAddress.swift b/Sources/NIOPosix/VsockAddress.swift
index 94d0f797..e08725c3 100644
--- a/Sources/NIOPosix/VsockAddress.swift
+++ b/Sources/NIOPosix/VsockAddress.swift
@@ -17,11 +17,6 @@ import NIOCore
 #if canImport(Darwin)
 import CNIODarwin
 #elseif os(Linux) || os(Android)
-#if canImport(Glibc)
-import Glibc
-#elseif canImport(Musl)
-import Musl
-#endif
 import CNIOLinux
 #endif
 private let vsockUnimplemented = "VSOCK support is not implemented for this platform"

The bad news is that I don't know why some of these are not needed. The imports in NIOPosix/VsockAddress.swift aren't needed because one of the Glibc imports in that module is @_exported, so that appears to cover all files in that module. I presume the same for NIOFileSystem/Internal/SystemFileHandle.swift and NIOFileSystem/FileSystem.swift, which both explicitly import NIOPosix. The other three are a mystery.

Pinging @ian-twilightcoder, perhaps you know what's going on here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe this is related to the current leaks in the module system, such as for member declarations that is currently being fixed, so given Ian hasn't responded, I will just change this so the compiler won't error, and we can merge and then revisit it later, once we know the Swift module leaks have been fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing this is related to the clang re-export leak that Allan wrote up on the forum.

#elseif canImport(Darwin)
import Darwin
#elseif canImport(Android)
import Android
import CoreFoundation
#endif

extension NameConstraintsPolicy {
Expand Down