Test Dab 0.0.3 on Windows and macOS - #14
Conversation
There was a problem hiding this comment.
Pull request overview
Expands Dab’s CI gate to validate the existing Ruby/native build contract on GitHub-hosted macOS and Windows runners, while advancing the root VERSION to 0.0.3 and making small cross-platform build/test harness adjustments needed for those environments.
Changes:
- Bump the root
VERSIONto0.0.3and relax the VERSION spec to validate a numeric semver format rather than a pinned value. - Add macOS and Windows GitHub Actions jobs (keeping the existing Linux Ruby matrix intact).
- Adjust build/test plumbing for Windows/macOS (DLL extension handling, Premake platform define, MSBuild platform name, and portable file moves).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
VERSION |
Advances authoritative project version to 0.0.3. |
spec/compiler/version_spec.rb |
Updates version contract to semver-format validation while still requiring tool output to match VERSION. |
.github/workflows/ruby.yml |
Adds macOS and Windows inherited-gate CI jobs alongside the existing Linux matrix. |
Rakefile |
Updates binary/library naming for Windows, fixes MSBuild platform target, and replaces shell mv with FileUtils.mv. |
premake5.lua |
Defines DAB_PLATFORM_WINDOWS on Windows and restricts dl linking to Linux. |
src/frontend/frontend.rb |
Replaces {$OS_DYLIB_EXT} with dll/so/dylib depending on OS. |
src/cvm/syscalls.cpp |
Minor adjustment in the Windows dlimport unsupported path (hunk touched). |
test/dab/0244_ffi_simple.dabt |
Skips Windows for FFI fixture (FFI remains unsupported on Windows). |
test/dab/0245_ffi_arg.dabt |
Skips Windows for FFI fixture (FFI remains unsupported on Windows). |
Comments suppressed due to low confidence (1)
src/cvm/syscalls.cpp:117
- The Windows stub uses an
if (true)block, which is redundant and makes the unsupported-path harder to read. You can simplify to a singlethrow(and optionally capitalize the message consistently) without changing behavior.
{
throw DabRuntimeError("function import not supported on windows yet");
}
#endif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add native GitHub-hosted CI jobs using the pinned Ruby and Premake baselines while preserving the Linux matrix and PR-only policy. Repair the narrow cross-platform build contracts needed by the inherited gate, retain the explicit Windows FFI limitation, and advance the authoritative version to 0.0.3.
Use the runner-provided macOS formatter and Windows UCRT GNU toolchain so the inherited gate keeps its fatal-warning policy. Correct Windows output discovery and remove the stale compile-only reference from the existing unsupported FFI branch.
Pass the library path directly to dlopen so the intentionally unsupported Windows branch does not retain an unused native pointer under fatal warnings.
a4e0cf9 to
059f3f4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/cvm/syscalls.cpp:116
- On Windows builds (with DAB_PLATFORM_WINDOWS defined and fatal warnings enabled),
method_nameandlibc_nameare computed above but never used in the#elsebranch, which can trigger -Wunused-variable/-Werror and break the build. Also theif (true)wrapper is redundant; explicitly mark locals unused and throw directly.
#else
if (true)
{
throw DabRuntimeError("function import not supported on windows yet");
}
|
Repair follow-up: I am addressing the valid suppressed Copilot finding in the Windows unsupported-FFI branch only. I will remove the redundant control flow and avoid computing Windows-unused locals without changing the existing error contract or broadening FFI, then rerun focused local validation, push to this branch, and reconverge all five exact-head CI jobs plus fresh Copilot review. |
Throw before computing native import state on Windows so the guarded branch remains warning-free while preserving the existing exception contract.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
premake5.lua:44
DAB_PLATFORM_WINDOWSis already defined insrc/cshared/dab.hbased on_WIN32/__MINGW32__. Defining it again here (without a value) can trigger macro-redefinition warnings and may break Windows builds when warnings are treated as errors.
filter "system:windows"
defines { "DAB_PLATFORM_WINDOWS" }
src/cvm/syscalls.cpp:58
- In the Windows-only branch,
reglistis only referenced byassert(...), which may compile out in Release builds (e.g., whenNDEBUGis defined). That can leavereglistunused and fail builds under-Werror//WX.
#ifdef DAB_PLATFORM_WINDOWS
(void)out_reg;
throw DabRuntimeError("function import not supported on windows yet");
#else
.github/workflows/ruby.yml:55
- The macOS job hard-codes
CLANG_FORMATviabrew --prefix llvm@18. If that formula isn’t installed (or brew is unavailable), the command substitution will fail and abort the step before tests run. Consider falling back to the defaultclang-formatwhen LLVM 18 isn’t present.
- name: Run tests
run: CLANG_FORMAT="$(brew --prefix llvm@18)/bin/clang-format" PREMAKE='./premake5' bundle exec rake
Keep the unsupported FFI branch warning-free when assertions are disabled and rely on the existing platform header for the Windows macro instead of defining it twice.
|
Repair completed at |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/cvm/syscalls.cpp:59
- Runtime error message uses lowercase "windows"; this is user-facing and should match the platform name capitalization used elsewhere (e.g., test skip metadata uses "windows" as a token but messages print "Windows").
#ifdef DAB_PLATFORM_WINDOWS
(void)out_reg;
(void)reglist;
throw DabRuntimeError("function import not supported on windows yet");
#else
Summary
VERSIONfrom0.0.2to0.0.3without adding a production version source or pinning the current value in stable testsExact base and head
master30ba808fa3571c1d6ea458399316f5299bb2e270d0952089ccf0cdf8b1984f554d167f1fc77be12bPlatform matrix
ubuntu-latestbundle exec rakemacos-15-intelbundle exec rakewindows-latestbundle exec rakeValidation
actionlint .github/workflows/ruby.ymlgit diff --checkbundle exec rubocop Rakefile src/frontend/frontend.rb spec/compiler/version_spec.rb-Wall -Wextra -Werror.exeand.dlltargetsRBENV_VERSION=3.3.12 PREMAKE='./premake5' bundle exec rake(passed)VERSIONGenerated files
The inherited gate was run only in a disposable checkout. No generated documentation or build artifacts are included in this PR.
Limitations and dependency state
Windows FFI remains explicitly unsupported; the two newer FFI fixtures now carry the same Windows skip metadata as the existing FFI fixtures. This PR does not redesign or broaden FFI.
The former dependency on #13 is resolved: #13 was squash-merged into
masteras30ba808fa3571c1d6ea458399316f5299bb2e270, and this branch is rebased on that exact commit. The intended VERSION transition is now0.0.2to0.0.3. Exact-head CI and review have converged; this PR remains unmerged by design.