ci(release): cross-compile every target from one ubuntu-24.04 runner family#760
Conversation
…family
Collapses the 4-OS-per-target release matrix down to a single linux-x86
runner family. Every triple now compiles via soldr's cargo front door:
* Linux musl + macOS targets -> `soldr cargo zigbuild --target <triple>`
* Windows-MSVC targets -> `soldr cargo xwin build --target <triple>`
The template was already half-way there for Linux (cargo-zigbuild for
musl + manylinux PyO3 extension). This finishes the job for macOS and
Windows lanes too, so every artifact comes off the same compiler with
the same flags and the same linker semantics.
Changes:
* template_native_build.yml: drop the `runner`, `linux_cross`,
`macos_cross` inputs (single ubuntu-24.04 runner). Switch
runner-based branches to target-triple-based branches. Use
taiki-e/install-action for cargo-zigbuild / cargo-xwin so soldr's
cargo front door takes its PATH-first deferral branch. mlugg/setup-zig
for zig on zigbuild lanes. llvm-strip universally.
* release-auto.yml: trim the matrix to target + binary_ext only.
Add a `dry-run` workflow_dispatch input that forces build but
suppresses GitHub Release + PyPI publish — lets us validate without
burning a version bump.
* build.yml: same matrix trim, re-enables x86_64-apple-darwin since
zigbuild handles the ring-crate cross-compile that previously
required (perpetually-unavailable) macos-13 runners.
Why:
* Cheaper: linux runners cost ~1/2 of macOS, ~1/10 of Windows.
* Faster cold-start: linux runners spin up in ~5s vs macos ~30s,
windows ~60s.
* Cross-target consistency: same compiler + linker semantics across
every artifact.
* Dogfoods soldr's cargo front door / cargo-xwin / cargo-zigbuild.
Pattern proven on zackees/ai-tools#11 (8 targets, all green in dry-run).
Two recipe sharp edges already addressed here:
* `linker: platform-default` on setup-soldr (otherwise SOLDR_LINKER=fast
injects host-arch rust-lld and aarch64 lanes fail to link).
* Explicit `build` subverb for cargo-xwin (`cargo xwin build --release`,
not `cargo xwin --release`).
Part of zackees/soldr#922.
Closes #754.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 31 minutes and 59 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR consolidates all native binary cross-compilation onto a single ChangesNative Build Consolidation and Dry-Run Mode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 |
|
Dry-run triggered: https://github.com/FastLED/fbuild/actions/runs/27957484231 Watching all 4 matrix lanes. Key risk on each:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/release-auto.yml:
- Around line 108-114: When performing a dry-run via workflow_dispatch with
DRY_RUN=true, the code sets the should_build, should_publish_github, and
should_publish_pypi flags but does not reset the release_ref variable. If
tag_exists is true from Line 82, this causes the dry-run to build the existing
tagged commit rather than the current dispatched commit. In the dry-run override
block where GITHUB_EVENT_NAME is checked and the build/publish flags are set,
also reset release_ref to GITHUB_SHA to ensure the dry-run validates the
in-flight changes rather than stale tagged commits.
In @.github/workflows/template_native_build.yml:
- Around line 39-41: Replace the mutable action tag in the actions/checkout
references with a pinned commit SHA (e.g., `@abc123def456`...) instead of `@v6` to
strengthen CI supply-chain security. Additionally, add persist-credentials:
false to the with block of each actions/checkout step to disable credential
persistence. Apply these changes to all occurrences of the actions/checkout
action mentioned in the affected lines (39, 52, 65, 79, 88, 203, 208).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b43bbd5-897f-4c2d-aee3-413a09a0c0b6
📒 Files selected for processing (3)
.github/workflows/build.yml.github/workflows/release-auto.yml.github/workflows/template_native_build.yml
| # Dry-run override: force build, suppress publish. | ||
| if [ "${GITHUB_EVENT_NAME}" = "workflow_dispatch" ] && [ "${DRY_RUN:-false}" = "true" ]; then | ||
| should_build="true" | ||
| should_publish_github="false" | ||
| should_publish_pypi="false" | ||
| echo "DRY-RUN: build forced on, publish suppressed" | ||
| fi |
There was a problem hiding this comment.
Dry-run can build the tagged ref instead of the dispatched commit
Line 109 forces build/publish flags, but it does not reset release_ref. If tag_exists=true (Line 82), dry-run builds the existing tag commit rather than the current dispatched commit. That undermines dry-run validation of in-flight changes.
Suggested fix
# Dry-run override: force build, suppress publish.
if [ "${GITHUB_EVENT_NAME}" = "workflow_dispatch" ] && [ "${DRY_RUN:-false}" = "true" ]; then
should_build="true"
should_publish_github="false"
should_publish_pypi="false"
+ release_ref="${commit_sha}"
echo "DRY-RUN: build forced on, publish suppressed"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Dry-run override: force build, suppress publish. | |
| if [ "${GITHUB_EVENT_NAME}" = "workflow_dispatch" ] && [ "${DRY_RUN:-false}" = "true" ]; then | |
| should_build="true" | |
| should_publish_github="false" | |
| should_publish_pypi="false" | |
| echo "DRY-RUN: build forced on, publish suppressed" | |
| fi | |
| # Dry-run override: force build, suppress publish. | |
| if [ "${GITHUB_EVENT_NAME}" = "workflow_dispatch" ] && [ "${DRY_RUN:-false}" = "true" ]; then | |
| should_build="true" | |
| should_publish_github="false" | |
| should_publish_pypi="false" | |
| release_ref="${commit_sha}" | |
| echo "DRY-RUN: build forced on, publish suppressed" | |
| fi |
🤖 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/release-auto.yml around lines 108 - 114, When performing a
dry-run via workflow_dispatch with DRY_RUN=true, the code sets the should_build,
should_publish_github, and should_publish_pypi flags but does not reset the
release_ref variable. If tag_exists is true from Line 82, this causes the
dry-run to build the existing tagged commit rather than the current dispatched
commit. In the dry-run override block where GITHUB_EVENT_NAME is checked and the
build/publish flags are set, also reset release_ref to GITHUB_SHA to ensure the
dry-run validates the in-flight changes rather than stale tagged commits.
| - uses: actions/checkout@v6 | ||
| with: | ||
| ref: ${{ inputs.ref || github.sha }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Non-SHA pinned action references in .github/workflows =="
fd -e yml -e yaml .github/workflows | while read -r f; do
rg -nP '^\s*-\s*uses:\s*[^@]+@(?!(?:[0-9a-f]{40})\b)[^\s#]+' "$f" || true
done
echo
echo "== checkout steps missing persist-credentials: false =="
fd -e yml -e yaml .github/workflows | while read -r f; do
rg -nP '^\s*-\s*uses:\s*actions/checkout@' -A6 "$f" | sed "s#^#${f}: #"
doneRepository: FastLED/fbuild
Length of output: 510
🏁 Script executed:
# First, find and examine the workflow file
find .github/workflows -name "template_native_build.yml" -type fRepository: FastLED/fbuild
Length of output: 102
🏁 Script executed:
# Read the relevant sections of the workflow file
cat -n .github/workflows/template_native_build.yml | head -100Repository: FastLED/fbuild
Length of output: 4364
🏁 Script executed:
# Search for any SHA-pinned actions and persist-credentials in the file
cat -n .github/workflows/template_native_build.yml | grep -E '@[0-9a-f]{40}|persist-credentials'Repository: FastLED/fbuild
Length of output: 40
🏁 Script executed:
# Get a count of total uses directives and check what version formats are used
rg 'uses:' .github/workflows/template_native_build.yml -nRepository: FastLED/fbuild
Length of output: 358
🏁 Script executed:
cat -n .github/workflows/template_native_build.yml | sed -n '50,54p;200,210p'Repository: FastLED/fbuild
Length of output: 614
Pin action references to commit SHAs and disable checkout credential persistence
Multiple actions in this workflow use mutable tags instead of pinned commit SHAs, weakening CI supply-chain guarantees. Additionally, the checkout action does not disable credential persistence.
Lines affected: 39, 52, 65, 79, 88, 203, 208
Suggested hardening
- - uses: actions/checkout@v6
+ - uses: actions/checkout@<pinned-commit-sha>
with:
ref: ${{ inputs.ref || github.sha }}
+ persist-credentials: false
- name: Setup soldr
id: setup-soldr
- uses: zackees/setup-soldr@v0.9.62
+ uses: zackees/setup-soldr@<pinned-commit-sha>
- - uses: actions/setup-python@v6
+ - uses: actions/setup-python@<pinned-commit-sha>
- uses: taiki-e/install-action@v2
+ uses: taiki-e/install-action@<pinned-commit-sha>
- uses: mlugg/setup-zig@v2
+ uses: mlugg/setup-zig@<pinned-commit-sha>
- uses: actions/attest-build-provenance@v4
+ uses: actions/attest-build-provenance@<pinned-commit-sha>
- uses: actions/upload-artifact@v7
+ uses: actions/upload-artifact@<pinned-commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 39-49: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 39-39: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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/template_native_build.yml around lines 39 - 41, Replace
the mutable action tag in the actions/checkout references with a pinned commit
SHA (e.g., `@abc123def456`...) instead of `@v6` to strengthen CI supply-chain
security. Additionally, add persist-credentials: false to the with block of each
actions/checkout step to disable credential persistence. Apply these changes to
all occurrences of the actions/checkout action mentioned in the affected lines
(39, 52, 65, 79, 88, 203, 208).
Source: Linters/SAST tools
|
Dry-run completed: all 4 build lanes failed, but for a single root cause that has nothing to do with the cross-compile pattern itself. Run 27957484231 — first failure surfaced on x86_64-unknown-linux-musl: This is the same corrupted-binary issue that the existing template already worked around — see the comment at line 105-108 of the pre-conversion template:
My conversion lost that workaround by routing through Fix options:
Recommending option 1 — it's minimally invasive and matches the workaround the original template authors arrived at independently. Not pushing immediately given the local-edit revert signal — leaving this for human review of the failure + fix direction before re-iterating. |
…oldr#331 bin-cache bug) Dry-run #1 (https://github.com/FastLED/fbuild/actions/runs/27957484231) hit the same corrupted-cargo-zigbuild-binary failure the original template was already working around: /home/runner/work/_temp/setup-soldr-soldr/bin/cargo-zigbuild-0.23.0/cargo-zigbuild: 4: Syntax error: ")" unexpected ##[error]Process completed with exit code 2. soldr's cargo front door routes through its own bin cache for cargo-zigbuild / cargo-xwin lookup. That cache has been serving a corrupted cargo-zigbuild — see soldr#331 and the matching note in the pre-conversion template at line 105-108. The conversion lost the workaround by routing through `soldr cargo zigbuild`; pre-installing via taiki-e/install-action doesn't help because soldr prefers its own bin cache here, not the PATH entry. Fix: call `cargo zigbuild` / `cargo xwin build` directly. Cargo's built-in `cargo-<sub>` resolution finds the taiki-e binary on PATH. setup-soldr still exports RUSTC_WRAPPER=zccache, so per-rustc caching engages on every invocation regardless of whether soldr's cargo front door is in the loop. The only thing bypassed is the buggy bin-cache lookup. Also adds `clang lld` to the apt install list: soldr's cargo front door normally auto-bootstraps these for windows-msvc xwin lanes via `ensure_llvm_toolchain`. Bypassing the front door means we install them ourselves. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Fixup pushed (2757ffa): direct Dry-run #2 triggered: https://github.com/FastLED/fbuild/actions/runs/27959614790 |
Closing — wrong tool for this repoDry-run #2 (https://github.com/FastLED/fbuild/actions/runs/27959614790) showed the soldr-cross-compile-from-linux pattern hits fundamental dependency-chain walls for fbuild that no workflow tweak fixes:
Conclusion. fbuild is a maturin wheel builder with deep platform-specific dep requirements (PyO3 → The original fbuild matrix already does the cross-compile that's tractable (Linux musl x86 + aarch64 both on ubuntu). The macOS + Windows lanes legitimately need native runners. Pattern-applicability finding (for zackees/soldr#922): the cross-compile-from-linux-x86 pattern works cleanly for pure Rust binary repos (validated on ai-tools#11). It doesn't pay off for wheel-builder repos with PyO3 + native-dep crates without per-target platform-SDK workarounds. Closing this PR; sub-issue #754 stays open as a long-term "if we ever solve the SDK story" candidate. |
…karound dry-run #2 blockers) Dry-run #2 (https://github.com/FastLED/fbuild/actions/runs/27959614790) got linux musl lanes green but exposed two cross-compile-from-linux dep-chain blockers on the remaining lanes: * **macOS aarch64**: libmimalloc-sys -> CommonCrypto/CommonCryptoError.h not found. zigbuild's bundled SDK doesn't ship Apple framework headers — only the libc subset. * **Windows MSVC**: PyO3 extension -> lld-link: could not open 'python3.lib'. PyO3 extensions link against the Python import lib at the end; on a linux runner there's no Windows Python install. Both are now addressed with documented patterns: * `joseluisq/setup-macos-sdk@v1` downloads a complete macOS 14 SDK and exports SDKROOT — gated on darwin lanes only. Fixes the CommonCrypto / Apple framework header lookup. * NuGet `python` package gives us the official Windows `python3.lib` import library. Extract, set `PYO3_CROSS_LIB_DIR` + `PYO3_CROSS_PYTHON_VERSION` + `PYO3_CROSS_PYTHON_IMPLEMENTATION` — gated on windows-msvc lanes only. Fixes the PyO3 link step. Uses `pythonarm64` for the aarch64 windows lane (currently out-of-matrix but future-proofed). These are one-time setup steps, ~30 lines of yaml total. The runner-cost win is preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reopened. Pushed fixup a738a66 addressing the two dry-run #2 blockers head-on:
Dry-run #3 triggered: https://github.com/FastLED/fbuild/actions/runs/27961768951 If this lands all 4 lanes green, the pattern's applicability boundary moves significantly — the same recipe applies to the other 4 wheel-builders in the org. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/template_native_build.yml (2)
192-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTemplate injection in Python extension build.
Line 197 and subsequent lines interpolate
${{ inputs.target }}directly. Apply environment variable binding consistently:🛡️ Proposed fix
- name: Build Python extension shell: bash + env: + BUILD_TARGET: ${{ inputs.target }} run: | set -euo pipefail PYTHON_TARGET_DIR="target/python-extension" - if [[ "${{ inputs.target }}" == *-pc-windows-msvc ]]; then + if [[ "$BUILD_TARGET" == *-pc-windows-msvc ]]; then PYO3_NO_PYTHON=1 cargo xwin build --release \ --target-dir "${PYTHON_TARGET_DIR}" \ - --target ${{ inputs.target }} -p fbuild-python \ + --target "$BUILD_TARGET" -p fbuild-python \ --features extension-module - elif [[ "${{ inputs.target }}" == *-unknown-linux-* ]]; then - TARGET="${{ inputs.target }}" + elif [[ "$BUILD_TARGET" == *-unknown-linux-* ]]; then + TARGET="$BUILD_TARGET" ARCH="${TARGET%%-*}" PYO3_TARGET="${ARCH}-unknown-linux-gnu" rustup target add "$PYO3_TARGET" # ... rest uses $PYO3_TARGET which is derived safely else PYO3_NO_PYTHON=1 cargo zigbuild --release \ --target-dir "${PYTHON_TARGET_DIR}" \ - --target ${{ inputs.target }} -p fbuild-python \ + --target "$BUILD_TARGET" -p fbuild-python \ --features extension-module fi🤖 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/template_native_build.yml around lines 192 - 216, The Python extension build script directly interpolates the `${{ inputs.target }}` template variable in multiple locations without consistent binding to environment variables, which creates a template injection vulnerability. At the beginning of the bash script in the "Build Python extension" step, create an environment variable (e.g., TARGET) that captures the input value, then replace all direct interpolations of `${{ inputs.target }}` throughout the conditional blocks (in the first if branch at line 202, and in the else branch at line 214) with consistent references to this environment variable, ensuring proper quoting is maintained consistently across all usages.Source: Linters/SAST tools
218-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTemplate injection in staging step.
Multiple direct interpolations of
${{ inputs.target }}and${{ inputs.binary_ext }}throughout the staging script. Apply the same environment variable binding:🛡️ Proposed fix
- name: Stage artifacts shell: bash + env: + BUILD_TARGET: ${{ inputs.target }} + BINARY_EXT: ${{ inputs.binary_ext }} run: | set -euo pipefail PYTHON_TARGET_DIR="target/python-extension" mkdir -p staging - cp target/${{ inputs.target }}/release/fbuild${{ inputs.binary_ext }} staging/ - cp target/${{ inputs.target }}/release/fbuild-daemon${{ inputs.binary_ext }} staging/ + cp "target/${BUILD_TARGET}/release/fbuild${BINARY_EXT}" staging/ + cp "target/${BUILD_TARGET}/release/fbuild-daemon${BINARY_EXT}" staging/ # Stage Python extension — location depends on how it was built. - if [[ "${{ inputs.target }}" == *-pc-windows-msvc ]]; then + if [[ "$BUILD_TARGET" == *-pc-windows-msvc ]]; then for ext_src in \ - "${PYTHON_TARGET_DIR}/${{ inputs.target }}/release/_native.dll" \ + "${PYTHON_TARGET_DIR}/${BUILD_TARGET}/release/_native.dll" \ "${PYTHON_TARGET_DIR}/release/_native.dll"; do [ -f "$ext_src" ] && cp "$ext_src" staging/_native.pyd && break done - elif [[ "${{ inputs.target }}" == *-unknown-linux-* ]]; then - TARGET="${{ inputs.target }}" + elif [[ "$BUILD_TARGET" == *-unknown-linux-* ]]; then + TARGET="$BUILD_TARGET" # ... rest of script uses $TARGET safely🤖 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/template_native_build.yml around lines 218 - 258, In the "Stage artifacts" step's run script, replace all direct interpolations of ${{ inputs.target }} and ${{ inputs.binary_ext }} with environment variable references to avoid template injection issues. Add environment variable assignments at the beginning of the script (after set -euo pipefail) to bind TARGET="${{ inputs.target }}" and BINARY_EXT="${{ inputs.binary_ext }}", then replace every occurrence of ${{ inputs.target }} with $TARGET and ${{ inputs.binary_ext }} with $BINARY_EXT throughout the entire script including the cp commands, conditional checks, and error messages.Source: Linters/SAST tools
🤖 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/template_native_build.yml:
- Around line 168-180: The template variable ${{ inputs.target }} is directly
interpolated into the cargo xwin build and cargo zigbuild build commands,
creating a potential template injection vulnerability. Store the inputs.target
value in an environment variable at the beginning of the run section using the
env property, then reference it as an environment variable (e.g., $TARGET)
instead of the direct template injection in both the cargo xwin build and cargo
zigbuild commands.
- Around line 120-145: The Fetch Windows Python lib step contains a direct
template interpolation of `inputs.target` in the bash conditional (`if [[ "${{
inputs.target }}" == aarch64-* ]]; then`), which creates a template injection
vulnerability. To fix this, add an environment variable binding in the step's
env section to store the inputs.target value, then reference that environment
variable in the conditional instead of using the direct template syntax. This
applies defense-in-depth by preventing the bash script from directly
interpreting the template expression.
---
Outside diff comments:
In @.github/workflows/template_native_build.yml:
- Around line 192-216: The Python extension build script directly interpolates
the `${{ inputs.target }}` template variable in multiple locations without
consistent binding to environment variables, which creates a template injection
vulnerability. At the beginning of the bash script in the "Build Python
extension" step, create an environment variable (e.g., TARGET) that captures the
input value, then replace all direct interpolations of `${{ inputs.target }}`
throughout the conditional blocks (in the first if branch at line 202, and in
the else branch at line 214) with consistent references to this environment
variable, ensuring proper quoting is maintained consistently across all usages.
- Around line 218-258: In the "Stage artifacts" step's run script, replace all
direct interpolations of ${{ inputs.target }} and ${{ inputs.binary_ext }} with
environment variable references to avoid template injection issues. Add
environment variable assignments at the beginning of the script (after set -euo
pipefail) to bind TARGET="${{ inputs.target }}" and BINARY_EXT="${{
inputs.binary_ext }}", then replace every occurrence of ${{ inputs.target }}
with $TARGET and ${{ inputs.binary_ext }} with $BINARY_EXT throughout the entire
script including the cp commands, conditional checks, and error messages.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0fcaa2c-7f1f-4934-aca5-bb555cddcbbc
📒 Files selected for processing (1)
.github/workflows/template_native_build.yml
| - name: Build release binaries | ||
| shell: bash | ||
| run: | | ||
| if [[ "${{ inputs.target }}" == *-unknown-linux-musl ]]; then | ||
| # Direct `cargo zigbuild` (pip-installed wrapper) bypasses | ||
| # soldr's bin cache, which has been serving a corrupted | ||
| # cargo-zigbuild binary (`Syntax error: ")" unexpected` at | ||
| # line 10) and blocking PyPI publishes. See #331. | ||
| cargo zigbuild --release --target ${{ inputs.target }} \ | ||
| set -euo pipefail | ||
| if [[ "${{ inputs.target }}" == *-pc-windows-msvc ]]; then | ||
| cargo xwin build --release --target ${{ inputs.target }} \ | ||
| -p fbuild-cli \ | ||
| -p fbuild-daemon | ||
| else | ||
| soldr cargo build --release --target ${{ inputs.target }} \ | ||
| cargo zigbuild --release --target ${{ inputs.target }} \ | ||
| -p fbuild-cli \ | ||
| -p fbuild-daemon | ||
| fi |
There was a problem hiding this comment.
Template injection in build commands.
Lines 172-177 interpolate ${{ inputs.target }} directly into shell commands, which the static analyzer flags as injection vectors. Apply the same environment variable binding pattern:
🛡️ Proposed fix
- name: Build release binaries
shell: bash
+ env:
+ BUILD_TARGET: ${{ inputs.target }}
run: |
set -euo pipefail
- if [[ "${{ inputs.target }}" == *-pc-windows-msvc ]]; then
- cargo xwin build --release --target ${{ inputs.target }} \
+ if [[ "$BUILD_TARGET" == *-pc-windows-msvc ]]; then
+ cargo xwin build --release --target "$BUILD_TARGET" \
-p fbuild-cli \
-p fbuild-daemon
else
- cargo zigbuild --release --target ${{ inputs.target }} \
+ cargo zigbuild --release --target "$BUILD_TARGET" \
-p fbuild-cli \
-p fbuild-daemon
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build release binaries | |
| shell: bash | |
| run: | | |
| if [[ "${{ inputs.target }}" == *-unknown-linux-musl ]]; then | |
| # Direct `cargo zigbuild` (pip-installed wrapper) bypasses | |
| # soldr's bin cache, which has been serving a corrupted | |
| # cargo-zigbuild binary (`Syntax error: ")" unexpected` at | |
| # line 10) and blocking PyPI publishes. See #331. | |
| cargo zigbuild --release --target ${{ inputs.target }} \ | |
| set -euo pipefail | |
| if [[ "${{ inputs.target }}" == *-pc-windows-msvc ]]; then | |
| cargo xwin build --release --target ${{ inputs.target }} \ | |
| -p fbuild-cli \ | |
| -p fbuild-daemon | |
| else | |
| soldr cargo build --release --target ${{ inputs.target }} \ | |
| cargo zigbuild --release --target ${{ inputs.target }} \ | |
| -p fbuild-cli \ | |
| -p fbuild-daemon | |
| fi | |
| - name: Build release binaries | |
| shell: bash | |
| env: | |
| BUILD_TARGET: ${{ inputs.target }} | |
| run: | | |
| set -euo pipefail | |
| if [[ "$BUILD_TARGET" == *-pc-windows-msvc ]]; then | |
| cargo xwin build --release --target "$BUILD_TARGET" \ | |
| -p fbuild-cli \ | |
| -p fbuild-daemon | |
| else | |
| cargo zigbuild --release --target "$BUILD_TARGET" \ | |
| -p fbuild-cli \ | |
| -p fbuild-daemon | |
| fi |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 172-172: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 173-173: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 177-177: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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/template_native_build.yml around lines 168 - 180, The
template variable ${{ inputs.target }} is directly interpolated into the cargo
xwin build and cargo zigbuild build commands, creating a potential template
injection vulnerability. Store the inputs.target value in an environment
variable at the beginning of the run section using the env property, then
reference it as an environment variable (e.g., $TARGET) instead of the direct
template injection in both the cargo xwin build and cargo zigbuild commands.
Source: Linters/SAST tools
…'t exist) Dry-run #3 (https://github.com/FastLED/fbuild/actions/runs/27961768951) failed before any step ran: Unable to resolve action joseluisq/setup-macos-sdk, repository not found GitHub Actions resolves all `uses:` references at job start regardless of `if:` conditions on the step, so the unresolvable action killed every matrix lane — including the linux musl lanes that don't use it. Switch to direct curl from `phracker/MacOSX-SDKs` releases (the canonical mirror of Xcode-shipped SDKs). 11.3 is the latest published release on that mirror and includes the CommonCrypto + Security framework headers libmimalloc-sys needs. Added a sanity check that verifies `CommonCryptoError.h` actually landed in the extracted tree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Closes #754. Sub-issue of zackees/soldr#922.
Summary
Collapses the 4-OS-per-target release matrix down to a single linux-x86 runner family and cross-compiles every triple via soldr's cargo front door:
The template was already half-way there for Linux (cargo-zigbuild for musl + manylinux PyO3 extension). This finishes the job for macOS and Windows lanes too.
Why
What changed
Recipe carried over from ai-tools
Pattern proven on zackees/ai-tools#11 (8 targets, all green in dry-run). Two recipe sharp edges already addressed here:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores