Skip to content

Fix conditions for packaging binaries for MacOS#27

Merged
ServeurpersoCom merged 1 commit intoServeurpersoCom:masterfrom
audiohacking:master
Mar 14, 2026
Merged

Fix conditions for packaging binaries for MacOS#27
ServeurpersoCom merged 1 commit intoServeurpersoCom:masterfrom
audiohacking:master

Conversation

@lmangani
Copy link
Contributor

@lmangani lmangani commented Mar 14, 2026

Required fix to correctly handle each condition (bug derived from excluding the windows builds temporarily)

Summary by CodeRabbit

  • Chores
    • Improved build pipeline accuracy by ensuring platform-specific binaries are packaged only on their respective operating systems, resulting in more reliable release artifacts.

Signed-off-by: Lorenzo Mangani <lorenzo.mangani@gmail.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 14, 2026

📝 Walkthrough

Walkthrough

The 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

Cohort / File(s) Summary
CI/CD Workflow Configuration
.github/workflows/release.yml
Refined OS-specific conditional logic: Linux packaging now runs only on runner.os == 'Linux' (previously runner.os != 'Windows'), and macOS packaging now runs only on runner.os == 'macOS' (previously runner.os != 'macOS'). Windows packaging condition unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

🐰 A rabbit hops through workflows clear,
Making conditions bright and dear,
No more "not-this" or "that-instead,"
Just "exactly here," the rabbit said! 🚀

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: fixing conditions for macOS binary packaging in the workflow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

@lmangani lmangani marked this pull request as ready for review March 14, 2026 11:01
Copy link
Contributor

@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.

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

nproc is not available on macOS.

This step runs on macOS (per the runner.os == 'macOS' condition), but nproc is 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

📥 Commits

Reviewing files that changed from the base of the PR and between ab60d7a and 9052d8d.

📒 Files selected for processing (1)
  • .github/workflows/release.yml

@lmangani
Copy link
Contributor Author

@ServeurpersoCom ServeurpersoCom merged commit bc39a50 into ServeurpersoCom:master Mar 14, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants