Skip to content

Update Actions w/ new bin naming#29

Merged
ServeurpersoCom merged 6 commits intoServeurpersoCom:masterfrom
audiohacking:action-builder
Mar 16, 2026
Merged

Update Actions w/ new bin naming#29
ServeurpersoCom merged 6 commits intoServeurpersoCom:masterfrom
audiohacking:action-builder

Conversation

@lmangani
Copy link
Contributor

@lmangani lmangani commented Mar 16, 2026

Summary by CodeRabbit

  • Chores
    • Updated build and release workflows for improved efficiency.
    • Added caching for non-Windows CI runners to speed Linux/macOS builds.
    • Adjusted smoke test invocations used in CI.
    • Broadened and standardized packaged binaries across Linux, macOS, and Windows with platform-specific packaging tweaks.
    • Aligned upload/release steps to the updated artifact set.

Updated packaging process for macOS binaries to include rpath and additional libraries.

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

coderabbitai bot commented Mar 16, 2026

📝 Walkthrough

Walkthrough

Swaps smoke-test binaries (replace ace-qwen3 and dit-vae with ace-lm and ace-synth) in CI and release workflows, adds a ccache step for non-Windows runners, and adjusts packaging copy/install steps across Linux, macOS, and Windows to reflect the new binary set.

Changes

Cohort / File(s) Summary
Smoke Test Updates
.github/workflows/ci-build.yml, .github/workflows/release.yml
Replaced smoke-test invocations of ./build/ace-qwen3 --help and ./build/dit-vae --help with ./build/ace-lm --help and ./build/ace-synth --help; retained other smoke tests such as quantize.
Build Optimization
.github/workflows/release.yml
Added a ccache setup step for non-Windows runners to enable build caching in Linux/macOS build path.
Linux Packaging
.github/workflows/release.yml
Changed copy logic to include wildcard ace-* binaries (instead of explicit ace-qwen3/dit-vae), plus mp3-codec and relevant shared objects for distribution.
macOS Packaging
.github/workflows/release.yml
Adjusted install_name_tool/rpath handling and packaging to iterate over ace-*, quantize, neural-codec, mp3-codec; copying also includes libacestep*.a and libggml*.dylib.
Windows Packaging
.github/workflows/release.yml
Updated Windows distribution binary list to include ace-lm and ace-synth instead of ace-qwen3 and dit-vae; other binaries (ace-understand, quantize, neural-codec, mp3-codec) remain included.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 In pipelines I nibble and hop,
Old names swapped — I never stop.
ace-qwen3 hops out, ace-lm hops in,
ace-synth joins the packaging bin.
Ccache hums, the artifacts grin. ✨

🚥 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 describes the main change: updating CI/CD workflow files to reflect new binary naming conventions (ace-lm, ace-synth replacing ace-qwen3, dit-vae).
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.

Tip

You can disable poems in the walkthrough.

Disable the reviews.poem setting to disable the poems in the walkthrough.

@lmangani lmangani marked this pull request as ready for review March 16, 2026 11:05
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.

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/release.yml (1)

70-76: ⚠️ Potential issue | 🟠 Major

nproc is not available on macOS.

Line 76 uses $(nproc) which is a Linux-specific utility. On macOS, this command will fail. The CI workflow correctly uses $(sysctl -n hw.ncpu) for macOS (see line 36 of ci-build.yml).

🐛 Proposed fix
       - name: Configure & Build (Linux / macOS)
         if: runner.os == 'macOS'
         run: |
           mkdir build
           cd build
           cmake ..
-          cmake --build . --config Release -j "$(nproc)"
+          cmake --build . --config Release -j "$(sysctl -n hw.ncpu)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/release.yml around lines 70 - 76, The macOS build step in
the "Configure & Build (Linux / macOS)" job uses the Linux-only "$(nproc)";
change the macOS command in that run block (the Configure & Build step) to use
the macOS CPU count command "$(sysctl -n hw.ncpu)" (or detect platform and
choose between nproc and sysctl) so the cmake --build invocation uses the
correct parallelism on macOS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/release.yml:
- Around line 127-134: Fix the typo in the copy command: replace the incorrect
glob "ace-ace-*" with "ace-*" so the cp -P command copies the actual ace
binaries (ace-*), quantize, neural-codec, mp3-codec, libacestep*.a and
libggml*.dylib into ../dist/; update the cp invocation that currently references
"ace-ace-*" to use "ace-*" (the rest of the command remains unchanged).

---

Outside diff comments:
In @.github/workflows/release.yml:
- Around line 70-76: The macOS build step in the "Configure & Build (Linux /
macOS)" job uses the Linux-only "$(nproc)"; change the macOS command in that run
block (the Configure & Build step) to use the macOS CPU count command "$(sysctl
-n hw.ncpu)" (or detect platform and choose between nproc and sysctl) so the
cmake --build invocation uses the correct parallelism on macOS.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 881487a0-124b-419d-8211-a4cac27bc938

📥 Commits

Reviewing files that changed from the base of the PR and between b6593f4 and 1255975.

📒 Files selected for processing (2)
  • .github/workflows/ci-build.yml
  • .github/workflows/release.yml

Signed-off-by: Lorenzo Mangani <lorenzo.mangani@gmail.com>
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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
.github/workflows/release.yml (1)

127-132: Prefer explicit macOS binary list over ace-* wildcard for release determinism.

ace-* in both install_name_tool and cp is broader than needed and can accidentally include non-release matches, making packaging brittle.

♻️ Proposed hardening
           cd build
-          for bin in ace-* quantize neural-codec mp3-codec; do
+          bins=(ace-lm ace-synth ace-understand quantize neural-codec mp3-codec)
+          for bin in "${bins[@]}"; do
             install_name_tool -add_rpath `@executable_path` "$bin"
           done
-          cp -P ace-* quantize neural-codec mp3-codec libacestep*.a libggml*.dylib ../dist/
+          cp -P "${bins[@]}" libacestep*.a libggml*.dylib ../dist/
           cd ..
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/release.yml around lines 127 - 132, The release step uses
a broad ace-* wildcard in the install_name_tool loop and cp command which can
pull unintended files; replace the wildcard with an explicit, deterministic list
of macOS binaries (the exact names used in the build) in both the for-loop that
calls install_name_tool and the cp -P command so only intended artifacts are
processed and packaged (update the install_name_tool loop and the cp invocation
to reference that explicit list instead of ace-*).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/release.yml:
- Around line 140-143: The Windows packaging block (the $bins array and foreach
($b in $bins) Copy-Item "build\Release\$b.exe" dist\) is unreachable because the
workflow matrix only includes Linux and macOS runners; either add a Windows
entry to the job matrix so these steps run on a Windows runner, or remove/guard
this Windows-specific packaging code behind a condition (e.g., if: runner.os ==
'Windows') so it only executes when a Windows runner (matrix entry) is present.

---

Nitpick comments:
In @.github/workflows/release.yml:
- Around line 127-132: The release step uses a broad ace-* wildcard in the
install_name_tool loop and cp command which can pull unintended files; replace
the wildcard with an explicit, deterministic list of macOS binaries (the exact
names used in the build) in both the for-loop that calls install_name_tool and
the cp -P command so only intended artifacts are processed and packaged (update
the install_name_tool loop and the cp invocation to reference that explicit list
instead of ace-*).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c2f835f8-f109-447d-ac34-64c1ecfa6bb2

📥 Commits

Reviewing files that changed from the base of the PR and between 1255975 and 9a7f390.

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

@lmangani lmangani marked this pull request as draft March 16, 2026 11:32
@ServeurpersoCom ServeurpersoCom marked this pull request as ready for review March 16, 2026 12:25
@ServeurpersoCom ServeurpersoCom merged commit 955190c into ServeurpersoCom:master Mar 16, 2026
3 of 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