Update unsupported CPU target case in CI workflow#217
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe GitHub Actions workflow adds error handling to the CPU dispatch logic by introducing a default case that rejects unsupported CPU targets with a logged error message and exit code 1, preventing silent failures for unrecognized architectures. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
frostney
force-pushed
the
fix/cross-package-bootstrap
branch
from
24401db to
a7070b0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/toolchain.yml:
- Around line 288-299: After copying package unit dirs, add a hard validation
that at least one package was installed for the current ${target} and fail the
job if not; use the same units_base/target variables used in the loop
(pkg_units_dir, units_base, target) to compute the installed count (e.g. count
directories under "${units_base}"/*/) and if the count is zero (or below your
required representative non-RTL threshold) print a clear error and exit 1 so the
workflow stops before saving the cache.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8fe5de8-633a-4c19-b7f9-fece74f88f82
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/toolchain.ymldocs/build-system.md
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 177 improved · 🔴 32 regressed · 65 unchanged · avg +4.3% arraybuffer.js — Interp: 🟢 12, 🔴 1, 1 unch. · avg +8.3% · Bytecode: 🔴 12, 2 unch. · avg -5.0%
arrays.js — Interp: 🟢 18, 1 unch. · avg +6.7% · Bytecode: 🟢 1, 🔴 16, 2 unch. · avg -3.4%
async-await.js — Interp: 🟢 6 · avg +9.5% · Bytecode: 🔴 6 · avg -7.7%
classes.js — Interp: 🟢 23, 🔴 4, 4 unch. · avg +4.2% · Bytecode: 🔴 15, 16 unch. · avg -2.6%
closures.js — Interp: 🟢 9, 2 unch. · avg +4.6% · Bytecode: 🟢 1, 🔴 5, 5 unch. · avg -1.7%
collections.js — Interp: 🟢 9, 🔴 2, 1 unch. · avg +12.1% · Bytecode: 🔴 11, 1 unch. · avg -4.1%
destructuring.js — Interp: 🟢 6, 🔴 7, 9 unch. · avg +0.4% · Bytecode: 🔴 19, 3 unch. · avg -4.8%
fibonacci.js — Interp: 🟢 5, 3 unch. · avg +6.4% · Bytecode: 🟢 3, 🔴 3, 2 unch. · avg +0.2%
for-of.js — Interp: 🔴 1, 6 unch. · avg +0.2% · Bytecode: 🟢 3, 🔴 2, 2 unch. · avg +0.5%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 12, 8 unch. · avg +2.3% · Bytecode: 🔴 20 · avg -6.2%
json.js — Interp: 🟢 18, 2 unch. · avg +6.5% · Bytecode: 🟢 1, 🔴 16, 3 unch. · avg -2.7%
jsx.jsx — Interp: 🟢 7, 🔴 3, 11 unch. · avg +0.7% · Bytecode: 🔴 21 · avg -6.1%
modules.js — Interp: 🟢 9 · avg +6.8% · Bytecode: 🟢 5, 🔴 4 · avg -0.6%
numbers.js — Interp: 🟢 8, 3 unch. · avg +8.3% · Bytecode: 🟢 2, 🔴 6, 3 unch. · avg -2.2%
objects.js — Interp: 🟢 3, 🔴 3, 1 unch. · avg -0.2% · Bytecode: 🔴 7 · avg -6.5%
promises.js — Interp: 🟢 7, 5 unch. · avg +1.9% · Bytecode: 🔴 11, 1 unch. · avg -5.7%
regexp.js — Interp: 🟢 9, 2 unch. · avg +4.9% · Bytecode: 🟢 3, 🔴 4, 4 unch. · avg -1.0%
strings.js — Interp: 🟢 9, 🔴 1, 1 unch. · avg +5.4% · Bytecode: 🟢 1, 🔴 8, 2 unch. · avg -2.8%
typed-arrays.js — Interp: 🟢 7, 🔴 10, 5 unch. · avg +0.2% · Bytecode: 🔴 20, 2 unch. · avg -3.9%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/toolchain.yml:
- Around line 262-267: The case statement selecting the compiler wrapper (case
"$cpu" in ... ) currently handles only x86_64, i386, and aarch64 and lacks a
default branch; add an explicit unsupported-CPU guard by adding a default case
(*) that logs/echoes a clear error (including the offending $cpu value) and
exits non‑zero so builds fail fast when CPU is not one of the three supported
values—update the block that sets local wrapper and the case "$cpu" in construct
to include this hard-fail branch.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2fe4039-efcb-4ae9-990e-ac21be9ef19b
📒 Files selected for processing (1)
.github/workflows/toolchain.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/toolchain.yml:
- Around line 139-161: The Darwin as-fix shim created under PREFIX (the script
at $PREFIX/lib/as-fix/as) isn't added to PATH for downstream Darwin compilation
steps that invoke CROSS_FPC, so FPC can call /usr/bin/as directly and hit the
Xcode 16.4 CFI-label error; fix by ensuring the shim is always visible to
downstream jobs: either prepend $PREFIX/lib/as-fix to PATH in the CI steps that
run CROSS_FPC for Darwin targets (ci.yml steps that call $CROSS_FPC), or install
the shim into the toolchain wrapper (e.g., integrate invocation into the
generated fpc wrapper at $PREFIX/bin/fpc) so every invocation of CROSS_FPC uses
the shim.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c75cff85-df4f-4681-b55f-263362f35c08
📒 Files selected for processing (1)
.github/workflows/toolchain.yml
Add a default case to the CPU selection that fails fast with a clear error instead of silently falling through with unset variables. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
force-pushed
the
fix/cross-package-bootstrap
branch
from
bde5f60 to
bb30aa4
Compare
Owner
Author
SummaryReverted to the existing curated approach with one small improvement: an unsupported-CPU guard in the What was attemptedThe goal was to replace the curated per-package build with FPC 3.2.2 + Xcode 16.4 assembler incompatibility: FPC generates Multiple workarounds were attempted and all failed:
Why revert is the right callThe curated approach on
There are no missing packages today. The What this PR now containsJust the unsupported-CPU guard from CodeRabbit's review — a |
This was referenced Apr 9, 2026
frostney
changed the title
frostney
dismissed
coderabbitai[bot]’s stale review
Outdated and the scope has shifted
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
toolchain.yml(rtl-objpas, rtl-generics, fcl-process + source copies of fcl-base and regexpr) with FPC'smake packages, which builds all standard packages for each cross targetci.ymlto use a single-Fu"${UNITS_DIR}/*"wildcard instead of 6 separate-Fupaths per compilation commandv31to trigger a fresh toolchain buildMotivation
Fixes #170 — the curated package approach meant every new standard FPC dependency (like
Base64fromfcl-base) risked a CI failure requiring per-package CI edits. The new approach installs compiled units into the standardunits/$TARGET/*tree so any shipped FPC/FCL package is automatically available.What changed
toolchain.ymlbuild_target()reduced from ~80 lines of manual per-unit compilation tomake packages+ install loop (~15 lines). Removed source copies of fcl-base/regexpr.ci.ymlUNITS_DIR. Per-package-Fuchains → single-Fu"${UNITS_DIR}/*"wildcard.docs/build-system.mdTest plan
make packagesfor all 5 cross targets-Fupath🤖 Generated with Claude Code