dockerfiles: Install minimum components and avoiding to use includeRecommended#11477
dockerfiles: Install minimum components and avoiding to use includeRecommended#11477
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:
📝 WalkthroughWalkthroughReplaces workload-based Visual Studio Build Tools install with explicit components ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dockerfiles/Dockerfile.windows`:
- Around line 49-52: The Dockerfile currently adds the unversioned
Microsoft.VisualStudio.Component.Windows10SDK which only installs the Universal
C Runtime and, because --includeRecommended was removed, the full Windows SDK
(headers/libs) is missing; replace the unversioned component with a versioned
full SDK component such as Microsoft.VisualStudio.Component.Windows10SDK.20348
or Microsoft.VisualStudio.Component.Windows11SDK.26100 in the same install list
(next to Microsoft.VisualStudio.Component.VC.Tools.x86.x64) so the build gets
the full headers/libraries required for CMake/NMake C++ builds.
295342e to
b1404df
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dockerfiles/Dockerfile.windows (1)
49-52: Component IDs are valid; consider a newer Windows SDK for theltsc2025base image.
Microsoft.VisualStudio.Component.VC.Tools.x86.x64andMicrosoft.VisualStudio.Component.Windows10SDK.19041are both listed as valid Optional components in the VS Build Tools 2022 component directory (17.14.36015.10). The explicit addition ofVC.Tools.x86.x64is the correct way to ensure the MSVC compiler is installed without--includeRecommended.One minor consideration: the base image is
ltsc2025(Windows Server 2025), andWindows10SDK.19041(10.0.19041.0, May 2020) is a noticeably older SDK than what--includeRecommendedwould have pulled in. For recent releases, use of a Windows 11 SDK or later is strongly recommended. If Fluent Bit's build or any dependency ever needs a newer API surface, a one-line bump to e.g.Windows11SDK.26100would be needed.19041is sufficient for the current build, but the delta is worth being intentional about.🔧 Optional: bump to a newer SDK family
- '--add Microsoft.VisualStudio.Component.Windows10SDK.19041', ` + '--add Microsoft.VisualStudio.Component.Windows11SDK.26100', `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockerfiles/Dockerfile.windows` around lines 49 - 52, The Dockerfile.windows currently installs Microsoft.VisualStudio.Component.Windows10SDK.19041 which is older than appropriate for the ltsc2025 base image; update the SDK component to a newer Windows 11 SDK (for example replace Microsoft.VisualStudio.Component.Windows10SDK.19041 with Microsoft.VisualStudio.Component.Windows11SDK.26100) in the same install command that includes Microsoft.VisualStudio.Workload.VCTools and Microsoft.VisualStudio.Component.VC.Tools.x86.x64 so the MSVC toolchain stays installed and the SDK targets a newer API surface compatible with ltsc2025.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dockerfiles/Dockerfile.windows`:
- Around line 49-52: The Dockerfile.windows currently installs
Microsoft.VisualStudio.Component.Windows10SDK.19041 which is older than
appropriate for the ltsc2025 base image; update the SDK component to a newer
Windows 11 SDK (for example replace
Microsoft.VisualStudio.Component.Windows10SDK.19041 with
Microsoft.VisualStudio.Component.Windows11SDK.26100) in the same install command
that includes Microsoft.VisualStudio.Workload.VCTools and
Microsoft.VisualStudio.Component.VC.Tools.x86.x64 so the MSVC toolchain stays
installed and the SDK targets a newer API surface compatible with ltsc2025.
b1404df to
e2771ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dockerfiles/Dockerfile.windows`:
- Around line 150-153: The PowerShell Docker RUN lines call cmd /C "call
%MSVS_HOME%\VC\Auxiliary\Build\vcvars64.bat && vcpkg install ..." but do not
propagate cmd's non‑zero exit to the PowerShell process, so vcpkg failures are
swallowed; after each cmd invocation (the lines using cmd /S /C "call ... &&
vcpkg install ...") add an explicit check of $LASTEXITCODE and if it is non‑zero
call exit $LASTEXITCODE (or throw) before running the subsequent Remove-Item
cleanup statements so the Docker build fails when vcpkg fails; apply the same
change to the other similar cmd invocation block referenced in the comment (the
lines around the second vcpkg install).
e2771ab to
d11a6f6
Compare
d11a6f6 to
5089ffc
Compare
5089ffc to
b3964e3
Compare
21fa13e to
93a663a
Compare
93a663a to
fe62dfc
Compare
fe62dfc to
1fa60ea
Compare
1fa60ea to
96162ba
Compare
…commended Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
96162ba to
4400ec6
Compare
|
Build images runners are succeeded to build: But creating packages are failed: Interesting... |
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:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-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