ci: migrate primary CI jobs to ARM runners#7232
ci: migrate primary CI jobs to ARM runners#7232thepastaclaw wants to merge 1 commit intodashpay:developfrom
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
WalkthroughCI workflows and test environment scripts were updated to remove the aarch64 build path and related setup, shift many linux64 jobs to run on ARM runners, and modify native setup scripts to change or remove HOST detection. A new workflow step fetches a base-image manifest digest and exposes it as Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions Workflow
participant Check as check-skip job
participant Docker as Docker Hub
participant Caller as workflow caller
participant Depends as build-depends workflow
participant Cache as Actions Cache
participant Build as Build Job
GH->>Check: start check-skip
Check->>Docker: request manifest for base image
Docker-->>Check: return base-image-digest (or unknown)
Check-->>Caller: emit base-image-digest output
Caller->>Depends: invoke build-depends (base-image-digest, effective-runs-on)
Depends->>Cache: compute cache-key-prefix (includes base-image-digest)
Cache-->>Depends: restore using cache-key-prefix
Depends->>Build: pass cache info and inputs
Build->>Cache: save cache with cache-key-prefix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
your branch ci fails here: https://github.com/thepastaclaw/dash/actions/runs/23168985036/job/67317597479
please continue tracking this until push ci on your repo is successful
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
4b72d41 to
cba08c0
Compare
|
The CI failure on the old commit (
The fixes in the force-push:
|
|
✅ Review complete (commit 1f22662) |
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
d619b9f to
cba08c0
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The CI host-triplet cleanup is mostly correct, but it introduces one real regression in previous-release handling for ARM-native linux64 functional jobs. The only confirmed issue is blocking: the new ARM host mapping reaches a legacy release path that has no matching aarch64 artifact, so the job aborts before tests run.
Reviewed commit: cba08c0
🔴 1 blocking
1 additional finding
🔴 blocking: ARM native linux64 jobs fail when downloading the oldest previous-release binary
test/get_previous_releases.py (lines 127-134)
test-linux64 now runs on an ARM runner, 00_setup_env.sh sets HOST=aarch64-linux-gnu for that environment, and the native linux64 test target still enables --previous-releases. In check_host(), that host maps to args.platform = "aarch64-linux-gnu", and the default tag list includes v0.12.1.5. For tags earlier than v0.12.3, download_binary() only remaps arm-linux-gnueabihf to RPi2 and x86_64-linux-gnu to linux64, so v0.12.1.5 is requested as dashcore-0.12.1.5-aarch64-linux-gnu.tar.gz. That artifact is not present in SHA256_SUMS and does not exist in the release assets, so the script returns Binary tag was not found and the functional job fails before running tests.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/get_previous_releases.py`:
- [BLOCKING] lines 127-134: ARM native linux64 jobs fail when downloading the oldest previous-release binary
`test-linux64` now runs on an ARM runner, `00_setup_env.sh` sets `HOST=aarch64-linux-gnu` for that environment, and the native `linux64` test target still enables `--previous-releases`. In `check_host()`, that host maps to `args.platform = "aarch64-linux-gnu"`, and the default tag list includes `v0.12.1.5`. For tags earlier than `v0.12.3`, `download_binary()` only remaps `arm-linux-gnueabihf` to `RPi2` and `x86_64-linux-gnu` to `linux64`, so `v0.12.1.5` is requested as `dashcore-0.12.1.5-aarch64-linux-gnu.tar.gz`. That artifact is not present in `SHA256_SUMS` and does not exist in the release assets, so the script returns `Binary tag was not found` and the functional job fails before running tests.
cba08c0 to
a9c7f69
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The arm64 CI migration is mostly coherent, but one test path still assumes legacy x86_64 release binaries and will break the native linux64 functional job. I also confirmed a cache-key mismatch that will keep the arm64 previous-releases cache cold after the workflow switch, but that is a performance issue rather than a correctness blocker.
Reviewed commit: a9c7f69
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: The releases cache key no longer distinguishes x86_64 and arm64 contents
.github/workflows/test-src.yml (lines 46-52)
PREVIOUS_RELEASES_DIR now includes $HOST, so the migrated linux64 job writes downloaded releases under releases/aarch64-linux-gnu instead of the old x86_64 directory. The cache key here still depends only on file hashes, so an existing cache entry created by the former x86_64 linux64 job can be restored on the new arm64 job under the same key. Because that restore is an exact key hit, the cache action will not publish the newly downloaded arm64 subtree back under a distinct key, which leaves the arm64 job redownloading previous releases on subsequent runs.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/get_previous_releases.py`:
- [BLOCKING] lines 127-134: Native arm64 `linux64` jobs still fetch the x86_64 v0.12.1.5 tarball
The new `linux64` workflow now runs on arm64 runners and `ci/test/00_setup_env.sh` derives `HOST=aarch64-linux-gnu` on those machines, while the Qt5 functional job still enables `--previous-releases`. In `download_binary()`, tags older than `v0.12.3` still remap both `x86_64-linux-gnu` and `aarch64-linux-gnu` to the legacy `linux64` tarball, which is the x86_64 build. `feature_unsupported_utxo_db.py` explicitly starts `v0.12.1.5`, and the functional harness launches that binary directly from `PREVIOUS_RELEASES_DIR` without any emulator path. On the new native arm64 runner this turns into an exec-format failure, so the `linux64` functional job will fail until old-release handling is made architecture-aware or the test is excluded there.
In `.github/workflows/test-src.yml`:
- [SUGGESTION] lines 46-52: The releases cache key no longer distinguishes x86_64 and arm64 contents
`PREVIOUS_RELEASES_DIR` now includes `$HOST`, so the migrated `linux64` job writes downloaded releases under `releases/aarch64-linux-gnu` instead of the old x86_64 directory. The cache key here still depends only on file hashes, so an existing cache entry created by the former x86_64 `linux64` job can be restored on the new arm64 job under the same key. Because that restore is an exact key hit, the cache action will not publish the newly downloaded arm64 subtree back under a distinct key, which leaves the arm64 job redownloading previous releases on subsequent runs.
| elif platform in ["x86_64-apple-darwin", "arm64-apple-darwin"]: | ||
| print(f"Binaries not available for {tag} on {platform}") | ||
| return 1 | ||
| elif platform in ["x86_64-linux-gnu"]: | ||
| elif platform in ["x86_64-linux-gnu", "aarch64-linux-gnu"]: | ||
| platform = "linux64" |
There was a problem hiding this comment.
🔴 Blocking: Native arm64 linux64 jobs still fetch the x86_64 v0.12.1.5 tarball
The new linux64 workflow now runs on arm64 runners and ci/test/00_setup_env.sh derives HOST=aarch64-linux-gnu on those machines, while the Qt5 functional job still enables --previous-releases. In download_binary(), tags older than v0.12.3 still remap both x86_64-linux-gnu and aarch64-linux-gnu to the legacy linux64 tarball, which is the x86_64 build. feature_unsupported_utxo_db.py explicitly starts v0.12.1.5, and the functional harness launches that binary directly from PREVIOUS_RELEASES_DIR without any emulator path. On the new native arm64 runner this turns into an exec-format failure, so the linux64 functional job will fail until old-release handling is made architecture-aware or the test is excluded there.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/get_previous_releases.py`:
- [BLOCKING] lines 127-134: Native arm64 `linux64` jobs still fetch the x86_64 v0.12.1.5 tarball
The new `linux64` workflow now runs on arm64 runners and `ci/test/00_setup_env.sh` derives `HOST=aarch64-linux-gnu` on those machines, while the Qt5 functional job still enables `--previous-releases`. In `download_binary()`, tags older than `v0.12.3` still remap both `x86_64-linux-gnu` and `aarch64-linux-gnu` to the legacy `linux64` tarball, which is the x86_64 build. `feature_unsupported_utxo_db.py` explicitly starts `v0.12.1.5`, and the functional harness launches that binary directly from `PREVIOUS_RELEASES_DIR` without any emulator path. On the new native arm64 runner this turns into an exec-format failure, so the `linux64` functional job will fail until old-release handling is made architecture-aware or the test is excluded there.
a9c7f69 to
3841458
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The ARM migration is mostly consistent, but it introduces a real previous-release functional test failure on the native linux64 job.
Reviewed commit: 3841458
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/get_previous_releases.py`:
- [BLOCKING] lines 133-134: ARM linux64 job now downloads an x86_64-only v0.12.1.5 binary for previous-release tests
Validated against the checked-out CI wiring: the migrated `linux64` job now runs on ARM runners, `00_setup_env.sh` sets `HOST=aarch64-linux-gnu` there, and `00_setup_env_native_qt5.sh` still enables `--previous-releases`. The linux64 functional test list still includes `feature_unsupported_utxo_db.py`, which explicitly starts version `120105` (v0.12.1.5). For tags `< v0.12.3`, this code maps `aarch64-linux-gnu` to `linux64`, which resolves to `dashcore-0.12.1.5-linux64.tar.gz` from `SHA256_SUMS` and that artifact is the legacy x86_64 tarball. Unlike the removed cross-build path, the new native ARM job does not configure qemu to run x86_64 binaries, so this test path will fail when it tries to launch the downloaded release.
| elif platform in ["x86_64-linux-gnu", "aarch64-linux-gnu"]: | ||
| platform = "linux64" |
There was a problem hiding this comment.
🔴 Blocking: ARM linux64 job now downloads an x86_64-only v0.12.1.5 binary for previous-release tests
Validated against the checked-out CI wiring: the migrated linux64 job now runs on ARM runners, 00_setup_env.sh sets HOST=aarch64-linux-gnu there, and 00_setup_env_native_qt5.sh still enables --previous-releases. The linux64 functional test list still includes feature_unsupported_utxo_db.py, which explicitly starts version 120105 (v0.12.1.5). For tags < v0.12.3, this code maps aarch64-linux-gnu to linux64, which resolves to dashcore-0.12.1.5-linux64.tar.gz from SHA256_SUMS and that artifact is the legacy x86_64 tarball. Unlike the removed cross-build path, the new native ARM job does not configure qemu to run x86_64 binaries, so this test path will fail when it tries to launch the downloaded release.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/get_previous_releases.py`:
- [BLOCKING] lines 133-134: ARM linux64 job now downloads an x86_64-only v0.12.1.5 binary for previous-release tests
Validated against the checked-out CI wiring: the migrated `linux64` job now runs on ARM runners, `00_setup_env.sh` sets `HOST=aarch64-linux-gnu` there, and `00_setup_env_native_qt5.sh` still enables `--previous-releases`. The linux64 functional test list still includes `feature_unsupported_utxo_db.py`, which explicitly starts version `120105` (v0.12.1.5). For tags `< v0.12.3`, this code maps `aarch64-linux-gnu` to `linux64`, which resolves to `dashcore-0.12.1.5-linux64.tar.gz` from `SHA256_SUMS` and that artifact is the legacy x86_64 tarball. Unlike the removed cross-build path, the new native ARM job does not configure qemu to run x86_64 binaries, so this test path will fail when it tries to launch the downloaded release.
18e3dce to
49bf536
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The current HEAD still has one real CI regression in the native ARM functional-test path. The new ARM-specific exclusion is injected through TEST_RUNNER_EXTRA, but the workflow passes a second --exclude later on the command line, so test_runner.py drops feature_unsupported_utxo_db from the final exclusion set.
Reviewed commit: 49bf536
🔴 1 blocking
1 additional finding
🔴 blocking: ARM-specific functional-test exclusion is still overridden by workflow arguments
ci/dash/test_integrationtests.sh (line 47)
On native ARM, ci/test/00_setup_env_native_qt5.sh now exports TEST_RUNNER_EXTRA with --exclude feature_pruning,feature_dbcrash,feature_unsupported_utxo_db, because the previous-release binary required by feature_unsupported_utxo_db is unavailable on aarch64. This script still expands ${TEST_RUNNER_EXTRA} before $PASS_ARGS, and test/functional/test_runner.py defines --exclude as a single-value option, so the later workflow-provided --exclude feature_pruning,feature_dbcrash replaces the ARM-specific list instead of extending it. The linux64 ARM job will therefore continue to schedule feature_unsupported_utxo_db and fail in the exact scenario the setup change was meant to avoid.
💡 Suggested change
LD_LIBRARY_PATH="$DEPENDS_DIR/$HOST/lib" ./test/functional/test_runner.py --ci --attempts=3 --ansi --combinedlogslen=4000 --timeout-factor="${TEST_RUNNER_TIMEOUT_FACTOR}" $PASS_ARGS ${TEST_RUNNER_EXTRA} --failfast --nocleanup --tmpdir="$(pwd)/testdatadirs" $EXTRA_ARGS
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `ci/dash/test_integrationtests.sh`:
- [BLOCKING] line 47: ARM-specific functional-test exclusion is still overridden by workflow arguments
On native ARM, `ci/test/00_setup_env_native_qt5.sh` now exports `TEST_RUNNER_EXTRA` with `--exclude feature_pruning,feature_dbcrash,feature_unsupported_utxo_db`, because the previous-release binary required by `feature_unsupported_utxo_db` is unavailable on aarch64. This script still expands `${TEST_RUNNER_EXTRA}` before `$PASS_ARGS`, and `test/functional/test_runner.py` defines `--exclude` as a single-value option, so the later workflow-provided `--exclude feature_pruning,feature_dbcrash` replaces the ARM-specific list instead of extending it. The linux64 ARM job will therefore continue to schedule `feature_unsupported_utxo_db` and fail in the exact scenario the setup change was meant to avoid.
e7f1160 to
7b0e40c
Compare
|
This pull request has conflicts, please rebase. |
|
@coderabbitai review |
|
This pull request has conflicts, please rebase. |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated the current HEAD at e7f1160 against the available rerun artifacts and the workflow/script changes in the working tree. The agent findings input was empty, there were no available CodeRabbit findings to react to in .review-e7f11606, and I did not confirm any actionable correctness issue in the current patch.
Reviewed commit: e7f1160
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
7b0e40c to
e7f1160
Compare
229d466 ci: use cheaper ARM bootstrap runner for check-skip (PastaClaw) Pull request description: # Summary Move only the `check-skip` bootstrap job onto a cheaper ARM runner path. ## Problem `check-skip` currently uses the generic amd64 runner setting, so even the lightweight bootstrap step can consume the more expensive primary runner pool. ## Solution - add a dedicated `RUNNER_CHECK_SKIP` override for the `check-skip` job - default that job to GitHub-hosted ARM (`ubuntu-24.04-arm`) - leave downstream dynamic runner selection unchanged for the heavier CI jobs This keeps the tiny bootstrap step cheap by default while still allowing an org-level Blacksmith ARM label, such as a small 1 vCPU runner, to be configured later without affecting the main build/test jobs. ## Related - split out from <#7232> ## Validation - parsed `.github/workflows/build.yml` successfully with PyYAML after the change - reviewed the workflow context around `check-skip`, `select_dynamic_runner.py`, and `cache-depends-sources.yml` - ran the mandatory code-review gate on `upstream/develop..ci/check-skip-arm-bootstrap` → *ship* Top commit has no ACKs. Tree-SHA512: a0163facade75a60bc69f668225b069660a8cb3faa12da4b9d43fbbd248b7291a0db521873f46ba98f37929031d09d1571f3c020bcaade5bce6c1fadc4614b70
|
CI on pastaclaw fork looks good: https://github.com/thepastaclaw/dash/actions/runs/24152406811 |
| # On native ARM (aarch64), v0.12.1.5 has no aarch64 binary; exclude the test | ||
| # that requires it and limit previous release downloads to available versions | ||
| if [ "$(uname -m)" = "aarch64" ]; then | ||
| export TEST_RUNNER_EXTRA="--previous-releases --coverage --extended --exclude feature_pruning,feature_dbcrash,feature_unsupported_utxo_db" |
There was a problem hiding this comment.
@thepastaclaw why did you add feature_unsupported_utxo_db to what not to run?
.github/workflows/build.yml
Outdated
| uses: ./.github/workflows/build-depends.yml | ||
| needs: [check-skip, container, cache-sources] | ||
| if: ${{ vars.SKIP_ARM_LINUX == '' }} | ||
| if: ${{ false && vars.SKIP_ARM_LINUX == '' }} |
There was a problem hiding this comment.
so just disabled? we should remove in that case
.github/workflows/build.yml
Outdated
|
|
||
| src-aarch64-linux: | ||
| name: aarch64-linux-build | ||
| if: ${{ false }} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
114-124: Remove the hard-disabled aarch64 jobs instead of keepingif: falsestubs.These jobs are dead code now, and
actionlintis already flagging the constant-false conditions. Since this PR removes the cross-compiled aarch64 path, deleting the inert nodes will keep the workflow DAG cleaner and avoid ongoing lint noise.Also applies to: 195-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 114 - 124, Remove the dead aarch64 job stubs that use a constant-false condition—specifically delete the depends-aarch64-linux job block (and the analogous job block at lines referenced 195-206) that include "if: ${{ false && vars.SKIP_ARM_LINUX == '' }}", its uses: ./.github/workflows/build-depends.yml stanza, with: build-target: aarch64-linux, container-path, base-image-digest and runs-on entries; also remove any remaining workflow job nodes or job references that only exist to satisfy that disabled aarch64 path so the DAG no longer contains inert jobs and actionlint warnings for constant-false conditions are eliminated.
🤖 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/build-depends.yml:
- Around line 74-76: CACHE_KEY_PREFIX is currently keyed with inputs.runs-on
which can differ from the actual runner used (inputs.effective-runs-on), causing
restores to pull artifacts from the wrong runner; update the construction of
CACHE_KEY_PREFIX to use inputs.effective-runs-on if present, falling back to
inputs.runs-on (i.e., use the expression inputs.effective-runs-on ||
inputs.runs-on) so the depends cache is namespaced by the actual executing
runner; update the reference where CACHE_KEY_PREFIX is set and ensure any
related echo to GITHUB_OUTPUT continues to use the updated CACHE_KEY_PREFIX
value.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 114-124: Remove the dead aarch64 job stubs that use a
constant-false condition—specifically delete the depends-aarch64-linux job block
(and the analogous job block at lines referenced 195-206) that include "if: ${{
false && vars.SKIP_ARM_LINUX == '' }}", its uses:
./.github/workflows/build-depends.yml stanza, with: build-target: aarch64-linux,
container-path, base-image-digest and runs-on entries; also remove any remaining
workflow job nodes or job references that only exist to satisfy that disabled
aarch64 path so the DAG no longer contains inert jobs and actionlint warnings
for constant-false conditions are eliminated.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1a101c6-8bbc-45f7-ad15-f2cd70c7e4ac
📒 Files selected for processing (12)
.github/workflows/build-depends.yml.github/workflows/build.yml.github/workflows/test-src.ymlci/dash/matrix.shci/dash/test_integrationtests.shci/test/00_setup_env.shci/test/00_setup_env_aarch64.shci/test/00_setup_env_native_multiprocess.shci/test/00_setup_env_native_qt5.shci/test/00_setup_env_native_tsan.shci/test/05_before_script.shtest/get_previous_releases.py
💤 Files with no reviewable changes (3)
- ci/test/00_setup_env_native_multiprocess.sh
- ci/test/00_setup_env_aarch64.sh
- ci/test/00_setup_env_native_tsan.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- test/get_previous_releases.py
- ci/dash/matrix.sh
Move depends-linux64 and its consumers (linux64, fuzz, sqlite, ubsan) to ARM runners, making aarch64 the primary CI architecture. This includes both build and test stages. Drop the aarch64-linux cross-compile job (depends + src) and its setup env script since we don't ship arm-linux-gnueabihf binaries and now have native aarch64 coverage through the migrated jobs. Jobs remaining on x86: - nowallet (build + test): sole x86_64 native coverage - mac, win64: cross-compile targets (x86 host) - lint: architecture-independent Update 00_setup_env_native_qt5.sh to dynamically detect HOST based on the runner architecture instead of hardcoding x86_64.
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
e7f1160 to
1f22662
Compare
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/get_previous_releases.py (1)
133-134:⚠️ Potential issue | 🟠 MajorARM jobs will fail when downloading v0.12.1.5 binaries for previous-release tests.
This mapping causes
aarch64-linux-gnuto resolve to thelinux64tarball (dashcore-0.12.1.5-linux64.tar.gz), which contains x86_64 binaries. On native ARM runners without QEMU emulation, executing these binaries will fail with exec-format errors.The
feature_unsupported_utxo_db.pytest explicitly starts version120105(v0.12.1.5). Either:
- Exclude tests requiring v0.12.1.5 on ARM via
PREVIOUS_RELEASES_TAGS, or- Skip the previous-releases tests entirely on ARM for pre-v0.12.3 versions
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/get_previous_releases.py` around lines 133 - 134, The current platform mapping in get_previous_releases.py maps "aarch64-linux-gnu" to "linux64", causing ARM runners to download x86_64 tarballs (e.g., v0.12.1.5) and fail; update the logic around the platform variable (and the PREVIOUS_RELEASES_TAGS usage) to detect "aarch64-linux-gnu" and skip/omit any previous-release tags older than 0.12.3 (numeric tag < 120203) so tests like feature_unsupported_utxo_db.py which start version 120105 are excluded on ARM, or alternatively map aarch64 to an appropriate ARM tarball name if such a tarball exists for those releases—implement the skip by adding a guard after platform is determined that filters PREVIOUS_RELEASES_TAGS (or returns early) when platform == "aarch64-linux-gnu" and tag < 120203.
🤖 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/build-depends.yml:
- Around line 141-143: The command substitution $(nproc) in the workflow run
step can undergo word splitting; update the run block so the job-parallelism
argument is quoted—replace make -j$(nproc) -C depends with make -j"$(nproc)" -C
depends (i.e., quote "$(nproc)") to prevent word splitting and satisfy static
analysis.
---
Duplicate comments:
In `@test/get_previous_releases.py`:
- Around line 133-134: The current platform mapping in get_previous_releases.py
maps "aarch64-linux-gnu" to "linux64", causing ARM runners to download x86_64
tarballs (e.g., v0.12.1.5) and fail; update the logic around the platform
variable (and the PREVIOUS_RELEASES_TAGS usage) to detect "aarch64-linux-gnu"
and skip/omit any previous-release tags older than 0.12.3 (numeric tag < 120203)
so tests like feature_unsupported_utxo_db.py which start version 120105 are
excluded on ARM, or alternatively map aarch64 to an appropriate ARM tarball name
if such a tarball exists for those releases—implement the skip by adding a guard
after platform is determined that filters PREVIOUS_RELEASES_TAGS (or returns
early) when platform == "aarch64-linux-gnu" and tag < 120203.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 073b3f23-78d3-400c-afcd-c99420b9ead5
📒 Files selected for processing (12)
.github/workflows/build-depends.yml.github/workflows/build.yml.github/workflows/test-src.ymlci/dash/matrix.shci/dash/test_integrationtests.shci/test/00_setup_env.shci/test/00_setup_env_aarch64.shci/test/00_setup_env_native_multiprocess.shci/test/00_setup_env_native_qt5.shci/test/00_setup_env_native_tsan.shci/test/05_before_script.shtest/get_previous_releases.py
💤 Files with no reviewable changes (3)
- ci/test/00_setup_env_native_multiprocess.sh
- ci/test/00_setup_env_native_tsan.sh
- ci/test/00_setup_env_aarch64.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/test-src.yml
- ci/dash/matrix.sh
- ci/test/00_setup_env_native_qt5.sh
| run: | | ||
| export HOST="${{ needs.check-cache.outputs.host }}" | ||
| env ${{ needs.check-cache.outputs.dep-opts }} make -j$(nproc) -C depends |
There was a problem hiding this comment.
Quote $(nproc) to prevent word splitting.
Static analysis flagged this unquoted command substitution.
Proposed fix
- name: Build depends
run: |
export HOST="${{ needs.check-cache.outputs.host }}"
- env ${{ needs.check-cache.outputs.dep-opts }} make -j$(nproc) -C depends
+ env ${{ needs.check-cache.outputs.dep-opts }} make -j"$(nproc)" -C depends🧰 Tools
🪛 actionlint (1.7.12)
[error] 141-141: shellcheck reported issue in this script: SC2046:warning:2:54: Quote this to prevent word splitting
(shellcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-depends.yml around lines 141 - 143, The command
substitution $(nproc) in the workflow run step can undergo word splitting;
update the run block so the job-parallelism argument is quoted—replace make
-j$(nproc) -C depends with make -j"$(nproc)" -C depends (i.e., quote "$(nproc)")
to prevent word splitting and satisfy static analysis.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I re-reviewed the incremental delta from e7f11606a13a9ba99129e01bc4efbc5732ead154 to 1f226623c5ebbddd6d6838fe6e7fe939f7a6bdf5. The current changes are clean: the depends cache key now uses the effective runner label that actually executes the reusable workflow job, which fixes the remaining architecture-mismatch cache issue for native linux64 ARM runs, and the removed aarch64-linux workflow blocks were already permanently disabled and are no longer referenced by any active jobs. I did not find a new correctness or coverage regression in the incremental delta.
Reviewed commit: 1f22662
2 participants
Summary
Migrate the primary CI path from x86 to ARM runners, reducing costs and providing native aarch64 build+test coverage.
Changes
Moved to ARM runners (
runner-arm64)depends-linux64(shared depends for linux64, fuzz, sqlite, ubsan)src-linux64+test-linux64(primary native build+test)src-linux64_fuzz(fuzz build)src-linux64_sqlite+test-linux64_sqlite(SQLite build+test)src-linux64_ubsan+test-linux64_ubsan(UBSan build+test)Removed
depends-aarch64-linux+src-aarch64-linux— the cross-compile job foraarch64-linux-gnufrom x86 runners. We don't shiparm-linux-gnueabihfbinaries, and native aarch64 coverage is now provided by the migrated jobs.ci/test/00_setup_env_aarch64.sh— the cross-compile setup scriptRemaining on x86 (
runner-amd64)nowallet(build + test) — sole native x86_64 coveragemac,win64— cross-compile targetslint— architecture-independentHOST detection centralization
Moved architecture-based HOST detection (
aarch64-linux-gnu/x86_64-pc-linux-gnu) from individual setup scripts intoci/test/00_setup_env.sh. This:aarch64-linux-gnuinstead ofconfig.guess'saarch64-unknown-linux-gnu)00_setup_env_native_qt5.sh,00_setup_env_native_multiprocess.sh,00_setup_env_native_tsan.shPREVIOUS_RELEASES_DIRpath computation which depended on HOST being set before the target script ranPrevious releases download fix
Added exact-match patterns for
aarch64-linux-gnuandx86_64-linux-gnuintest/get_previous_releases.py, since the existing fnmatch globaarch64-*-linux*requires a vendor field (e.g.aarch64-unknown-linux-gnu) and won't match the vendorless triplet.Validation
runner-arm64in build.yml00_setup_env.shcovers aarch64/x86_64 with dpkg fallbackaarch64-linux-gnuin SHA256_SUMSdepends-aarch64-linuxorsrc-aarch64-linuxremain in build.yml