Repository navigation
Conversation
conan install (line before cmake --fresh) let Conan auto-detect its own CMake generator for dependencies built from source (--build=missing, e.g. yajl), based on whatever Visual Studio version its auto-detected profile picked up. On a machine with more than one VS version installed, that can differ from the "Visual Studio 17 2022" generator this script explicitly requests for the main project two lines later. yajl/2.1.0's own CMakeLists.txt still does `cmake_policy(SET CMP0026 OLD)`, a policy newer CMake versions have dropped OLD-behavior support for entirely. Under a newer, auto-detected generator/CMake (observed: "Visual Studio 18 2026" on a GitHub-hosted windows-2025 runner that had more than one VS install), yajl's configure step hard-errors on that unsupported policy before it ever gets to build. Under the generator this project actually targets, it configures fine. Fix: explicitly pass the same generator to `conan install` via `-c tools.cmake.cmaketoolchain:generator`, so dependency builds use the identical toolchain as the main project rather than whatever a given machine's Conan profile auto-detection happens to prefer. Reported downstream via ModSecurity-nginx's Windows CI (owasp-modsecurity/ModSecurity-nginx#388 built this via vcbuild.bat and hit this exact failure). Fixes #3604. Note on validation: this repo's own PR-triggered Windows CI (ci.yml/ci_new.yml) runs on windows-2022, which -- as far as I can tell -- only has one Visual Studio version installed, so it's unlikely to reproduce the generator-mismatch this fixes even without the fix. The pin is a no-op when only one matching VS version is present either way, so it shouldn't regress that CI; real confirmation that this resolves the original failure will need a windows-2025-class runner (or a maintainer's machine with more than one VS version installed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit fixed the generator mismatch but left the resulting "Visual Studio 17 2022" string duplicated across the Conan and CMake invocations -- exactly the kind of duplication that could drift out of sync on a future VS upgrade and silently reintroduce the same class of bug. Single source of truth via VS_GENERATOR instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the Windows build script (vcbuild.bat) use a single, consistent CMake generator for both Conan-built dependencies (built from source under --build=missing) and the main CMake configure step, preventing generator/toolchain mismatches on machines with multiple Visual Studio installations.
Changes:
- Introduces a
VS_GENERATORvariable as the single source of truth for the targeted Visual Studio/CMake generator. - Pins Conan’s
CMakeToolchaingenerator via-c tools.cmake.cmaketoolchain:generator=...to match the project’s CMake-Ggenerator selection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
still same problem... |
Collaborator
Author
|
@Zorono this log shows a different failure than the one this PR fixes. The generator is now consistent ( yajl 2.1.0's upstream Until the recipe is patched, the workaround is to let Conan build yajl with a CMake 3.x, e.g. install CMake 3.31 and pass |
Contributor
|
Hi @fzipi, regarding your comment about YAJL’s incompatibility with CMake 4: I have addressed that issue in #3653 using a different approach. While #3605 aligns the CMake generator used by Conan and the main project, my changes also patch the YAJL recipe itself. The patch removes CMP0026 OLD and replaces the LOCATION target-property lookups for json_reformat and json_verify with $<TARGET_FILE:...> generator expressions. The patched recipe is included as yajl/2.1.0@modsecurity/ci. Additionally, vcbuild.bat uses Conan-generated CMake presets rather than hard-coding the Visual Studio 2022 generator. The intention is to address the YAJL incompatibility directly and support the VS 2026/CMake 4 toolchain, rather than requiring CMake 3.x as a workaround. The two PRs therefore overlap in the Windows build setup, but they are not identical fixes. My PR also contains broader CI updates, so I wanted to highlight the YAJL-specific change here. For transparency, the patched YAJL build has been tested locally with CMake 4 using Zig/Clang, but I have not performed a complete local ModSecurity build with MSVC/VS 2026. Could you take a look at the YAJL patch in #3653 and share your thoughts on how best to coordinate the overlapping changes? |
This branch has not been deployed
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.
Summary
Fixes #3604.
conan install(invcbuild.bat) let Conan auto-detect its own CMake generator for dependencies built from source (--build=missing, e.g.yajl), based on whatever Visual Studio version its auto-detected profile picks up. On a machine with more than one VS version installed, that can differ from the"Visual Studio 17 2022"generator this script explicitly requests for the main project two lines later viacmake --fresh .. -G "Visual Studio 17 2022".yajl/2.1.0's ownCMakeLists.txtstill doescmake_policy(SET CMP0026 OLD), a policy newer CMake versions have droppedOLD-behavior support for entirely. Under a newer, auto-detected generator/CMake (observed:"Visual Studio 18 2026"on a GitHub-hostedwindows-2025runner that had more than one VS install),yajl's configure step hard-errors on that unsupported policy before it ever gets to build. Under the generator this project actually targets, it configures fine.conan installvia-c tools.cmake.cmaketoolchain:generator, so dependency builds use the identical toolchain as the main project rather than whatever a given machine's Conan profile auto-detection happens to prefer.VS_GENERATORvariable referenced by both the Conan and CMake invocations, so a future VS-version bump only needs to change one line instead of two that must be kept in sync.Reported downstream via
ModSecurity-nginx's Windows CI (owasp-modsecurity/ModSecurity-nginx#388 built this viavcbuild.batand hit this exact failure).Note on validation
This repo's own PR-triggered Windows CI (
ci.yml/ci_new.yml) runs onwindows-2022, which — as far as I can tell — only has one Visual Studio version installed, so it's unlikely to reproduce the generator-mismatch this fixes even without the fix. The pin is a no-op when only one matching VS version is present either way, so it shouldn't regress that CI; real confirmation that this resolves the original failure will need awindows-2025-class runner (or a maintainer's machine with more than one VS version installed).Test plan
-c tools.cmake.cmaketoolchain:generatorconf key against Conan'sCMakeToolchain._get_generator()source to confirm it's the correct override point.ci.yml/ci_new.yml(windows-2022) still pass — expected to be a no-op there.windows-2025-class runner (or a machine with multiple VS installs) that this resolves the originalCMP0026failure.