Skip to content

Conversation

@mabrarov
Copy link
Contributor

@mabrarov mabrarov commented Apr 21, 2025

Changes:

Note that when Fluent Bit is built with MS Visual C++, then static linkage with C/C++ runtime is used - refer to https://github.com/fluent/fluent-bit/blob/master/CMakeLists.txt#L76 - so that removed DLLs are not needed. I copied Fluent Bit binary from built docker image and checked its dependencies with https://github.com/lucasg/Dependencies to ensure that removed DLLs are not used directly by Fluent Bit binary:

$ docker build -t fluent/fluent-bit -f dockerfiles/Dockerfile.windows . && \
container_id="$(docker create fluent/fluent-bit)" && \
flb_executable="$(mktemp --suffix .exe)" && \
docker cp "${container_id}:C:\\fluent-bit\\bin\\fluent-bit.exe" "${flb_executable}" && \
docker rm -fv "${container_id}" && \
Dependencies -modules -depth 1 "${flb_executable}" && \
rm -f "${flb_executable}"
...
Successfully copied 17.4MB to C:\Users\...\AppData\Local\Temp\tmp.75D0EaxWDN.exe
905a43715bc6b6cc365d6444b10269dc436441a965e642401827f1dc4422189c
[ROOT] tmp.75D0EaxWDN.exe : C:/Users/.../AppData/Local/Temp/tmp.75D0EaxWDN.exe
[WellKnownDlls] WS2_32.dll : C:\WINDOWS\system32\WS2_32.dll
[WellKnownDlls] CRYPT32.dll : C:\WINDOWS\system32\CRYPT32.dll
[WellKnownDlls] SHLWAPI.dll : C:\WINDOWS\system32\SHLWAPI.dll
[WellKnownDlls] SHELL32.dll : C:\WINDOWS\system32\SHELL32.dll
[WellKnownDlls] ADVAPI32.dll : C:\WINDOWS\system32\advapi32.dll
[WellKnownDlls] KERNEL32.dll : C:\WINDOWS\system32\kernel32.dll
[WellKnownDlls] USER32.dll : C:\WINDOWS\system32\user32.dll
[WellKnownDlls] ole32.dll : C:\WINDOWS\system32\ole32.dll
[WellKnownDlls] OLEAUT32.dll : C:\WINDOWS\system32\OLEAUT32.dll
[WindowsFolder] bcrypt.dll : C:\WINDOWS\system32\bcrypt.dll
[WindowsFolder] IPHLPAPI.DLL : C:\WINDOWS\system32\IPHLPAPI.DLL
[WindowsFolder] wevtapi.dll : C:\WINDOWS\system32\wevtapi.dll
[WindowsFolder] NETAPI32.dll : C:\WINDOWS\system32\NETAPI32.dll
[WindowsFolder] Secur32.dll : C:\WINDOWS\system32\Secur32.dll

$ docker run --rm --entrypoint powershell fluent/fluent-bit dir 'C:\fluent-bit\bin'


    Directory: C:\fluent-bit\bin


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
-a----        4/22/2025  12:25 AM       17359360 fluent-bit.exe

Testing

  • [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
  • [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.

Summary by CodeRabbit

  • Documentation
    • Clarified Windows build guidance by removing an outdated OS-version constraint for the build tools.
  • Chores
    • Upgraded vcpkg tooling to a newer release and removed redundant runtime DLL copying to simplify Windows image artifacts.
  • CI / Tests
    • Introduced dynamic image tagging via an IMAGE variable and added a post-build sanity check that runs the produced Windows container before pushing.

@patrick-stephens
Copy link
Collaborator

Any chance we can add some basic sanity tests for the container build to confirm?

@mabrarov
Copy link
Contributor Author

mabrarov commented Apr 22, 2025

Hi @patrick-stephens,

some basic sanity tests for the container build to confirm?

Isn't .github/workflows/pr-image-tests.yaml:116-119 (https://github.com/fluent/fluent-bit/actions/runs/14580963650/job/40897379281?pr=10233) sufficient for this pull request?

Thank you.

@mabrarov
Copy link
Contributor Author

mabrarov commented Apr 22, 2025

@edsiper and @patrick-stephens,

Do you have any thoughts / requirements / doubts on this pull request? Considering this pull request was part of original #10180 which was merged by @edsiper, I find missing changes being caused by merge conflict resolution issue. It is the reason I was under impression that this PR is going to be accepted soon to avoid keeping half of original #10180 in master branch. Please note that building of docker image for Windows Server Containers in master branch doesn't fail only because we are lucky and DLLs (which copying is removed in this PR) exist in builder-base image only because of installation of Visual Studio Build Tools. Base image (mcr.microsoft.com/windows/servercore:ltsc2019) of builder-base doesn't have these DLLs - command provided below doesn't find them:

docker run --rm --entrypoint powershell mcr.microsoft.com/windows/servercore:ltsc2019 dir 'C:\Windows\System32' | grep -E -i '(msvcp140|vccorlib140|vcruntime140)\.dll' 

Thank you.

@patrick-stephens
Copy link
Collaborator

Hi @patrick-stephens,

some basic sanity tests for the container build to confirm?

Isn't .github/workflows/pr-image-tests.yaml:116-119 (https://github.com/fluent/fluent-bit/actions/runs/14580963650/job/40897379281?pr=10233) sufficient for this pull request?

Thank you.

Ah we did add some already then should be ok.

@patrick-stephens
Copy link
Collaborator

@edsiper and @patrick-stephens,

Do you have any thoughts / requirements / doubts on this pull request? Considering this pull request was part of original #10180 which was merged by @edsiper, I find missing changes being caused by merge conflict resolution issue. It is the reason I was under impression that this PR is going to be accepted soon to avoid keeping half of original #10180 in master branch. Please note that building of docker image for Windows Server Containers in master branch doesn't fail only because we are lucky and DLLs (which copying is removed in this PR) exist in builder-base image only because of installation of Visual Studio Build Tools. Base image (mcr.microsoft.com/windows/servercore:ltsc2019) of builder-base doesn't have these DLLs - command provided below doesn't find them:

docker run --rm --entrypoint powershell mcr.microsoft.com/windows/servercore:ltsc2019 dir 'C:\Windows\System32' | grep -E -i '(msvcp140|vccorlib140|vcruntime140)\.dll' 

Thank you.

Have you actually used the containers after this change is the main one? It's not clear if you have so be good to include that info. I think the change is ok, but I cannot test it myself and I just want to avoid any silly issues.

@mabrarov
Copy link
Contributor Author

mabrarov commented Apr 22, 2025

Hi @patrick-stephens,

I am not production user of Windows Server Containers, but the image built on my local environment (Windows Server 2019 VM) from this pull request source branch seems to be running same way as image built from master branch when using:

docker build -t fluent/fluent-bit -f dockerfiles/Dockerfile.windows . && \
docker run --rm -it fluent/fluent-bit

Thank you.

@mabrarov
Copy link
Contributor Author

Hi @edsiper and @patrick-stephens,

Could you please share - what's the plan (please check description of this PR before answering)?

Thank you for your patience and work.

@patrick-stephens
Copy link
Collaborator

I've approved it, will be merged at some point

@mabrarov
Copy link
Contributor Author

mabrarov commented May 19, 2025

Hi @edsiper and @patrick-stephens,

Sorry for spamming you. I just don't like a half of work (half of #10180) to remain pending for a month while the only reason it was not merged is (IMHO) merge conflict resolution issue (which I fixed within the same day, but it was too late).

Thank you.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
dockerfiles/Dockerfile.windows (1)

112-113: Update outdated vcpkg version reference in comment.

Line 112's comment still references the old vcpkg version (2024.05.24), but line 113 now specifies 2025.10.17. Update the comment to reflect the current version to avoid confusion during maintenance.

-# https://github.com/microsoft/vcpkg/blob/2024.05.24/scripts/bootstrap.ps1
+# https://github.com/microsoft/vcpkg/blob/2025.10.17/scripts/bootstrap.ps1
 ENV VCPKG_VERSION=2025.10.17 `
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1dc25a and 01e0bbc.

📒 Files selected for processing (1)
  • dockerfiles/Dockerfile.windows (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.

Applied to files:

  • dockerfiles/Dockerfile.windows
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
  • GitHub Check: PR - Classic docker build test
  • GitHub Check: PR - Docker windows build test, windows 2022 and 2025 (2025)
  • GitHub Check: PR - Docker windows build test, windows 2022 and 2025 (2022)
  • GitHub Check: PR - Buildkit docker build test
🔇 Additional comments (2)
dockerfiles/Dockerfile.windows (2)

26-26: Comment update clarifies MSVS_VERSION support, removes OS constraint.

The comment now clearly documents support for MSVS_VERSION 16 and 17 without tying it to a specific Windows Server version, simplifying future maintenance.


113-113: vcpkg version bump requires local verification of triplet installations.

The web search results indicate that the main breaking changes between 2024.05.24 and 2025.10.17 are CI/workflow-related (binary cache provider changes, tool metadata JSON migration) rather than core functionality issues. No specific breaking changes were identified for openssl or libyaml triplet installations.

However, the search results do not provide granular details about:

  • Bootstrap script changes specific to Windows PowerShell
  • Triplet-specific compatibility for openssl and libyaml
  • Any platform-specific behavioral shifts for Windows containers

Since you note the image was tested locally, document the test results (build completion, triplet download/installation success, and any vcpkg tool version-specific observations) to confirm compatibility before merging. This ensures the version bump is validated for the specific Windows Docker build context.

@cosmo0920
Copy link
Contributor

Sanity check is now green. Looks good to me. 👍

https://github.com/fluent/fluent-bit/actions/runs/19067980822/job/54463260838

@cosmo0920 cosmo0920 added this to the Fluent Bit v4.2 milestone Nov 5, 2025
@patrick-stephens patrick-stephens merged commit c7a707e into fluent:master Nov 5, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants