Skip to content

Conversation

scottmarchant
Copy link
Contributor

@scottmarchant scottmarchant commented Mar 24, 2025

Fixes NIOCore compiler errors for the wasm32 architecture.

Motivation:

When compiling NIOCore for a Swift WebAssembly target, several compilation errors such as the following occur:

> swift build --swift-sdk wasm32-unknown-wasi --target NIOCore

.../swift-nio/Sources/NIOCore/FileHandle.swift:151:42: error: cannot find type 'TwoUInt32s' in scope
149 | public final class NIOFileHandle: FileDescriptor & Sendable {
150 |     private static let descriptorClosed: CInt = CInt.min
151 |     private let descriptor: UnsafeAtomic<TwoUInt32s>
    |                                          `- error: cannot find type 'TwoUInt32s' in scope
152 | 
153 |     public var isOpen: Bool {
.../swift-nio/Sources/NIOCore/FileHandle.swift:77:8: error: Unknown architecture
 75 | typealias TwoUInt32s = DoubleWord
 76 | #else
 77 | #error("Unknown architecture")
    |        `- error: Unknown architecture
 78 | #endif
 79 | 
 
…

The compilation errors were introduced with the following change:

https://github.com/apple/swift-nio/pull/2598/files#diff-a041d0c0d50a8f4fcda2020eeb4450ea994920ef61653cc06014457ccdb3ba0cR70

Modifications:

The fix is straight-forward and unlikely to affect any existing compilation targets other than the wasm32 target.

We simply need to add arch(wasm32) to compiler architecture condtions for a new typedef on line, then NIOCore compiles to wasm32 again.

Result:

Before: NIOCore wasm build fails ❌

After: NIOCore wasm build succeeds ✅

@scottmarchant scottmarchant marked this pull request as draft March 24, 2025 18:11
@scottmarchant scottmarchant marked this pull request as ready for review March 24, 2025 18:40
@scottmarchant
Copy link
Contributor Author

cc @MaxDesiatov @kateinoigakukun

@weissi weissi enabled auto-merge (squash) March 24, 2025 21:45
@weissi weissi added the 🔨 semver/patch No public API change. label Mar 24, 2025
@weissi
Copy link
Member

weissi commented Mar 24, 2025

Amazing, thanks so much and sorry for missing wasm!

}
}
}
#elseif arch(arm) || arch(i386) || arch(arm64_32)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this only matters the pointer size, _pointerBitWidth(_64) and _pointerBitWidth(_32) might be better choice

Copy link
Member

Choose a reason for hiding this comment

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

+1, otherwise it misses plenty of other 32-bit architectures that in theory could be supported in the future.

Copy link
Member

Choose a reason for hiding this comment

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

That's up to the NIO folks but

  • _pointerBitWidth* is underscored and we have non-underscored alternatives (but yes, they're annoying)
  • mixed platforms like arm64_32 need to fall into the correct place, which in this case would be true (assuming that _pointerBitWidth(_32) is true for arm64_32

Copy link
Contributor

Choose a reason for hiding this comment

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

My gut feeling is to avoid the underscored APIs. If it becomes necessary to support more 32-bit architectures in the future then we should push to get a non-underscored API.

Copy link
Contributor Author

@scottmarchant scottmarchant Mar 25, 2025

Choose a reason for hiding this comment

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

Sounds like the consensus is stick with || arch(wasm32). I'll leave as-is unless someone voice opinions otherwise. Happy to change this if desired, though.

I tried out adding || _pointerBitWidth(_32) instead of || arch(wasm32), and that also worked, just as a data point.

Copy link
Member

Choose a reason for hiding this comment

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

Friendly ping @glbrntt to approve & merge

@weissi weissi requested a review from Lukasa March 25, 2025 10:16
@scottmarchant
Copy link
Contributor Author

Amazing, thanks so much and sorry for missing wasm!

@weissi No worries at all - pretty hard to catch them all. I meant to mention, I was impressed with that nice #error("Unknown architecture") fail mode. Pointed me straight to the right spot and helped me figure out what to do.

auto-merge was automatically disabled March 25, 2025 17:45

Head branch was pushed to by a user without write access

@scottmarchant scottmarchant force-pushed the fix/fix-NIOCore-build-for-wasm32 branch from 339d6db to f2cddc5 Compare March 25, 2025 17:45
@weissi weissi enabled auto-merge (squash) March 26, 2025 14:43
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Thank you!

@weissi weissi merged commit 320fe4c into apple:main Mar 27, 2025
39 of 41 checks passed
@kateinoigakukun
Copy link
Contributor

FWIW I just opened a PR to set up CI for Wasm SDK #3159

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.

5 participants