Add Windows to CI matrix - #2
Merged
Merged
Conversation
CMakeLists.txt: winflexbison3 (the Chocolatey package CI installs) provides win_bison.exe/win_flex.exe, not binaries literally named bison/flex -- a new WIN32 block points BISON_EXECUTABLE/FLEX_EXECUTABLE at those explicitly, mirroring the existing macOS Homebrew override right above it rather than relying on FindBISON/FindFLEX's own search to already know the Windows-specific name. ci.yml: adds windows-latest to the matrix, a winflexbison3 install step, and --config/-C Release on the build/test steps (a no-op on the single-config generators Linux/macOS use, but required on Windows's default multi-config Visual Studio generator, which ignores CMAKE_BUILD_TYPE entirely). Also bumps actions/checkout v4 -> v7 while here (v4 still targets node20, which GitHub Actions runners now flag for deprecation). This is the first time this codebase has ever been built with MSVC -- expect follow-up fixes once CI actually runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
winflexbison3's Chocolatey package is stuck on its 2021 release (bison
3.7.4), which fails parser.y's own `%require "3.8"` check
("require bison 3.8, but have 3.7.4", confirmed by the actual CI run).
windows-latest ships MSYS2 pre-installed at C:\msys64 with a current
bison (3.8.2); point CMake at those binaries directly, falling back to
win_bison/win_flex for a dev machine that has winflexbison instead.
Also bumps find_package(BISON ...)'s own minimum from 3.5 to 3.8 to
match the grammar's real requirement, so a too-old bison anywhere
fails at configure time with a clear message rather than a confusing
one from bison itself mid-build.
MSVC's cl.exe doesn't understand GCC/Clang-style -Wall/-Wextra flags
("invalid numeric argument '/Wextra'") -- gate them with a
CXX_COMPILER_ID generator expression so Linux/macOS keep the same
warnings unchanged and Windows just skips them (MSVC has its own
default warning level already).
Two genuine first-ever-MSVC compile errors, both from this being the parser's first Windows build: - parser.y: NodePtr (unique_ptr<ASTNode>) is move-only. Every action already moves it explicitly, which GCC/Clang tolerate, but MSVC's more eager instantiation of value_type::copy<T> (referenced by emplace<T> whether or not it's ever actually called) fails to compile for a non-copyable T. `%define api.value.automove` is Bison's own documented fix for move-only semantic value types. - lexer.l: flex's generated scanner includes <unistd.h> by default, which doesn't exist on Windows/MSVC. `%option nounistd` disables it; we never use the isatty()/POSIX-I/O codepath it guards anyway (never-interactive already opts out of interactive-terminal handling). Verified locally on macOS/Clang first: clean rebuild from scratch, full 626-test suite passes unchanged -- confirms automove didn't alter parse behavior on the platforms this was already working on.
Root-caused the "attempting to reference a deleted function" copy-ctor error that survived the api.value.automove fix: Bison's own lalr1.cc-skeleton stack-symbol move/copy constructor picks move vs. copy via a YY_MOVE_OR_COPY macro gated purely on `__cplusplus >= 201103L` (parser.tab.hpp) -- nothing to do with automove, which only affects user action code, not this internal stack-management path. MSVC famously always reports __cplusplus as 199711L for legacy-compatibility reasons regardless of the real /std: level unless /Zc:__cplusplus is explicitly passed, so Bison's generated code silently took the pre-C++11 copy-based branch and failed to compile for our move-only NodePtr semantic type. Confirmed by inspecting the actual generated parser.tab.cpp/.hpp locally (YY_MOVE_OR_COPY expands to `copy` under the pre-C++11 branch, `move` otherwise) rather than guessing from the error message alone.
SourceMap.ProcessIncludesSplicesFileContentInPlace failed only on windows-latest: the local writeFile() test helper (duplicated in test_source_map.cpp and test_comments_and_files.cpp) opened its ofstream in text mode, so on Windows every '\n' in a fixture's content silently became '\r\n' on disk. The production reader (source_map.cpp's processIncludes) opens included files in binary mode, so it never strips that '\r' back out -- a real, if latent, mismatch that Linux/macOS masked completely (text and binary mode are identical there) until this was the first-ever Windows test run. Fixed by writing the fixture in binary mode too, so what's on disk matches the literal string passed in on every platform.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
windows-latestto the CI matrix, alongside the existingubuntu-latest/macos-latest.CMakeLists.txt: pointsBISON_EXECUTABLE/FLEX_EXECUTABLEatwin_bison.exe/win_flex.exe(what thewinflexbison3Chocolatey package actually installs) onWIN32, mirroring the existing macOS Homebrew-bison override.--config/-C Releaseon the build/test steps: required for Windows's default multi-config Visual Studio generator (which ignoresCMAKE_BUILD_TYPE); harmless no-op on the single-config generators Linux/macOS use.actions/checkoutv4 → v7 (v4 still targets the now-deprecated Node 20 runtime).This is the first time this codebase has ever been built with MSVC — no local Windows environment to pre-verify against, so I expect this may need follow-up fixes once CI actually runs.
Test plan
🤖 Generated with Claude Code