-
Notifications
You must be signed in to change notification settings - Fork 67
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
Changes from 37 commits
f90b548
de4845a
aaa92d3
5161d65
0f88917
00f62b5
a89d322
957bb11
4ba23e0
81c64d4
0609132
825b49d
c310b3d
b97db7b
2e6ae4b
f9afd6c
e7edcbf
c7b3a17
106334f
1b608d6
b7981a7
bea6cb5
8f96321
c560e8b
e47e234
7ed6666
a1ed4a0
8dd4bdc
552866f
f148360
271f872
c7baafe
d300ae8
8155eae
7f8fa1d
34b45ae
bbcbb82
2d7a78d
78310a8
bb1ae72
a02dc0e
05b3f4d
243a70b
c2c35af
f481a8f
a1aa05c
f0b95c2
ef13a38
e084ac7
28ae72d
d6c00fe
09505e0
227025f
3460534
da996f5
d83968e
76fa5fe
dffd22b
4c9e290
3bf3b5c
19c2a18
40024ad
4f89847
9778f92
a8351bb
9ffc1a9
88ebfa1
27cc788
dd8bb92
2aa73a3
a6514f6
9560cfd
d8dc072
6c3fdb7
760fb51
9a52929
9cf3d86
e809c54
5d8e0da
fd3d55c
362d148
cf994bb
b662df6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ jobs: | |
id: set-matrix | ||
# Note: The json in this variable must be a single line for parsing to succeed. | ||
run: echo "::set-output name=matrix::$(cat .github/matrix.json | scripts/flatten_json.py)" | ||
|
||
builds: | ||
needs: generate-matrix | ||
runs-on: ${{ matrix.config.os }} | ||
|
@@ -30,6 +30,14 @@ jobs: | |
|
||
steps: | ||
- uses: actions/checkout@v2 | ||
- uses: mymindstorm/setup-emsdk@v11 | ||
if: ${{ startsWith(matrix.config.compiler, 'emscripten') }} | ||
|
||
- name: Verify emsdk installation (emscripten) | ||
if: ${{ startsWith(matrix.config.compiler, 'emscripten') }} | ||
run: | | ||
which emcc | ||
emcc -v | ||
nickpdemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- name: Install dependencies (macos) | ||
if: ${{ startsWith(matrix.config.os, 'macos') }} | ||
|
@@ -39,7 +47,7 @@ jobs: | |
shell: bash | ||
|
||
- name: Install dependencies (ubuntu) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 |
||
if: ${{ startsWith(matrix.config.os, 'ubuntu') }} | ||
if: ${{ startsWith(matrix.config.os, 'ubuntu') && !startsWith(matrix.config.compiler, 'emscripten') }} | ||
run: | | ||
sudo apt-get install -y ninja-build | ||
sudo apt-get install -y libboost-all-dev | ||
|
@@ -52,6 +60,14 @@ jobs: | |
vcpkg install boost-test:x64-windows boost-multiprecision:x64-windows boost-variant:x64-windows | ||
shell: cmd | ||
|
||
- name: Install dependencies (emscripten ubuntu) | ||
if: ${{ startsWith(matrix.config.compiler, 'emscripten') }} | ||
shell: bash | ||
run: | | ||
sudo apt-get install -y ninja-build | ||
sudo apt update -y && sudo apt install firefox -y | ||
nickpdemarco marked this conversation as resolved.
Show resolved
Hide resolved
nickpdemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
git clone --depth 1 --recurse-submodules https://github.com/boostorg/boost.git $HOME/boost | ||
nickpdemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- name: Set enviroment variables (Linux+GCC) | ||
if: ${{ matrix.config.compiler == 'gcc' }} | ||
shell: bash | ||
|
@@ -66,13 +82,34 @@ jobs: | |
echo "CC=clang-${{matrix.config.version}}" >> $GITHUB_ENV | ||
echo "CXX=clang++-${{matrix.config.version}}" >> $GITHUB_ENV | ||
|
||
- name: Configure (Unix) | ||
if: ${{ startsWith(matrix.config.os, 'ubuntu') || startsWith(matrix.config.os, 'macos') }} | ||
- name: Compile Boost (Ubuntu emscripten) | ||
if: ${{ startsWith(matrix.config.compiler, 'emscripten') }} | ||
shell: bash | ||
run: | | ||
mkdir -p ../build-boost | ||
emcmake cmake -S $HOME/boost -B ../build-boost -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_STANDARD=23 \ | ||
-DCMAKE_CXX_FLAGS="-pthread" \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain why this is needed? My expectation is that if this flag is needed, it comes from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was also surprised this was needed. Without it, the boost sources are not passed
Subsequently, we fail to link with this error:
The first google for that error suggested a missing I do not know why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think some more investigation is warranted here. Adding this to the top-level of the Boost build may work, but it doesn't solve the problem in the right place. Perhaps the CMakeLists.txt in filesystem needs an adjustment. Did you figure out which piece of code is brining in the -pthread requirement? Also, I didn't see this in my own experiments. When in the build process are you seeing a linker error? |
||
-DCMAKE_INSTALL_PREFIX=`which emsdk`/../upstream/emscripten/cache/sysroot/$HOME/boost-release-emscripten-install \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed when reading through Another thing I noticed in that file is that you can use |
||
-DBOOST_EXCLUDE_LIBRARIES="asio;beast;context;coroutine;fiber;log" | ||
|
||
cmake --build ../build-boost | ||
cmake --install ../build-boost | ||
|
||
- name: Configure (Unix && !emscripten) | ||
if: ${{ (startsWith(matrix.config.os, 'ubuntu') || startsWith(matrix.config.os, 'macos')) && !startsWith(matrix.config.compiler, 'emscripten') }} | ||
shell: bash | ||
run: | | ||
mkdir ../build | ||
cmake -S. -B../build -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_STANDARD=23 | ||
|
||
- name: Configure (Ubuntu emscripten) | ||
if: ${{ startsWith(matrix.config.compiler, 'emscripten') }} | ||
shell: bash | ||
run: | | ||
mkdir ../build | ||
emcmake cmake -S. -B../build -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_STANDARD=23 \ | ||
-DBOOST_ROOT=$HOME/boost-release-emscripten-install | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per my above comment, I think BOOST_ROOT may not be needed here. |
||
|
||
- name: Configure (Windows) | ||
if: ${{ startsWith(matrix.config.os, 'windows') }} | ||
shell: cmd | ||
|
@@ -82,7 +119,7 @@ jobs: | |
cmake -S. -B../build -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake -DCMAKE_CXX_STANDARD=23 | ||
|
||
- name: Build (Unix) | ||
if: ${{ startsWith(matrix.config.os, 'ubuntu') || startsWith(matrix.config.os, 'macos') }} | ||
if: ${{ (startsWith(matrix.config.os, 'ubuntu') || startsWith(matrix.config.os, 'macos')) }} | ||
nickpdemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
shell: bash | ||
run: | | ||
cmake --build ../build/ | ||
|
@@ -95,7 +132,15 @@ jobs: | |
cmake --build ../build/ | ||
|
||
- name: Test | ||
if: ${{ !startsWith(matrix.config.compiler, 'emscripten') }} | ||
shell: bash | ||
run: | | ||
cd ../build/ | ||
ctest | ||
|
||
- name: Test (emscripten) | ||
if: ${{ startsWith(matrix.config.compiler, 'emscripten') }} | ||
shell: bash | ||
run: | | ||
find ../build/test -name "stlab.test.*.html" -exec \ | ||
emrun --browser_args="--headless" --browser firefox --kill_start --kill_exit {} \; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you consider using ctest with the default node-based runner for these tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I tried to get ctest to work with this, but I made little progress. First, we'd need to change the output type from If we make that change, and run ctest, we fail with the following:
That The culprit seems to be To the best of my knowledge, neither ctest nor node are supported environments for running code transpiled with emscripten. The only "running" documentation is here, which suggests the use of emrun. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. In that case I think going with your own toolchain makes sense. You can set CMAKE_CROSSCOMPILING_EMULATOR to an emrun launch. I don't think it is true that CTest isn't a supported environment from running code transpiled with emscripten. It is a general tool that can be used to run any cross-compiled code provided an appropriate emulator. Node must be a supported environment for at least some emscripten since that is what they explicitly set in |
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.
Did you consider using apt-get to install enscripten rather than take on this dependency on mymindstorm/setup-emsdk?
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.
Neither apt nor apt-get have an emscripten or emsdk package, but I've removed this dependency in favor of a manual installation of emsdk.
Notably, it requires we modify bash_profile, so you'll see all subsequent emscripten steps will use
shell: bash -l {0}
, in order to override GitHub Actions' default of a --noprofile shell. See here for 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.
I have it installed on my Ubuntu machine. See: https://packages.ubuntu.com/jammy/emscripten
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.
Those are considered unofficial packages by the Emscripten team, and are not officially supported. I've opted to stick strictly to their installation guidelines and clone/install their repo instead of adding the aforementioned dependency.