Fix conditions for packaging binaries for MacOS#27
Fix conditions for packaging binaries for MacOS#27ServeurpersoCom merged 1 commit intoServeurpersoCom:masterfrom
Conversation
Signed-off-by: Lorenzo Mangani <lorenzo.mangani@gmail.com>
📝 WalkthroughWalkthroughThe pull request refines GitHub Actions workflow logic by making OS-specific packaging step conditions more explicit and precise. Linux and macOS packaging steps now run based on exact OS matching rather than negation-based conditions, while Windows behavior remains unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 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)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
63-69:⚠️ Potential issue | 🟠 Major
nprocis not available on macOS.This step runs on macOS (per the
runner.os == 'macOS'condition), butnprocis a GNU coreutils command that doesn't exist on macOS. This will cause the command substitution to fail.Proposed fix using macOS-compatible command
cmake .. - cmake --build . --config Release -j "$(nproc)" + cmake --build . --config Release -j "$(sysctl -n hw.logicalcpu)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 63 - 69, The workflow step "Configure & Build (Linux / macOS)" uses the Linux-only command substitution "$(nproc)" in the cmake --build invocation; replace that with a cross-platform CPU count call such as getconf _NPROCESSORS_ONLN (or sysctl -n hw.ncpu on macOS) so the `cmake --build . --config Release -j ...` line works on macOS too; update the cmake --build invocation accordingly in the "Configure & Build (Linux / macOS)" step.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
58-69: Step names are misleading.Both steps are named "Configure & Build (Linux / macOS)" but each runs on only one OS. Consider renaming for clarity:
- Line 58: "Configure & Build (Linux)"
- Line 63: "Configure & Build (macOS)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 58 - 69, The two workflow steps both use the name "Configure & Build (Linux / macOS)" but are conditioned per OS; update the name string for the first step to "Configure & Build (Linux)" and the second step to "Configure & Build (macOS)" so the step names match their if: conditions (refer to the name fields for the steps that currently read "Configure & Build (Linux / macOS)" and the if: runner.os checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 63-69: The workflow step "Configure & Build (Linux / macOS)" uses
the Linux-only command substitution "$(nproc)" in the cmake --build invocation;
replace that with a cross-platform CPU count call such as getconf
_NPROCESSORS_ONLN (or sysctl -n hw.ncpu on macOS) so the `cmake --build .
--config Release -j ...` line works on macOS too; update the cmake --build
invocation accordingly in the "Configure & Build (Linux / macOS)" step.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 58-69: The two workflow steps both use the name "Configure & Build
(Linux / macOS)" but are conditioned per OS; update the name string for the
first step to "Configure & Build (Linux)" and the second step to "Configure &
Build (macOS)" so the step names match their if: conditions (refer to the name
fields for the steps that currently read "Configure & Build (Linux / macOS)" and
the if: runner.os checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61386c7a-8fb3-4533-907b-8a931334bcf1
📒 Files selected for processing (1)
.github/workflows/release.yml
|
Test run passing on fork: https://github.com/audiohacking/acestep.cpp/actions/runs/23086645652 |
Required fix to correctly handle each condition (bug derived from excluding the windows builds temporarily)
Summary by CodeRabbit