Build on Windows with MSVC - #4
Open
quartzjer wants to merge 5 commits into
Open
Conversation
src/common.hpp declares rfdetr_logf with __attribute__((format(printf, 2, 3))). MSVC has no __attribute__, so the declaration fails to parse (C3646 / C2059) and the definition in common.cpp then reads as a redefinition (C2084). The whole tree stops on the first translation unit, so rf-detr.cpp does not build on Windows at all today. Wrap it in RFDETR_ATTRIBUTE_FORMAT with the same three cases ggml.h already handles for GGML_ATTRIBUTE_FORMAT: empty on non-GNUC, gnu_printf on MinGW, printf elsewhere. No change on gcc/clang — the attribute is still applied and -Wformat still fires on a mismatched call.
bicubic_resample_patch_grid declares `constexpr float A` as an automatic
and then names it from a captureless lambda. gcc and clang accept that —
A is never odr-used, only read as a constant — but MSVC rejects it with
C3493 ('cannot be implicitly captured because no default capture mode
has been specified'), and the two follow-on errors at line 159 are that
failure cascading through `kernel`.
Making A `static constexpr` gives it static storage duration, so no
capture is needed and every compiler accepts it. Same constant, same
value, no behaviour change.
examples/cli/main.cpp calls POSIX ::mkdir, which MSVC does not provide — it has _mkdir in <direct.h> — so the CLI is the last thing blocking a Windows build once the library compiles. Replace the stat-probe-then-mkdir pair with std::filesystem::create_ directories, which is already available (the project is C++17) and is a no-op when the directory exists, so the probe is redundant. The two remaining ::stat calls are left alone: MSVC does provide stat. One behaviour difference worth naming: create_directories also creates missing parents, where ::mkdir created only the leaf. --masks some/new/dir now works instead of failing. The error message is unchanged.
test_cli_integration drives rfdetr-cli as a child process and includes <sys/wait.h>, which MSVC does not have, so enabling RFDETR_BUILD_TESTS on Windows fails the build outright on that one file. Guard its registration with NOT WIN32. Everything else under tests/ is portable, so the remaining 25 tests still build and run under MSVC, which is what makes a Windows CI job worth having. On Linux the test is registered and run exactly as before.
Everything in ci.yml is ubuntu-only, so nothing has ever compiled this tree with MSVC. That is why three portability breaks reached main unnoticed: an __attribute__ on a declaration, an automatic constexpr named from a captureless lambda, and a POSIX-only ::mkdir. All three build clean on gcc and stop MSVC on the first translation unit. Add one windows-2022 job in the same shape as the ubuntu build job - configure, compile, ctest, usage banner. CPU only and no model downloads: this guards the compile, not the detection numbers, which the existing ubuntu smoke-test job already covers. The ggml patch script is invoked explicitly before configure because the CMake configure-time hook shells out to bash, which is not on PATH in the MSVC environment.
|
@mudler This is good to merge. The MSVC fixes are narrowly scoped, Linux behavior is preserved, and the added Windows job covers the remaining portable test suite. The linked fork runs provide the missing branch checks: 25 tests and the CLI usage smoke test pass under MSVC. |
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
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.
What
Three small portability fixes, a test-registration guard, and one CI job so it stays fixed. Together they get a clean MSVC build of the library,
rfdetr-cli, and the portable part of the test suite. Fixes 1 to 3 are independent of each other; fix 5 needs 1 through 4. Nothing here changes detection behaviour on gcc or clang, and one behaviour difference is called out in Notes.src/common.hpp:rfdetr_logfis declared with__attribute__((format(printf, 2, 3))). MSVC has no__attribute__, so the declaration does not parse (C3646/C2059) and the definition incommon.cppthen reads as a redefinition (C2084). Wrapped inRFDETR_ATTRIBUTE_FORMAT, with the same three casesggml.halready handles forGGML_ATTRIBUTE_FORMAT: empty on non-__GNUC__,gnu_printfon MinGW-without-clang,printfelsewhere.src/model_loader.cpp:bicubic_resample_patch_griddeclaresconstexpr float Aas an automatic and then names it from a captureless lambda. gcc and clang accept that (Ais read as a constant, never odr-used); MSVC rejects it with C3493, and the two errors that follow at line 159 are that failure cascading throughkernel.static constexprgives it static storage duration, so no capture is needed. Same constant, same value.examples/cli/main.cpp: the--maskspath calls POSIX::mkdir, which MSVC does not provide (it has_mkdir, in<direct.h>). Replaced the stat-probe-then-mkdir pair withstd::filesystem::create_directories, which is a no-op when the directory exists, so the probe goes with it. This adds no new build requirement: the vendored ggml already usesstd::filesysteminggml-backend-reg.cpp, which every default build compiles, and nothing in the tree linksstdc++fs. The two remaining::statcalls are left alone, since MSVC providesstat.tests/CMakeLists.txt:test_cli_integrationdrivesrfdetr-clias a child process through<sys/wait.h>, so building the suite with MSVC fails outright on that one file. Its registration is now guarded withNOT WIN32. The other 25 tests are portable and still build and run under MSVC; on Linux nothing changes, it stays registered and run as before..github/workflows/ci.yml: onewindows-2022job in the same shape as the existing ubuntubuildjob: configure, compile,ctest, usage banner. CPU only and no model downloads, so it guards the compile rather than the numbers, which the ubuntusmoke-testjob already covers. It brings the first non-actions/*dependency into this repo's CI:ilammy/msvc-dev-cmd@v1for the MSVC environment, the same action at the same ref your release workflows inced.cpp,voice-detect.cpp,face-detect.cppandparakeet.cppalready use. It also runs the ggml patch script explicitly, since the configure-time hook needs bash on PATH.Why
ci.ymlruns onubuntu-latestonly, so nothing exercises MSVC today, and the tree currently stops on the first translation unit under it. These came up one at a time while adding windows jobs to a release workflow (#5). Each fix revealed the next, which is the argument for fix 5: without a Windows job the next one lands the same way, unnoticed.Notes
The CI job costs 1m49s of runner time and no added wall clock. Measured on a fork: 27s configure, 38s build, 4s for the 25 tests, the rest checkout and MSVC setup. It has no
needs:, so it starts alongside the ubuntubuildjob and finishes well inside the existing critical path, which isbuildthensmoke-testand the ~370 MB of model downloads your own comment atci.yml:86budgets for. A PR does not wait any longer than it does today. If you would rather not carry it, deleting thebuild-windowsjob is the only change needed and the fixes stand on their own.The behaviour differences worth calling out, all in fix 3:
create_directoriesalso creates missing parents, where::mkdircreated only the leaf, so--masks some/new/dirnow works instead of failing; it uses the default mode (0777 & ~umask) where::mkdirpassed0755; and a--maskspath that already exists as a file now prints the error, where the stat probe used to succeed and stay quiet. The error message itself is unchanged. Happy to keep any of these as they were.Fix 1 is the one worth a second look, since a wrong macro would silently disable format checking rather than fail. After the change,
rfdetr_logf(RFDETR_LOG_INFO, "%d", "not an int")compiled with-Wformatstill warns on gcc.Validation
On Linux/gcc, from a clean checkout:
detectoutput is byte-identical to the same build before these commits, and matches the committed reference (5/5 detections, 0 extras). The--maskspath was exercised both ways: a nested target directory is created, and an uncreatable one still printsfailed to create masks dir '...'. With tests on,test_cli_integrationis still registered and run on Linux,Test #6of 26.On Windows, two things ran on a fork:
build-windowsCI job, configure through build, 25 tests green, usage banner: https://github.com/solpbc/rf-detr.cpp/actions/runs/32386504236Both runs are on the fork's
main, which is this branch plus the release workflow from the companion PR; the five changes here are byte-identical on both. Worth knowing if you review them side by side: the release matrix builds withRFDETR_BUILD_TESTS=OFF, so it never compiled the suite and could not have found fix 4, the test guard. The CI job with tests on is what surfacedtest_cli_integration.