dockerfiles: windows: Avoid to use recommended components on ltsc2025 #10757
Conversation
cosmo0920
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
WalkthroughAdds ENV WINDOWS_VERSION to expose the build-arg at runtime and introduces conditional PowerShell logic in the Windows Dockerfile to include or omit the Visual Studio Build Tools --includeRecommended flag based on WINDOWS_VERSION (omitted for ltsc2025, included otherwise). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant D as Docker Build
participant PS as PowerShell (RUN)
participant VS as VS Build Tools Installer
Dev->>D: docker build --build-arg WINDOWS_VERSION=...
D->>D: ARG then ENV WINDOWS_VERSION="$WINDOWS_VERSION"
D->>PS: RUN install script
PS->>PS: $includeRecommended = "--includeRecommended"
alt WINDOWS_VERSION == "ltsc2025"
PS->>PS: $includeRecommended = ""
note over PS: omit recommended components
else Other versions
note over PS: include recommended components
end
PS->>VS: Invoke installer with component list + $includeRecommended
PS->>VS: Start-Process ... $includeRecommended
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
dockerfiles/Dockerfile.windows (2)
33-35: Nit: fix comment typo; condition reads fine once ENV is in-stageSmall wording fix for clarity; behavior is correct provided WINDOWS_VERSION is set in-stage as noted above.
- # Bydefault includeRecommended is absent when using ltsc2025 + # By default, omit --includeRecommended when using ltsc2025
48-49: Avoid passing an empty argument to Start-Process; build the arg list dynamicallyWhen ltsc2025, $includeRecommended is an empty string, which becomes an empty item in -ArgumentList. It’s often harmless, but constructing the args array conditionally is cleaner and avoids edge cases.
You can build the argument array first and append $includeRecommended only when set:
$vsArgs = @( '--quiet', '--wait', '--norestart', '--nocache', "--installPath ${env:MSVS_HOME}", "--channelUri ${msvs_build_tools_channel}", "--installChannelUri ${msvs_build_tools_channel}", '--add Microsoft.VisualStudio.Workload.VCTools' ) if ($includeRecommended) { $vsArgs += $includeRecommended } Start-Process "${msvs_build_tools_dist}" -ArgumentList $vsArgs -NoNewWindow -Wait;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dockerfiles/Dockerfile.windows(3 hunks)
⏰ 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). (5)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- 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 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: PR - Buildkit docker build test
- GitHub Check: PR - Classic docker build test
cosmo0920
force-pushed
the
cosmo0920-avoid-to-use-recommended-components-on-ltsc2025
branch
from
118e376 to
cb02808
Compare
Signed-off-by: Hiroshi Hatake <[email protected]>
cosmo0920
force-pushed
the
cosmo0920-avoid-to-use-recommended-components-on-ltsc2025
branch
from
cb02808 to
1332517
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dockerfiles/Dockerfile.windows (1)
19-20: Fix ARG scoping: ENV WINDOWS_VERSION won’t expand here; ltsc2025 check won’t workARG declared before the first FROM is not in scope inside a stage unless re-declared. As written,
$WINDOWS_VERSIONin ENV will resolve to empty (or literal), so${env:WINDOWS_VERSION}inside the RUN won’t reflect the build arg. Re-declare ARG and prefer unquoted expansion for ENV.Apply this diff right after the FROM line (before using WINDOWS_VERSION in RUN):
-ENV WINDOWS_VERSION="$WINDOWS_VERSION" +ARG WINDOWS_VERSION +ENV WINDOWS_VERSION=$WINDOWS_VERSIONIf you also want WINDOWS_VERSION available at runtime, mirror the same ARG/ENV pair immediately after the runtime FROM as well (see separate note below).
🧹 Nitpick comments (3)
dockerfiles/Dockerfile.windows (3)
34-37: Avoid passing an empty argument; build an args array instead (also fix comment typo)Passing an empty string as an ArgumentList item is brittle. Prefer conditionally appending the flag. Also fix “Bydefault” -> “By default”.
- # Bydefault includeRecommended is absent when using ltsc2025 - $includeRecommended="--includeRecommended"; ` - if ("${env:WINDOWS_VERSION}" -eq "ltsc2025") { $includeRecommended="" }; ` + # By default, omit --includeRecommended on ltsc2025 + $argsIncludeRecommended = @(); ` + if ("${env:WINDOWS_VERSION}" -ne "ltsc2025") { $argsIncludeRecommended += '--includeRecommended' }; `
49-53: Pass the conditional flag as an array (no empty item) and keep ArgumentList cleanSwap
$includeRecommendedfor the array built above to avoid an empty string being passed to Start-Process.- '--add Microsoft.VisualStudio.Component.Windows10SDK.19041', ` - '--add Microsoft.VisualStudio.Component.VC.CMake.Project', ` - $includeRecommended ` - -NoNewWindow -Wait; ` + '--add Microsoft.VisualStudio.Component.Windows10SDK.19041', ` + '--add Microsoft.VisualStudio.Component.VC.CMake.Project' ` + @argsIncludeRecommended ` + -NoNewWindow -Wait; `If you prefer a single, clearer ArgumentList, you can also construct it explicitly (outside this hunk) and pass it as one variable:
PowerShell snippet to insert where appropriate (no diff since it’s outside the selected lines):
$installArgs = @( '--quiet', '--wait', '--norestart', '--nocache', "--installPath ${env:MSVS_HOME}", "--channelUri ${msvs_build_tools_channel}", "--installChannelUri ${msvs_build_tools_channel}", '--add Microsoft.VisualStudio.Workload.VCTools', '--add Microsoft.VisualStudio.Component.Windows10SDK.19041', '--add Microsoft.VisualStudio.Component.VC.CMake.Project' ) + $argsIncludeRecommended Start-Process "${msvs_build_tools_dist}" -ArgumentList $installArgs -NoNewWindow -Wait;
197-197: Optional: expose WINDOWS_VERSION in the runtime image as wellIf the intent was to make WINDOWS_VERSION visible at runtime (as the original PR title/approach suggests), add ARG+ENV in the runtime stage too.
PowerShell/Dockerfile snippet to add immediately after the runtime FROM (no diff since it’s outside the selected lines):
ARG WINDOWS_VERSION ENV WINDOWS_VERSION=$WINDOWS_VERSIONIf you don’t need it at runtime, ignore this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dockerfiles/Dockerfile.windows(3 hunks)
⏰ 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 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: PR - Docker windows build test, windows 2022 and 2025 (2022)
- GitHub Check: PR - Docker windows build test, windows 2022 and 2025 (2025)
- GitHub Check: PR - Classic docker build test
- GitHub Check: PR - Buildkit docker build test
🔇 Additional comments (1)
dockerfiles/Dockerfile.windows (1)
49-50: Confirm component IDs across MSVS 16/17 and Windows variantsVerify that these component IDs are valid for both 2019 (16) and 2022 (17) channels you target, and that they exist on ltsc2025 images with the chosen channel manifest.
Quick checks to perform in CI logs locally:
- VS installer output should show each “--add …” resolved to a known component.
- No “unknown component id” errors for Windows10 SDK 19041 and VC.CMake.Project.
- Build tools exit code 0.
If you want, I can draft a minimal installer dry-run command to validate component IDs against each channel.
cosmo0920
changed the title
|
closing as the space issue was fixed here #10760 |
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
New Features
Chores