Skip to content

fix: Fix NIOCore build for wasm targets#3156

Merged
weissi merged 1 commit intoapple:mainfrom
scottmarchant:fix/fix-NIOCore-build-for-wasm32
Mar 27, 2025
Merged

fix: Fix NIOCore build for wasm targets#3156
weissi merged 1 commit intoapple:mainfrom
scottmarchant:fix/fix-NIOCore-build-for-wasm32

Conversation

@scottmarchant
Copy link
Copy Markdown
Contributor

@scottmarchant scottmarchant commented Mar 24, 2025 •
edited
Loading

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
Copy Markdown
Contributor Author

scottmarchant commented Mar 24, 2025

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
Copy Markdown
Member

weissi commented Mar 24, 2025

Amazing, thanks so much and sorry for missing wasm!

kateinoigakukun
kateinoigakukun reviewed Mar 25, 2025
View reviewed changes
Sources/NIOCore/FileHandle.swift
}
}
}
#elseif arch(arm) || arch(i386) || arch(arm64_32)
Copy link
Copy Markdown
Contributor

@kateinoigakukun kateinoigakukun Mar 25, 2025

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
Copy Markdown
Member

@MaxDesiatov MaxDesiatov Mar 25, 2025

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
Copy Markdown
Member

@weissi weissi Mar 25, 2025

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
Copy Markdown
Contributor

@glbrntt glbrntt Mar 25, 2025

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
Copy Markdown
Contributor Author

@scottmarchant scottmarchant Mar 25, 2025 •
edited
Loading

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
Copy Markdown
Member

@weissi weissi Mar 26, 2025

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
Copy Markdown
Contributor Author

scottmarchant commented Mar 25, 2025

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
glbrntt
glbrntt approved these changes Mar 27, 2025
View reviewed changes
Copy link
Copy Markdown
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 kateinoigakukun mentioned this pull request Mar 27, 2025
Closed
@kateinoigakukun
Copy link
Copy Markdown
Contributor

kateinoigakukun commented Mar 27, 2025

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

@scottmarchant scottmarchant mentioned this pull request Jun 18, 2025
Open
68 tasks
@scottmarchant scottmarchant deleted the fix/fix-NIOCore-build-for-wasm32 branch June 20, 2025 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weissi weissi weissi left review comments

@glbrntt glbrntt glbrntt approved these changes

@Lukasa Lukasa Awaiting requested review from Lukasa

+2 more reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov left review comments

@kateinoigakukun kateinoigakukun kateinoigakukun approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🔨 semver/patch No public API change.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@scottmarchant @weissi @kateinoigakukun @MaxDesiatov @glbrntt