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

Correct a few Musl additions from #2449 for Android, plus error if libc not found #2451

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

finagolfin
Copy link
Contributor

Motivation:

Fix build errors on Android

Modifications:

  • Fix previous Musl modifications that assumed Glibc wasn't imported on Android
  • Add errors for all libc imports, so new platform ports error out early
  • Update my github username

Result:

NIO builds natively on Android again, with all the same tests passing

…if libc not found

Motivation

Fix build errors on Android

Modifications

- Fix previous Musl modifications that assumed Glibc wasn't imported on Android
- Add errors for all libc imports, so new platform ports error out early

Result

NIO builds natively on Android again, with all the same tests passing
@@ -66,7 +66,7 @@ extension NIOBSDSocket.SocketType: Hashable {
extension NIOBSDSocket.SocketType {
/// Supports datagrams, which are connectionless, unreliable messages of a
/// fixed (typically small) maximum length.
#if canImport(Glibc)
#if os(Linux) && !canImport(Musl)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxDesiatov, I presume this was your intent when you changed these two lines in this file.

@@ -37,7 +37,7 @@ import CNIOWindows

internal typealias MMsgHdr = CNIOWindows_mmsghdr
#else
let badOS = { fatalError("unsupported OS") }()
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 have no idea why @weissi used this formulation instead of erroring, as I added now.

Copy link
Member

@weissi weissi Jun 24, 2023

Choose a reason for hiding this comment

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

That code was from before #error(...) got added to Swift (or at least at a time when NIO still supported Swift versions that didn't have #error).

@@ -111,7 +111,7 @@ private let sysIfNameToIndex: @convention(c) (UnsafePointer<CChar>?) -> CUnsigne
private let sysSocketpair: @convention(c) (CInt, CInt, CInt, UnsafeMutablePointer<CInt>?) -> CInt = socketpair
#endif

#if canImport(Glibc)
#if os(Linux) && !canImport(Musl)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxDesiatov, I don't know why you changed this for Musl, these functions aren't there? Because this change you made doesn't define them for Musl at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxDesiatov, do you define these for Musl? If so, feel free to add a commit to this pull with those Musl defintions, and we can get that fix in too.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I’m generally happy with this but would like @MaxDesiatov to confirm as well.

Copy link
Member

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

LGTM

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Jun 26, 2023
@Lukasa
Copy link
Contributor

Lukasa commented Jun 26, 2023

@swift-server-bot add to allowlist

@Lukasa Lukasa enabled auto-merge (squash) June 26, 2023 09:28
@Lukasa Lukasa merged commit 231902a into apple:main Jun 26, 2023
@finagolfin finagolfin deleted the libc branch June 26, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants