Skip to content

ci: modernize and fix GitHub Actions workflow #2717

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.

Sign up for GitHub

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

Open
kevmoo wants to merge 5 commits into
base: main
Choose a base branch
from
+41 −41
Open

ci: modernize and fix GitHub Actions workflow #2717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0685ee1
ci: modernize and fix GitHub Actions workflow
kevmoo Mar 14, 2026
ea995cd
Fix CI failures introduced in PR 2717
kevmoo Mar 14, 2026
7860412
ci: Fix wasm2c MSVC test failures and bump GitHub actions
kevmoo Mar 14, 2026
ff81e6f
ci: add /FS msvc flag for concurrent wasm2c tests
kevmoo Mar 15, 2026
db3f22e
Merge remote-tracking branch 'origin/main' into build_cleanup
kevmoo Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 31 additions & 39 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ jobs:
name: lint
runs-on: ubuntu-latest
steps:
- uses: actions/setup-python@v1
- uses: actions/setup-python@v6
with:
python-version: '3.x'
- uses: actions/checkout@v1
- uses: actions/checkout@v6
with:
fetch-depth: 0
Copy link
Copy Markdown
Member

@sbc100 sbc100 Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need fetch-depth: 0 for some reason? IIRC this is slower since it fetches the whole history

Copy link
Copy Markdown
Contributor Author

@kevmoo kevmoo Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fetch-depth: 0 setting was necessary because the lint job relies on the

./scripts/clang-format-diff.sh script (called on line 27 of build.yml).

By default, the actions/checkout step only performs a shallow fetch of the repository (fetching just a single commit, i.e., depth=1).

However, the clang-format-diff.sh script uses Git commands that need to inspect your repository's history to figure out what changed so it only formats modified code:

# Finds the common ancestor commit
MERGE_BASE=$(git merge-base $BRANCH HEAD)
# Runs the formatter only on the diff against the merge base
git clang-format $MERGE_BASE -q --diff -- src/

Without the full Git history fetched (fetch-depth: 0), git merge-base and git clang-format wouldn't be able to find the common ancestor commit locally, causing the CI lint step to fail.

Copy link
Copy Markdown
Contributor Author

@kevmoo kevmoo Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally wrote that 😛

Copy link
Copy Markdown
Member

@sbc100 sbc100 Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, of course, this is only the lint step, that makes sense.

Why did the v1 version not need this? Did it fetch all the history by default maybe?

Copy link
Copy Markdown
Contributor Author

@kevmoo kevmoo Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v1 did the full checkout. v6 is being efficient. But too efficient in this case.

- name: install tools
run: |
pip3 install flake8==7.3.0
sudo apt-get install clang-format
sudo apt-get install -y clang-format
- run: flake8
- run: ./scripts/clang-format-diff.sh
env:
Expand All @@ -32,35 +34,29 @@ jobs:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
steps:
- uses: actions/setup-python@v5
- uses: actions/setup-python@v6
with:
python-version: '3.x'
- uses: actions/checkout@v1
- uses: actions/checkout@v6
with:
submodules: true
- name: install ninja (linux)
run: sudo apt-get install ninja-build
run: sudo apt-get install -y ninja-build
if: matrix.os == 'ubuntu-latest'
- name: install ninja (osx)
run: brew install ninja
if: matrix.os == 'macos-latest'
- name: install ninja (win)
run: choco install ninja
if: matrix.os == 'windows-latest'
- name: mkdir
run: mkdir -p out
- uses: ilammy/msvc-dev-cmd@v1
if: matrix.os == 'windows-latest'
Copy link
Copy Markdown
Member

@sbc100 sbc100 Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this new external dependency doing?

Copy link
Copy Markdown
Contributor Author

@kevmoo kevmoo Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That step (the ilammy/msvc-dev-cmd@v1 action) activates the Microsoft Visual C++ (MSVC) Developer Command Prompt environment on the windows-latest GitHub Actions runner.

It was introduced in the very same commit (ea995cd92) you asked about previously, which fixed Windows CI failures.

Here's why it was needed:
The WABT build uses the Ninja build system across all platforms (cmake -G Ninja). On Windows runners, GitHub Actions has multiple compilers installed (both MSVC and MinGW/GCC). If you run CMake with Ninja without explicitly setting up the MSVC environment, CMake might default to using the MinGW gcc compiler instead of Microsoft's cl.exe, or it might fail entirely to find the C/C++ compiler.

By calling - uses: ilammy/msvc-dev-cmd@v1, it injects all the environment variables (like PATH, LIB, and INCLUDE) required for CMake and Ninja to locate and use cl.exe + link.exe correctly for the native Windows build.

Copy link
Copy Markdown
Contributor Author

@kevmoo kevmoo Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also revert the bits here that move the Windows build to use Ninja, but it seemed nice to have everything using ninja 🤷

Copy link
Copy Markdown
Contributor Author

@kevmoo kevmoo Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to add more notes so there are bread crumbs if that's helpful – in the actual source

Copy link
Copy Markdown
Member

@sbc100 sbc100 Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split out the windows->ninja parts? We can discuss those more in a separate PR. The other changes here seem trivially good.

- name: tool versions
run: |
cmake --version
ninja --version
- name: cmake
run: cmake .. -G Ninja -DWERROR=ON -Werror=dev -DCMAKE_ERROR_DEPRECATED=OFF
working-directory: out
if: matrix.os != 'windows-latest'
- name: cmake (windows)
run: cmake .. -DWERROR=ON -Werror=dev -DCMAKE_ERROR_DEPRECATED=OFF
working-directory: out
if: matrix.os == 'windows-latest'
run: cmake -S . -B out -G Ninja -DWERROR=ON -Werror=dev -DCMAKE_ERROR_DEPRECATED=OFF
- name: build
run: cmake --build out
- name: check if generated files are up-to-date
Expand All @@ -75,7 +71,7 @@ jobs:
name: emscripten
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1
- uses: actions/checkout@v6
with:
submodules: true
- name: build
Expand All @@ -89,7 +85,7 @@ jobs:
name: wasi
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v6
with:
submodules: true
- name: build-tools
Expand All @@ -113,13 +109,13 @@ jobs:
sanitizer: [asan, ubsan, fuzz]
type: [debug, release]
steps:
- uses: actions/setup-python@v1
- uses: actions/setup-python@v6
with:
python-version: '3.x'
- uses: actions/checkout@v1
- uses: actions/checkout@v6
with:
submodules: true
- run: sudo apt-get install ninja-build
- run: sudo apt-get install -y ninja-build
- name: workaround for ASLR+ASAN compatibility # See https://github.com/actions/runner/issues/3207
run: sudo sysctl -w vm.mmap_rnd_bits=28
- run: make clang-${{ matrix.type }}-${{ matrix.sanitizer }}
Expand All @@ -133,13 +129,13 @@ jobs:
CC: "clang" # used by the wasm2c tests
WASM2C_CFLAGS: "-march=x86-64-v2 -fsanitize=address -DWASM_RT_USE_MMAP=0"
steps:
- uses: actions/setup-python@v1
- uses: actions/setup-python@v6
with:
python-version: '3.x'
- uses: actions/checkout@v1
- uses: actions/checkout@v6
with:
submodules: true
- run: sudo apt-get install ninja-build
- run: sudo apt-get install -y ninja-build
- name: workaround for ASLR+ASAN compatibility # See https://github.com/actions/runner/issues/3207
run: sudo sysctl -w vm.mmap_rnd_bits=28
- run: make clang-debug-asan
Expand All @@ -149,14 +145,14 @@ jobs:
name: min-cmake
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1
- uses: actions/checkout@v6
with:
submodules: true
- uses: actions/setup-python@v1
- uses: actions/setup-python@v6
with:
python-version: '3.x'
- name: Install Ninja
run: sudo apt-get install ninja-build
run: sudo apt-get install -y ninja-build
- name: Detect minimum CMake version
run: >
awk 'match($0, /cmake_minimum_required\(VERSION *([0-9]+\.[0-9]+)\)/, a)
Expand Down Expand Up @@ -209,20 +205,20 @@ jobs:
WASM2C_CC: "clang"
WASM2C_CFLAGS: "-DWASM_RT_USE_MMAP=1 -DWASM_RT_SKIP_SIGNAL_RECOVERY=1 -DWASM_RT_NONCONFORMING_UNCHECKED_STACK_EXHAUSTION=1 -DWASM2C_TEST_EMBEDDER_SIGNAL_HANDLING -DWASM_RT_ALLOW_SEGUE=1 -DWASM_RT_SEGUE_FREE_SEGMENT=1 -mfsgsbase -DWASM_RT_SANITY_CHECKS=1 -Wno-pass-failed"
steps:
- uses: actions/setup-python@v1
- uses: actions/setup-python@v6
with:
python-version: '3.x'
- uses: actions/checkout@v1
- uses: actions/checkout@v6
with:
submodules: true
- run: sudo apt-get install ninja-build
- run: sudo apt-get install -y ninja-build
- run: make clang-debug
- name: tests (wasm2c tests excluding memory64)
run: ./test/run-tests.py wasm2c --exclude-dir memory64

build-cross:
runs-on: ubuntu-latest
# Temporatily disabled until we can get it fixed:
# Temporarily disabled until we can get it fixed:
# https://github.com/WebAssembly/wabt/issues/2655
if: ${{ false }}
strategy:
Expand All @@ -238,10 +234,10 @@ jobs:
env:
QEMU_LD_PREFIX: /usr/${{matrix.arch}}-linux-gnu/
steps:
- uses: actions/setup-python@v1
- uses: actions/setup-python@v6
with:
python-version: '3.x'
- uses: actions/checkout@v1
- uses: actions/checkout@v6
with:
submodules: true
- name: Set up QEMU
Expand All @@ -250,13 +246,9 @@ jobs:
platforms: ${{matrix.arch}}
image: "tonistiigi/binfmt:master"
- name: apt-get update
run: sudo apt-get update
- name: install ninja
run: sudo apt-get install ninja-build
- name: install the toolchain
run: sudo apt-get install g++-${{matrix.arch}}-linux-gnu
- name: install distcc
run: sudo apt-get install distcc
run: sudo apt-get update -y
- name: install dependencies
run: sudo apt-get install -y ninja-build g++-${{matrix.arch}}-linux-gnu distcc
- name: mkdir distcc symlinks
run: sudo mkdir -p /opt/bin/distcc_symlinks
- name: distcc symlink
Expand Down
12 changes: 10 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,11 @@ endif ()

if (BUILD_TESTS)
if (CMAKE_BUILD_TYPE STREQUAL "Debug")
set(WASM2C_CFLAGS "${WASM2C_CFLAGS} -g -O0")
if (COMPILER_IS_MSVC)
set(WASM2C_CFLAGS "${WASM2C_CFLAGS} /Zi /Od /FS")
else ()
set(WASM2C_CFLAGS "${WASM2C_CFLAGS} -g -O0")
endif ()
endif ()

if (WABT_BIG_ENDIAN)
Expand Down Expand Up @@ -758,7 +762,11 @@ if (BUILD_TESTS)
if (COMPILER_IS_MSVC)
set_target_properties(${EXENAME} PROPERTIES COMPILE_FLAGS "-wd4311")
else ()
set_target_properties(${EXENAME} PROPERTIES COMPILE_FLAGS "-std=gnu11 -Wno-pointer-to-int-cast")
set(C_API_FLAGS "-std=gnu11 -Wno-pointer-to-int-cast")
if (COMPILER_IS_CLANG)
set(C_API_FLAGS "${C_API_FLAGS} -Wno-gnu-folding-constant")
endif ()
set_target_properties(${EXENAME} PROPERTIES COMPILE_FLAGS "${C_API_FLAGS}")
endif ()

target_link_libraries(${EXENAME} wasm Threads::Threads)
Expand Down
Loading