Support embedded Swift#116
Conversation
| public func withUnsafeBytesOfValue<Result>( | ||
| _ body: (UnsafeBufferPointer<UInt8>) throws -> Result | ||
| ) rethrows -> Result { | ||
| public func withUnsafeBytesOfValue<Result, Failure: Error>( |
There was a problem hiding this comment.
Is this API breakage real?
There was a problem hiding this comment.
IIUC this is not source-breaking, but very likely ABI-breaking.
There was a problem hiding this comment.
Correct. This is considered fine.
| } | ||
| #else | ||
| final func withLock<Result, Failure: Error>(_ body: () throws(Failure) -> Result) throws(Failure) -> Result { | ||
| try body() |
There was a problem hiding this comment.
Is it OK to not lock on embedded?
There was a problem hiding this comment.
Not universally so, Wasm does have multi-threaded flavors, but it also has import Synchronization available in all flavors. For other embedded platforms it should be done on case-by-case basis.
There was a problem hiding this comment.
Embedded Swift status says Synchronization is available but only for atomics, not mutexes: https://docs.swift.org/embedded/documentation/embedded/status
I remember reading a pitch about SpinLock a while back.
There was a problem hiding this comment.
I don't think that document reflects the current state of things, at least for Wasm. import Synchronization is fully available in Embedded Swift for Wasm.
There was a problem hiding this comment.
Yeah we should really use Mutex here if possible
There was a problem hiding this comment.
It does appear to be WASM specific: https://github.com/swiftlang/swift/blob/40b64b63bdbc5ff4641e6a51270b58aee9a81d80/stdlib/public/Synchronization/CMakeLists.txt#L202
There was a problem hiding this comment.
This condition you referenced means that if arch is wasm32, then all Synchronization sources are built, and of non-wasm32 platforms only Atomic is enabled (since other Embedded platforms don't have a predefined libc, like WASI-libc). So it's ok to rely on Mutex here.
if("${arch}" MATCHES "wasm32")
list(APPEND SWIFT_SYNCHRONIZATION_EMBEDDED_SOURCES ${SWIFT_SYNCHRONIZATION_SOURCES})
list(APPEND SWIFT_SYNCHRONIZATION_EMBEDDED_SOURCES ${SWIFT_SYNCHRONIZATION_WASM_SOURCES})
else()
list(APPEND SWIFT_SYNCHRONIZATION_EMBEDDED_SOURCES ${SWIFT_SYNCHRONIZATION_ATOMIC_SOURCES})
endif()There was a problem hiding this comment.
As clarified in a comment above, that patch wasn't cherry-picked for 6.3 unfortunately, so you'll have to make it conditional on 6.4+ as I suggested there.
MaxDesiatov
left a comment
MaxDesiatov
left a comment
There was a problem hiding this comment.
It would be great for this PR or imminent followup PRs to also add Wasm (both embedded and non-embedded) jobs on CI to ensure no regressions are introduced in the future.
| name: WebAssembly Swift SDK | ||
| uses: apple/swift-nio/.github/workflows/wasm_swift_sdk.yml@main | ||
| with: | ||
| additional_command_arguments: "--target HTTPTypes" |
There was a problem hiding this comment.
Would you mind using this one instead? Otherwise we'll have replicate Embedded Swift builds in swift-nio workflows.
embedded-swift:
name: Build with Embedded Swift
uses: swiftlang/github-workflows/.github/workflows/swift_package_test.yml@0.0.9
with:
enable_linux_checks: false
enable_macos_checks: false
enable_windows_checks: false
enable_wasm_sdk_build: false
enable_embedded_wasm_sdk_build: true
swift_flags: --target HTTPTypes
| fatalError() | ||
| } | ||
| #else | ||
| #if !hasFeature(Embedded) || os(WASI) |
There was a problem hiding this comment.
Seems like Mutex is only available on WASI in main snapshots, but not 6.3 or 6.2:
| #if !hasFeature(Embedded) || os(WASI) | |
| #if !hasFeature(Embedded) || (os(WASI) && compiler(>=6.4)) |
3 participants
In order for swift-http-api-proposal to support embedded Swift, swift-http-types has to support embedded Swift first.
Is there a CI we can enable for embedded?