CI job independence#59
Conversation
- Add fail-fast: false to all matrix strategies so one platform failure doesn't cancel other platforms - Add if: !cancelled() to test, wasm-test, benchmark, and examples jobs so they run even if build had partial matrix failures - Update artifacts and release jobs to explicitly check all upstream results via needs.*.result == 'success' - Add if: !cancelled() to PR workflow downstream jobs (test, wasm-test, benchmark) for the same independence - benchmark-comment requires benchmark success explicitly Co-authored-by: Johannes Stein <frostney@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCI and PR GitHub Actions matrices were adjusted to disable fail-fast and reorganize Windows matrix entries from x64 as a primary OS to explicit x86 includes; packaging and artifact handling were updated accordingly to reference Windows x86 and skip missing x64 artifacts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
Benchmark Results254 benchmarks (no baseline) arraybuffer.js — 14 benchmarks
arrays.js — 19 benchmarks
async-await.js — 6 benchmarks
classes.js — 31 benchmarks
closures.js — 11 benchmarks
collections.js — 12 benchmarks
destructuring.js — 22 benchmarks
fibonacci.js — 8 benchmarks
for-of.js — 7 benchmarks
iterators.js — 20 benchmarks
json.js — 20 benchmarks
jsx.jsx — 21 benchmarks
numbers.js — 11 benchmarks
objects.js — 7 benchmarks
promises.js — 12 benchmarks
strings.js — 11 benchmarks
typed-arrays.js — 22 benchmarks
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Revert the overly-aggressive !cancelled() conditions added in the previous commit. The build job must fully succeed before any downstream job runs. The test/wasm-test/benchmark/examples jobs were already independent of each other (they only depend on build, not on each other). The fail-fast: false on matrix strategies is the key change that ensures one platform failure within a job doesn't cancel siblings. Co-authored-by: Johannes Stein <frostney@users.noreply.github.com>
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/ci.yml:
- Around line 315-317: The CI configuration lists platforms in the "Package
releases" job using "windows-latest-x86" which contradicts docs and
FPC_TARGET_CPU usage expecting x64; decide whether Windows should be x86 or x64
and make the change consistently: update the platforms array in the "Package
releases" job to use "windows-latest-x64" if you want 64-bit builds (or update
docs/docs/build-system.md and references to FPC_TARGET_CPU to x86 if you intend
32-bit), and ensure the platform string and any mentions of FPC_TARGET_CPU and
the documented matrix all match the chosen architecture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60359896-ffc7-4be2-a528-620892eeff17
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/pr.yml
0352055 to
1ee1067
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
273-286: Consider addingfail-fast: falsefor consistency.The
artifactsjob matrix doesn't includefail-fast: falsewhile the build, test, benchmark, and examples jobs all do. If one platform's artifact upload fails, it would stop other platforms from completing. This may be intentional if you want all-or-nothing artifact uploads, but for consistency with the PR objective, consider addingfail-fast: falsehere as well.♻️ Suggested change
artifacts: needs: [test, wasm-test, benchmark, examples] if: github.ref == 'refs/heads/main' strategy: + fail-fast: false matrix: os: [ubuntu-latest, macos-latest, windows-latest]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 273 - 286, Add fail-fast: false to the artifacts job's matrix strategy so matrix failures for one platform don't cancel other platform runs; locate the artifacts job's strategy.matrix block (the matrix with os/arch/exclude/include shown) and insert fail-fast: false at the same level as matrix (matching how other jobs set fail-fast) to ensure each platform runs to completion even if one fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 273-286: Add fail-fast: false to the artifacts job's matrix
strategy so matrix failures for one platform don't cancel other platform runs;
locate the artifacts job's strategy.matrix block (the matrix with
os/arch/exclude/include shown) and insert fail-fast: false at the same level as
matrix (matching how other jobs set fail-fast) to ensure each platform runs to
completion even if one fails.
The Windows build runs on windows-latest (x64 runner) but FPC's i386-win32 toolchain produces 32-bit binaries. Fix artifact and release package names to say x86 instead of x64 using a conditional expression. The matrix structure and runners are unchanged. Update docs/build-system.md and AGENTS.md to reflect the x86 label, document fail-fast: false, job independence, and the wasm-test job. Co-authored-by: Johannes Stein <frostney@users.noreply.github.com>
1ee1067 to
6a4be74
Compare
Build must succeed on all platforms before downstream jobs run. Keeping the default fail-fast: true so a build failure cancels sibling builds early rather than wasting runner minutes. Co-authored-by: Johannes Stein <frostney@users.noreply.github.com>
The wasm-test job now runs on all platforms (ubuntu x64/arm64, macOS x64/arm64, windows x86) matching the test/benchmark/examples jobs. FPC and Node.js are installed per-OS. Co-authored-by: Johannes Stein <frostney@users.noreply.github.com>
Update CI workflows to ensure all jobs and matrix runs complete even if some fail.
Summary by CodeRabbit