-
Notifications
You must be signed in to change notification settings - Fork 660
CI: Add build check for WebAssembly SDK #3159
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
base: main
Are you sure you want to change the base?
Conversation
43fc72b
to
738186e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We sadly cannot accept this PR at this time. Currently WASM is not an officially supported platform by Swift and there is no official toolchain for it. This is something for the Platforms Steering Group to address. Once both issues have been solved we would love to add CI for WASM. cc @al45tair
Thanks for sharing! That's a good motivation to make it official :) |
@@ -88,3 +88,8 @@ jobs: | |||
name: Static SDK | |||
# Workaround https://github.com/nektos/act/issues/1875 | |||
uses: apple/swift-nio/.github/workflows/static_sdk.yml@main | |||
|
|||
wasm-sdk: | |||
name: WebAssembly SDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We sadly cannot accept this PR at this time. Currently WASM is not an officially supported platform by Swift…
@FranzBusch Curious what your thoughts are about allowing wasm builds into swif-nio
, but as optional
actions rather than required
?
This would improve visibility on wasm breakages without imposing a requirement for official support. Right now, contributors are likely a bit blind to any wasm breakages. If there is a way to improve awareness without imposing extra burden on contributors, feels like a win-win.
I believe we could make it optional by adding the following config to wasm-sdk
here.
continue-on-error: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @MaxDesiatov in case you have any insight on this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FranzBusch Curious what your thoughts are about allowing wasm builds into
swif-nio
, but asoptional
actions rather thanrequired
?
I'm fully empathetic on making sure we are not breaking WASM support. However, at this point we just can't use an externally produced toolchain here. That's why I would like to push the Platforms Steering Group on considering WASM as an official platform so that we can start producing official toolchains that we can use in our CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like it is a requirement in Pull Requests.
It might not look like it, but actually the Linux PR tests do do some WASM testing, and it is required to pass.
@kateinoigakukun This is awesome, thanks so much! |
Add build check CI for WebAssembly SDK
Motivation:
It's very difficult to keep the code with WebAssembly/WASI compatible, and we need to apply some fixes sometimes #3156
Modifications:
Added a job to check if
swift build --swift-sdk wasm32-unknown-wasi
passes.Result:
Setting up the CI job makes it easier to detect such incompatibility before merging.