-
Notifications
You must be signed in to change notification settings - Fork 66
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 CI #445
WASM CI #445
Conversation
…ite correctly setting -DBOOST_ROOT.
…at least one pthread symbol at link time
…riable to fix compilation
…nel' which will execute with emrun
…o get things working
… former does not fully support exceptions
Yes. There's just one - see executor_test.cpp, introduced with #306. |
.github/workflows/stlab.yml
Outdated
@@ -34,54 +34,98 @@ jobs: | |||
- name: Install dependencies (macos) | |||
if: ${{ startsWith(matrix.config.os, 'macos') }} | |||
run: | | |||
brew update | |||
brew install boost | |||
brew install ninja | |||
shell: bash | |||
|
|||
- name: Install dependencies (ubuntu) |
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.
Maybe call this "Install dependencies (Linux GCC/Clang)" and the new one "Install dependencies (Linux Webassembly)"? I'm going off the names in the matrix file. It'd be good if we could figure out something consistent.
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.
Yeah I also mulled this over and I'm craving consistency. I'll take another crack at it.
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.
Took a crack at it, not satisfied but it crossed my 80/20 threshold. Let me know what you think.
https://github.com/stlab/libraries/runs/7640371961?check_suite_focus=true
Co-authored-by: David Sankel <[email protected]>
…STLab.cmake per code review
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 did a white-glove review. This code is top quality. I'm approving, but there are a couple (minor) remaining issues.
- I made a handful of comment rewording and formatting suggestions.
- The top-level PR comment references
cmake/ExperimentalEmscripten.cmake
still. I didn't notice any other issues, but it'd probably be good for you to give it a once-over. I assume this PR title+comment is going to be the squash&merge commit message.
Co-authored-by: David Sankel <[email protected]>
Co-authored-by: David Sankel <[email protected]>
This PR adds CI support for Webassembly. It resolves #404, #405, and part of #406.
Note: 406 still requires unit tests for the WASM
main_executor
to be considered complete.This introduces a CMake toolchain file,
cmake/Platform/Emscripten-STLab.cmake
which extends the Emscripten SDK's own toolchain, and enables some experimental features required by STLab tests to compile and run successfully. Those features are threading support and exception handling.Following our existing convention, the tests may be executed simply with
ctest
from the build directory. A node runner is used.Note that Node 16.16.0 or newer is required for sufficient exception handling support. The new CMake toolchain will report an error if this requirement is not satisfied. To use an appropriate version, we must edit a configuration file
.emscripten
in the emsdk directory to point to the system installation of node (16.16.0 is installed for GH Actions).