-
Notifications
You must be signed in to change notification settings - Fork 13k
chore(ci): change build and test to use ARM by default #37404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThis PR shifts CI infrastructure to ARM64 architecture by changing the build-docker action default from amd64 to arm64, updating all CI workflow runners from ubuntu-24.04 to ubuntu-24.04-arm, removing platform constraints from docker-compose-ci.yml services, and adding defensive environment variable defaults throughout the compose configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
6ad4850 to
a95fb3b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37404 +/- ##
===========================================
- Coverage 68.98% 68.97% -0.01%
===========================================
Files 3357 3357
Lines 114243 114243
Branches 20531 20531
===========================================
- Hits 78813 78803 -10
- Misses 33345 33351 +6
- Partials 2085 2089 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
fba8dec to
4cf2b04
Compare
b2dcf01 to
85706a2
Compare
482fa73 to
a5f1336
Compare
8e80445 to
55ff3dd
Compare
a5f1336 to
f9d6b48
Compare
55ff3dd to
4ba7db6
Compare
f9d6b48 to
c2076af
Compare
e8d09af to
6c71b8e
Compare
3f28d9b to
d892cd2
Compare
There was a problem hiding this 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.github/actions/build-docker/action.yml(1 hunks).github/workflows/ci-code-check.yml(1 hunks).github/workflows/ci-deploy-gh-pages.yml(1 hunks).github/workflows/ci-test-storybook.yml(1 hunks).github/workflows/ci-test-unit.yml(1 hunks).github/workflows/ci.yml(9 hunks)docker-compose-ci.yml(2 hunks)packages/jest-presets/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (9)
.github/workflows/ci-test-storybook.yml (1)
22-22: Runner migration to ARM is consistent.The runner update aligns with the broader CI infrastructure shift to ubuntu-24.04-arm. No functional changes to the workflow logic.
.github/workflows/ci-code-check.yml (1)
18-18: Runner migration to ARM is consistent.The runner update aligns with the PR's infrastructure shift. The swap space configuration at line 29–31 is architecture-agnostic.
.github/workflows/ci-test-unit.yml (1)
26-26: Runner migration to ARM is consistent.The runner update aligns with the broader CI infrastructure shift. Test logic remains unchanged.
.github/actions/build-docker/action.yml (1)
15-15: Default arch changed to ARM64.The architecture default is now
arm64instead ofamd64. Since ci.yml'sbuild-gh-dockerjob explicitly specifies thearchparameter in the matrix (line 324, 337, 351, 366), this change is safe. Verify that any other callers of this action also explicitly specify the architecture..github/workflows/ci-deploy-gh-pages.yml (1)
12-12: Runner migration to ARM is consistent.The runner update aligns with the PR's infrastructure shift. Static asset deployment logic is platform-agnostic.
.github/workflows/ci.yml (2)
272-272: Conditional runner selection based on architecture is correct.Line 272's logic (
ubuntu-24.04${{ matrix.arch == 'arm64' && '-arm' || '' }}) correctly selects the runner based on the target architecture in the matrix. Combined with the multi-arch matrix at line 281 (arm64 and amd64), this ensures:
- ARM builds run on ubuntu-24.04-arm
- AMD64 builds run on ubuntu-24.04
This preserves cross-platform build capability.
26-26: Runner migration to ARM is consistent across all jobs.All primary workflow jobs are migrated to ubuntu-24.04-arm. The report-coverage job at line 590 correctly remains on ubuntu-24.04 (amd64) since it only aggregates coverage reports and doesn't need ARM architecture.
The multi-architecture build matrix is preserved via conditional runner selection at line 272.
Also applies to: 124-124, 167-167, 241-241, 373-373, 642-642, 680-680, 734-734, 850-850
docker-compose-ci.yml (2)
6-6: Defensive environment variable defaults improve robustness.The addition of
${VAR:-}defaults throughout the compose configuration prevents container initialization failures when environment variables are not set. This is particularly important for:
DENO_VERSION: empty default is safe (build arg)DEBUG: empty default disables debug loggingROCKETCHAT_LICENSE: empty default runs without enterprise featuresCOVERAGE_*: empty defaults safely disable coverage instrumentationDISABLE_DB_WATCHERS: empty default is treated as falseThis aligns with the goal of making the ARM-based CI more resilient.
Also applies to: 13-13, 17-17, 23-27
8-11: Multi-architecture buildx platform specifications are preserved.The
x-bakeplatform definitions (lines 8–11, 52–55, etc.) correctly specify bothlinux/amd64andlinux/arm64. These are buildx build platform specifications that determine what the build system produces, not runtime platform constraints. Retaining both architectures is correct for supporting multi-arch deployments from the CI system.Also applies to: 52-55, 72-75, 92-95, 112-115, 138-141, 159-162, 179-182
| ], | ||
| "dependencies": { | ||
| "@swc/core": "~1.13.20", | ||
| "@swc/core": "1.13.19", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify the @swc/core version downgrade.
The version was changed from ~1.13.20 to 1.13.19, which:
- Downgrades to an older patch version
- Removes the tilde (
~), enforcing strict pinning instead of allowing minor version flexibility
This appears unusual. Clarify whether this downgrade is intentional (e.g., to work around an ARM64 compatibility issue) or unintended.
🌐 Web query:
Does @swc/core version 1.13.20 have known issues with ARM64 architecture?
💡 Result:
Short answer: Yes — v1.13.20 had known ARM64 (and Alpine/musl) binary-loading crashes/segfaults; a fix was merged and released in 1.13.21 that improves ARM64 and Alpine (musl) binary loading/validation. [1][2]
Sources:
- PR/commit summary describing ARM64 + musl segfaults and the fix (binary loading/validation + workaround). [1]
- @swc/core changelog showing the 1.13.21 bugfix: "Improve ARM64 and Alpine Linux (musl) binary loading and validation". [2]
Recommendation: upgrade to 1.13.21 (or later).
Upgrade @swc/core to 1.13.21 to fix ARM64 issues.
Version 1.13.20 has known ARM64 (and Alpine/musl) binary-loading crashes/segfaults, with a fix released in 1.13.21. Downgrading to 1.13.19 avoids the bug but leaves an older version in place. Instead, upgrade to 1.13.21 or later where the ARM64 issue is resolved. Additionally, consider using a flexible version constraint (e.g., ^1.13.21) instead of strict pinning to allow patch updates.
🤖 Prompt for AI Agents
In packages/jest-presets/package.json around line 30, the pinned dependency
"@swc/core": "1.13.19" reintroduces an older version that avoids the ARM64 bug
but doesn't include the upstream fix; update the dependency to at least
"1.13.21" (recommended "^1.13.21" to allow patch updates) so the
ARM64/Alpine/musl binary-loading crash is resolved and future patches are picked
up; modify the version string accordingly and run yarn/npm install and tests to
ensure no regressions.
Depends on #37467
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit