-
Notifications
You must be signed in to change notification settings - Fork 722
[wasm][1/2] Add reusable WebAssembly workflow #3331
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
[wasm][1/2] Add reusable WebAssembly workflow #3331
Conversation
|
This is potentially superseded by swiftlang/github-workflows#142 |
5e6da08 to
ebb34ad
Compare
|
We still need this separate PR to add a reusable workflow based on swiftlang/github-workflows#142 |
This is not suppressed by it because NIO and all other server repos are not using the workflows provided by the linked repo due to their limited capabilities. |
| swift_flags: --target NIOCore | ||
| swift_nightly_flags: --target NIOCore |
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.
FYI: Thanks to swiftlang/github-workflows#149, these two lines can be shortened into one line.
| swift_flags: --target NIOCore | |
| swift_nightly_flags: --target NIOCore | |
| wasm_sdk_build_command: swift build --target NIOCore |
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.
IIRC swift_nightly_flags in absence of a defined value is auto-populated from swift_flags, so the workflow here should just specify the latter without a need for wasm_sdk_build_command.
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.
@MaxDesiatov That's incorrect. swift_nightly_flags is not inherited from swift_flags.
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.
I may be missing something, but there's || inputs.swift_flags on the line you've linked to?
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.
Yes, I linked to the following line.
BUILD_FLAGS: ${{ (contains(matrix.swift_version, 'nightly') && inputs.swift_nightly_flags) || inputs.swift_flags }}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.
This is a ternary operator.
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.
I don't think so. If swift_nightly_flags is undefined, the first operand of || is falsy, which then evaluates to the second operand swift_flags. You can also see swift_flags respected for nightlies in this workflow, where swift_nightly_flags is not defined at all https://github.com/swiftlang/swift-for-wasm-examples/blob/main/.github/workflows/pull_request.yml
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.
Sorry, I was wrong. I didn't expect that the empty string was falsy. Thank you.
…3332) Add build check CI for WebAssembly SDK ### Motivation: Add build check CI for WebAssembly SDK ### Modifications: Add Wasm build checks in `main.yml` and `pull_request.yml` based on #3331 Extracted from #3159 ### Result: Setting up the CI job makes it easier to detect such incompatibility before merging.
Add build check CI for WebAssembly SDK
Motivation:
Add build check CI for WebAssembly SDK
Modifications:
Add only reusable GHA workflow from #3159 to gracefully merge changes without breaking CI.
After this change being merged into main branch, we can merge the actual PR checks: #3331
Result:
Setting up the CI job makes it easier to detect such incompatibility before merging.