Skip to content

build: ci: tune MSVC flags, modify ci caching - #10343

Merged
edsiper merged 3 commits into
fluent:masterfrom
erikced:windows-ninja-build
May 23, 2025
Merged

build: ci: tune MSVC flags, modify ci caching#10343
edsiper merged 3 commits into
fluent:masterfrom
erikced:windows-ninja-build

Conversation

@erikced

@erikced erikced commented May 16, 2025

Copy link
Copy Markdown
Contributor
  • Change the importlib name for fluent-bit.exe, and modify how debug data output is configured for fluent-bit.dll so that build outputs are not overwritten by other build outputs.
  • Remove code preventing Ninja/MSVC builds.
  • Update MSVC linker flags altered by /DEBUG to match Microsoft's recommendations.
  • Update/fix vcpkg caching

All in all this reduces MSVC CI build times with ~20-25% if there is a cache hit for the vcpkg packages and the size of fluent-bit.exe is reduced with about 20 %.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [N/A] Example configuration file for the change
  • [N/A] Debug log output from testing the change
  • [N/A] Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • [N/A] Run local packaging test showing all targets (including any new ones) build.
  • [N/A] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • [N/A] Documentation required for this feature

Backporting

  • [N/A] Backport to latest stable release.

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.

@cosmo0920

Copy link
Copy Markdown
Contributor

I tried to use Ninja locally on my Windows development box but cmake in that environment does complain that that installed cmake does not support Ninja as a build tool. I'm just curious which version or flavor of cmake could generate rules for Ninja?
Plus, we did use NMake makefile generator for cutting the execution time of configuration phase.
The using Ninja building rules could be mostly equivalent against NMake Makefiles rules because CI tasks request to run configuring phase on every PRs. So, if using Ninja generator could reduce building time and makes longer configuring execution time, there's a trade-off for the point of execution time.

@erikced

erikced commented May 20, 2025

Copy link
Copy Markdown
Contributor Author

I tried to use Ninja locally on my Windows development box but cmake in that environment does complain that that installed cmake does not support Ninja as a build tool.

I used used CMake from MSVC 2022's C++ CMake tools for Windows, which identifies itself as 3.31.6-msvc6. Coincidentally 3.31.6 is the version that is installed in CI as well.

Plus, we did use NMake makefile generator for cutting the execution time of configuration phase. The using Ninja building rules could be mostly equivalent against NMake makefiles rules because CI tasks request to run configuring phase on every PRs. So, if using Ninja generator could reduce building time and makes longer configuring execution time, there's a trade-off for the point of execution time.

That's fair, I would say it reduces total time for the Windows builds even if configuration time may be longer, an example: with (n.b. with openssl cache miss, with a hit it would be even less) vs without these changes. These are just two examples but they align well with my measurements.
image
image
NMake does not execute in parallel and CMake does not help by generating one call per source file. Github Windows runners are quad core VMs so there is time to save by compiling in parallel.

That said, I understand that you may not necessarily wish to change NMake to Ninja in CI and will of course drop that commit if you wish.

@cosmo0920

cosmo0920 commented May 21, 2025

Copy link
Copy Markdown
Contributor

That said, I understand that you may not necessarily wish to change NMake to Ninja in CI and will of course drop that commit if you wish.

Got it. It's reasonable. Migrating from NMake to Ninja could be a future task. So, could you drop that commit and focus on the GHA caching and tweaking options for MSVC? Those of commit are still quite useful for us. And the dropped commit should be sent as another PR if you have a more cycles.

erikced added 3 commits May 22, 2025 08:28
Use CMAKE_SHARED_LINKER_FLAGS for MSVC to trigger creation of PDBs for
dynamic libraries (i.e. fluent-bit.dll) instead of specifying
target_link_options as those are inherited by the out_lib and
hello_world examples causing intermittent build failures when attempting
to overwrite fluent-bit.dll.pdb.

Set the suffix of the import lib for fluent-bit.exe to .exe.lib so that
it does not conflict with the static library's import lib.

Signed-off-by: Erik Cederberg <erik.cederberg@sectra.com>
Enabling /DEBUG implicitly disables /OPT:REF and /OPT:ICF which leads to
larger binaries (~25 % for x64 builds). Re-enable /OPT:REF for all
builds and /OPT:ICF for 64-bit builds.

Signed-off-by: Erik Cederberg <erik.cederberg@sectra.com>
Change caching to cache installed packages and run vcpkg upgrade to
match the previous behavior where packages were re-built each time.

Signed-off-by: Erik Cederberg <erik.cederberg@sectra.com>
@erikced
erikced force-pushed the windows-ninja-build branch from 15a28f9 to 36d4536 Compare May 22, 2025 06:29
@erikced erikced changed the title build: ci: enable Ninja/MSVC and use it in CI build: ci: tune MSVC flags, modify ci caching May 22, 2025
@erikced

erikced commented May 22, 2025

Copy link
Copy Markdown
Contributor Author

@cosmo0920 please have a look now.

@patrick-stephens

Copy link
Copy Markdown
Contributor

Looks ok to me but waiting on @cosmo0920 to confirm - plus need it to be green in CI too before merge.

Comment thread src/CMakeLists.txt

@cosmo0920 cosmo0920 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that this change won't affect to generate PDB for debugging purpose especially for WinDbg.
It looks great to me. 👍

@edsiper
edsiper merged commit 73e0625 into fluent:master May 23, 2025
@edsiper edsiper added this to the Fluent bit v4.0.3 milestone May 23, 2025
@erikced
erikced deleted the windows-ninja-build branch May 25, 2025 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci ok-package-test Run PR packaging tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants