github: workflows: Specify OS configs explicitly#11934
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c43eeeee2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Windows packaging workflow now uses ChangesWindows Packaging Runner Matrix Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/call-build-windows.yaml (1)
115-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCMake architecture hardcoded to x86_64 may not be optimal for ARM runners.
The CMake setup downloads the x86_64 version regardless of the runner architecture:
$cmakeArch = "x86_64"If the ARM64 build runs on a native
windows-11-armrunner, using x86_64 CMake relies on emulation, which may cause:
- Performance degradation during the build
- Potential compatibility issues with native ARM toolchains
- Unexpected build failures
Recommendation: Consider making
$cmakeArchdynamic based on the target architecture or runner platform.🔧 Proposed fix to make CMake architecture configurable
- name: Set up CMake run: | $cmakeVersion = "${{ matrix.config.cmake_version }}" - $cmakeArch = "x86_64" + # Use x86_64 CMake for x86/x64 builds and cross-compilation; ARM64 for native ARM builds + if ("${{ matrix.config.arch }}" -eq "arm64") { + $cmakeArch = "arm64" + } else { + $cmakeArch = "x86_64" + } $cmakeUrl = "https://github.com/Kitware/CMake/releases/download/v${cmakeVersion}/cmake-${cmakeVersion}-windows-${cmakeArch}.zip"Note: This assumes a native ARM build; if cross-compilation is intended, keep x86_64.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/call-build-windows.yaml at line 115, The hardcoded $cmakeArch = "x86_64" forces x86 downloads on ARM runners; change $cmakeArch to be computed dynamically (e.g., inspect runner/OS vars such as $env:PROCESSOR_ARCHITECTURE or $env:RUNNER_ARCH and map values like "ARM64"/"AARCH64" to "arm64" and default to "x86_64"), allow an explicit override via an environment input/variable for cross-compilation scenarios, and update any downstream uses (references to $cmakeArch) to rely on that computed/overridable value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/call-build-windows.yaml:
- Around line 95-100: The workflow defines an ARM64 job with an incompatible
cross-compilation setting: change the arch/os pairing so they match the intended
build mode — either make the job native ARM by setting arch to "arm64" when
using os: windows-11-arm, or make it a cross-compile by keeping arch:
"amd64_arm64" and changing os to a x64 runner (e.g., windows-latest); update the
corresponding cmake_additional_opt/cmake_triplet if you switch to native arm64
to remove the cross-toolchain flags currently set in cmake_additional_opt and
ensure vcpkg_triplet matches the chosen mode.
---
Outside diff comments:
In @.github/workflows/call-build-windows.yaml:
- Line 115: The hardcoded $cmakeArch = "x86_64" forces x86 downloads on ARM
runners; change $cmakeArch to be computed dynamically (e.g., inspect runner/OS
vars such as $env:PROCESSOR_ARCHITECTURE or $env:RUNNER_ARCH and map values like
"ARM64"/"AARCH64" to "arm64" and default to "x86_64"), allow an explicit
override via an environment input/variable for cross-compilation scenarios, and
update any downstream uses (references to $cmakeArch) to rely on that
computed/overridable value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 782e2004-8f5b-4d07-8cfc-3633ce021b68
📒 Files selected for processing (1)
.github/workflows/call-build-windows.yaml
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/call-build-windows.yaml (1)
210-214: ⚡ Quick winCondition should check OS, not architecture.
The WiX installation is conditioned on
matrix.config.arch == 'amd64_arm64', but the actual intent is to install WiX onwindows-11-armrunners where it's not pre-installed (unlikewindows-latestrunners). Checking the architecture instead of the OS is fragile and obscures the intent.♻️ Recommended condition refactor
- - name: Get WiX Toolset w/ chocolatey - if: ${{ matrix.config.arch == 'amd64_arm64' }} + - name: Get WiX Toolset w/ chocolatey + if: matrix.config.os == 'windows-11-arm' uses: crazy-max/ghaction-chocolatey@dff3862348493b11fba2fbc49147b6d2dfe09b66 # v4.0.0 with: args: install wixtoolset -y🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/call-build-windows.yaml around lines 210 - 214, The step named "Get WiX Toolset w/ chocolatey" currently gates WiX installation on matrix.config.arch == 'amd64_arm64' which checks architecture instead of runner OS; update the step's if condition to target the Windows ARM runner (for example: matrix.config.os == 'windows-11-arm' or runner.os == 'Windows' && matrix.config.os == 'windows-11-arm' depending on your matrix shape) so the WiX install runs only on the windows-11-arm runner where WiX is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/call-build-windows.yaml:
- Around line 210-214: The step named "Get WiX Toolset w/ chocolatey" currently
gates WiX installation on matrix.config.arch == 'amd64_arm64' which checks
architecture instead of runner OS; update the step's if condition to target the
Windows ARM runner (for example: matrix.config.os == 'windows-11-arm' or
runner.os == 'Windows' && matrix.config.os == 'windows-11-arm' depending on your
matrix shape) so the WiX install runs only on the windows-11-arm runner where
WiX is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 724debae-d4ec-4555-90e8-79c601ba7ddd
📒 Files selected for processing (1)
.github/workflows/call-build-windows.yaml
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
1 participant
Recently, VS2026 18 bump is occurred so we need to address this incompatibility on GHA.
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