build: CMake project fixes for MS Visual C++ compiler#11397
build: CMake project fixes for MS Visual C++ compiler#11397mabrarov wants to merge 11 commits intofluent:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds delayed-extension CXX/link flag variables, moves MSVC runtime and /utf-8 handling into per-config delayed workflow, updates Windows builder to use Ninja/vcpkg, refines CMake flag handling across platforms, and fixes 64-bit file-mapping on Windows. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CI
participant Docker as Windows Builder
participant Ninja as Ninja
participant CMake as CMake
participant MSVC as MSVC Toolchain
participant Linker as Linker
Dev->>Docker: trigger Windows build
Docker->>Ninja: install & use Ninja generator
Ninja->>CMake: configure (reads FLB_EXTRA_CMAKE_CXX_* and vcpkg toolchain)
CMake->>MSVC: apply per-config flags (/utf-8, runtime selection, delayed CXX flags)
MSVC->>Linker: compile & link
Linker->>Dev: produce artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mabrarov
force-pushed
the
feature/cmake_msvc
branch
from
0f21046 to
36badb0
Compare
mabrarov
force-pushed
the
feature/cmake_msvc
branch
from
36badb0 to
ae52f35
Compare
mabrarov
force-pushed
the
feature/cmake_msvc
branch
from
ae52f35 to
6e2e864
Compare
|
This PR is ready for review. The failed CI jobs seem to be failing because of internal infrastructure issues now (previously it was this PR issue - refer to #11397 (comment)). |
mabrarov
force-pushed
the
feature/cmake_msvc
branch
from
7f8fdee to
1ea245f
Compare
|
Hi @edsiper, @patrick-stephens and @cosmo0920 , I appreciate if you can find time to review this PR, because it is essential for development of Fluent Bit on / for Windows (e.g. debugging). Thank you. |
|
Thanks for this @mabrarov, appreciate the effort you have made to provide this as its a hard target for us to support. Can you check the CI is all green? Generally that's a blocker before we can review. |
|
Regarding your question about failed CI:
Thank you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 57-62: The current add_compile_definitions call unconditionally
defines _DEBUG for all compilers in Debug builds which affects bundled libraries
(e.g., lib/miniz's mz_zip_array_range_check) — change this to only add _DEBUG
for MSVC Debug builds by replacing the unconditional generator expression with a
conditional one that checks both Debug config and the MSVC compiler ID
(referencing add_compile_definitions and the _DEBUG symbol), preserving the
existing non-cache variables FLB_EXTRA_CMAKE_CXX_FLAGS and
FLB_EXTRA_CMAKE_CXX_LINK_FLAGS behavior.
🧹 Nitpick comments (1)
lib/onigmo/CMakeLists.txt (1)
33-49: MSVC runtime and UTF-8 handling mirrors the root CMakeLists.txt pattern.The duplication of MSVC runtime handling here is necessary because onigmo's
CMakeLists.txtresetsCMAKE_C_FLAGS(line 61), and the per-configCMAKE_C_FLAGS_*variables need the MD→MT fixup in this scope as well.Consider extracting the shared MSVC setup (runtime selection, UTF-8, per-config fixup) into a reusable CMake include file to reduce duplication across the root
CMakeLists.txtandlib/onigmo/CMakeLists.txt.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 65-82: The CMAKE_MSVC_RUNTIME_LIBRARY assignment is ignored
because policy CMP0091 defaults to OLD under the current cmake_minimum_required;
set the policy to NEW before you assign CMAKE_MSVC_RUNTIME_LIBRARY (use
cmake_policy(SET CMP0091 NEW) placed earlier than the
set(CMAKE_MSVC_RUNTIME_LIBRARY ...) line) so the variable takes effect; keep the
existing add_compile_options(/MT...) and the regex replacement loop
(string(REGEX REPLACE ...)) if you want to remain robust against toolchain files
that may still inject /MD.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
712-727: Delayed CXX flag application afterenable_language(CXX)— good pattern.The
/MD→/MTreplacement for CXX flags correctly mirrors the C flags handling above (lines 72–76), and applyingFLB_EXTRA_CMAKE_CXX_FLAGS/FLB_EXTRA_CMAKE_CXX_LINK_FLAGShere ensures they only take effect once the CXX language is actually enabled.Minor note: the concatenation on line 725 (
"${CMAKE_CXX_FLAGS}${FLB_EXTRA_CMAKE_CXX_FLAGS}") relies onFLB_EXTRA_CMAKE_CXX_FLAGShaving a leading space from how values are appended (e.g., line 485:"${FLB_EXTRA_CMAKE_CXX_FLAGS} -g -O0 ..."). This works but is a subtle convention. A space before${FLB_EXTRA_CMAKE_CXX_FLAGS}would be more defensive.Optional: explicit space separator
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}${FLB_EXTRA_CMAKE_CXX_FLAGS}") - set(CMAKE_CXX_LINK_FLAGS "${CMAKE_CXX_LINK_FLAGS}${FLB_EXTRA_CMAKE_CXX_LINK_FLAGS}") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${FLB_EXTRA_CMAKE_CXX_FLAGS}") + set(CMAKE_CXX_LINK_FLAGS "${CMAKE_CXX_LINK_FLAGS} ${FLB_EXTRA_CMAKE_CXX_LINK_FLAGS}")
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DEVELOPER_GUIDE.md (1)
800-857:⚠️ Potential issue | 🟡 MinorUseful addition of vcpkg manifest and build instructions for Windows Server 2022.
The step-by-step guide with
vcpkg.json, dependency installation, and Visual Studio solution generation is clear and actionable.One thing to note: the CMake configure command on line 845 uses the Visual Studio multi-config generator but doesn't set
-DFLB_DEBUG=Off -DFLB_RELEASE=On, soFLB_DEBUGdefaults toYes. This means CMake-level debug definitions will be active even when building with--config Releaseon line 856. This is the multi-config generator limitation acknowledged in the PR objectives, so not a blocker — but it may be worth adding a brief note here to help developers avoid confusion.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
712-726: Delayed CXX flag application is well-structured.The pattern of accumulating flags in
FLB_EXTRA_CMAKE_CXX_FLAGS/FLB_EXTRA_CMAKE_CXX_LINK_FLAGSbeforeenable_language(CXX)and applying them after is correct —CMAKE_CXX_FLAGSisn't meaningful until the CXX language is enabled. The MSVC/MD→/MTregex replacement forCMAKE_CXX_FLAGS_*variants mirrors the earlier C flags treatment (lines 72-76), maintaining consistency.One minor note: the concatenation on line 725 (
"${CMAKE_CXX_FLAGS}${FLB_EXTRA_CMAKE_CXX_FLAGS}") relies on accumulated values inFLB_EXTRA_CMAKE_CXX_FLAGSalways having a leading space (e.g., line 485:" -g -O0 ..."). This works correctly given the current usage patterns, but is a fragile convention. Consider a space separator for robustness:♻️ Optional: safer concatenation
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}${FLB_EXTRA_CMAKE_CXX_FLAGS}") - set(CMAKE_CXX_LINK_FLAGS "${CMAKE_CXX_LINK_FLAGS}${FLB_EXTRA_CMAKE_CXX_LINK_FLAGS}") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${FLB_EXTRA_CMAKE_CXX_FLAGS}") + set(CMAKE_CXX_LINK_FLAGS "${CMAKE_CXX_LINK_FLAGS} ${FLB_EXTRA_CMAKE_CXX_LINK_FLAGS}")
|
FYI, the only remaining library linking issue with multi-config CMake generators and MSVC is LibYAML located at: Line 438 in d142e3f and Line 1082 in d142e3f There is a chance that it is the only remaining issue with multi-config CMake generators and MSVC at all. Due to LibYAML (CMake project of LibYAML) doesn't provide its layout (location and naming of built libraries) for multi-config builds - release and debug versions of LibYAML library file have same file name and are placed in the same directory - a custom FindLibYAML CMake module (e.g. to use yaml.lib for release builds and yamld.lib for debug builds with MSVC) is debatable and is not included into this PR. I plan to add it in a separate PR (refer to feature/cmake_libyaml branch) once this PR is approved and merged. |
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
| @@ -76,6 +77,8 @@ int cio_file_native_map(struct cio_file *cf, size_t map_size) | |||
| DWORD desired_access; | |||
| size_t file_size; | |||
| size_t actual_map_size; | |||
| DWORD actual_map_size_high; | |||
| DWORD actual_map_size_low; | |||
| int ret; | |||
|
|
|||
| if (cf == NULL) { | |||
| @@ -131,10 +134,18 @@ int cio_file_native_map(struct cio_file *cf, size_t map_size) | |||
|
|
|||
| /* CreateFileMappingA requires size as two DWORDs (high and low) */ | |||
| /* Use actual_map_size to ensure consistency */ | |||
| #if SIZE_MAX > MAXDWORD | |||
| actual_map_size_high = (DWORD)((actual_map_size >> (sizeof(DWORD) * CHAR_BIT)) | |||
| & 0xFFFFFFFFUL); | |||
| actual_map_size_low = (DWORD)(actual_map_size & 0xFFFFFFFFUL); | |||
| #else | |||
| actual_map_size_high = 0; | |||
| actual_map_size_low = (DWORD)actual_map_size; | |||
| #endif | |||
| cf->backing_mapping = CreateFileMappingA(cf->backing_file, NULL, | |||
| desired_protection, | |||
| (DWORD)(actual_map_size >> 32), | |||
| (DWORD)(actual_map_size & 0xFFFFFFFFUL), | |||
| actual_map_size_high, | |||
| actual_map_size_low, | |||
There was a problem hiding this comment.
Could you send your patch into fluent/cmetrics?
It would be better to handle there.
There was a problem hiding this comment.
3 participants
Summary
Note: multi-config CMake Generators (like "Visual Studio" ones) can still have issues due to Fluent Bit CMake project doesn't honor and doesn't support the build type(s) specified when CMake generates native build system project - e.g.
CMAKE_BUILD_TYPEpassed through command line andCMAKE_CONFIGURATION_TYPES. Support of multi-config CMake Generators can require usage of CMake generator expressions everywhere build-config specific compiler / linker options are added / modified, can require revisiting of the way linked libraries are specified, e.g. they should not use-loption with hard-coded name of static library file, because the name of library file can depend on build configuration on Windows - e.g. libcrypto.lib forReleaseandRelWithDebInfobuilds with/MTcompiler option (static release MS C/C++ runtime) vs libcryptod.lib forDebugbuild with/MTdcompiler option (static debug MS C/C++ runtime).Testing
Before we can approve your change; please submit the following in a comment:
TEST_PRESET=valgrind SKIP_TESTS='flb-rt-out_td flb-it-network' ./run_code_analysis.shok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Build & Infrastructure
Documentation
Bug Fixes