Add Windows to CI matrix - #6
Merged
Merged
Conversation
…CI merge Updates the openscad_cpp_parser submodule pointer to the just-merged PR #2 (BelfrySCAD/openscad_cpp_parser@35d9957), which added and fixed its own Windows CI (MSYS2-sourced bison/flex, /Zc:__cplusplus for Bison's move-only-variant codegen, a Windows-only test fixture bug). This repo's own CI matrix gains windows-latest, sourcing bison/flex from MSYS2 the same way. Two of this repo's own build files needed the identical MSVC fixes already applied in the submodule: - src/CMakeLists.txt: -Wall/-Wextra gated behind a non-MSVC generator expression (MSVC's cl.exe doesn't understand GCC-style warning flags). - CMakeLists.txt (root): /Zc:__cplusplus for MSVC, since this project's own targets (and its FetchContent deps -- nlohmann_json, Manifold, Boost.Polygon) are a separate directory scope from the submodule's own copy of the same fix and don't inherit it. fail-fast: false added to the matrix so a Windows-specific failure during this initial rollout doesn't cancel the already-established Linux/macOS jobs mid-run. Manifold/Boost.Polygon's own Windows buildability under MSVC is not yet verified in this project's specific FetchContent-pinned configuration -- expect further MSVC-specific fixes from the actual CI run, same iterative pattern used to land the submodule's own Windows CI.
Two genuine bugs, both from this being the evaluator's first-ever MSVC build: - scope_trail.hpp used std::out_of_range without #include <stdexcept> -- masked on Clang/GCC by transitive coverage through some other standard header, same class of gap as this session's earlier Linux-build header sweep. - M_PI (used in src/builtins/roof.cpp and src/import/svg_import.cpp) is a POSIX <cmath> extension Unix libcs define unconditionally; MSVC only defines it when _USE_MATH_DEFINES is set before <cmath>'s first include. Fixed as a single MSVC-only compile definition in the root CMakeLists.txt rather than patching each call site, so a future M_PI use anywhere in this project doesn't reintroduce the same gap. (The svg_import.cpp "const object must be initialized"/ "no matching std::max overload" errors in the same CI log were cascading fallout from the undeclared M_PI, not separate bugs -- confirmed by inspecting the actual flagged expressions.) Verified locally on macOS/Clang first: clean rebuild, full 1190-test suite passes unchanged.
…ures Manifold's own CMakeLists.txt defaults BUILD_SHARED_LIBS ON, building manifold.dll/manifoldc.dll under build/_deps/manifold-build/ instead of static libs. Harmless on Linux/macOS -- CMake gives build-tree executables an automatic rpath to a build-tree .so/.dylib -- but Windows has no rpath equivalent, so every test/CLI executable failed to even launch (STATUS_DLL_NOT_FOUND, surfaced as CMake's gtest_discover_tests failing at build time with exit code 0xc0000135) since the DLLs live in a different directory than the .exe files and nothing copies them over or adds that directory to PATH. Forcing BUILD_SHARED_LIBS off before fetching Manifold (matching the existing MANIFOLD_TEST override right above it) is simpler and more robust than a Windows-only DLL-copy CI step, and fixes this for good across every executable this project builds (CLI, tests, examples), not just the one gtest_discover_tests happened to catch first. Verified locally on macOS/Clang: clean rebuild produces no .dylib/.so anywhere in the build tree (confirms the static-forcing actually took effect), full 1190-test suite passes unchanged.
Two genuine test-portability bugs, both invisible until this repo's first Windows CI run: - test_export.cpp hardcoded "/tmp/..." paths (a Unix-only directory) for four fixtures, unlike every other test file here, which already uses std::filesystem::temp_directory_path() (test_cli.cpp, test_dxf_svg_import.cpp, test_surface.cpp, test_zip_stored.cpp, test_use.cpp, test_import_export.cpp). Brought it in line with that existing pattern rather than inventing a new one. - test_cli.cpp's CliDebugRepl.SetOverridesVariableOnResume opened an ifstream on the exported output file to verify its contents, then called std::filesystem::remove() on that same file later in the same scope while the ifstream was still open (its destructor doesn't run until the end of the TEST body). POSIX allows deleting an open file (unlink just removes the directory entry); Windows enforces an exclusive lock and the remove() throws. Fixed with an explicit in.close() right after the last read. Verified locally on macOS/Clang: full rebuild, full 1190-test suite passes unchanged.
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 this repo's own CI matrix, alongside the existing ubuntu-latest/macos-latest jobs.openscad_cpp_parsersubmodule pointer to its own just-merged Windows CI fix (Add Windows to CI matrix openscad_cpp_parser#2).-Wall/-Wextragated behind a non-MSVC check, and/Zc:__cplusplusfor MSVC's__cplusplusmisreporting (this repo's own targets are a separate CMake directory scope from the submodule's copy of the fix).Test plan
ctestunaffected by these changes (both are no-ops on non-MSVC).🤖 Generated with Claude Code