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.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCI workflows and test environment scripts were changed to remove the aarch64 cross-build path and delete its setup script, shift many linux64 jobs to run on ARM runners, and adjust several native setup scripts to change or remove HOST detection logic. The reusable build-depends workflow gained an optional Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Check as check-skip job
participant Registry as Container Registry
participant Caller as workflow caller
participant Depends as build-depends workflow
participant Cache as Actions Cache
participant Build as Build job
GH->>Check: run check-skip
Check->>Registry: query base image manifest (optional)
Registry-->>Check: return manifest digest (optional)
Check-->>Caller: emit outputs (including runner-arm64 decision)
Caller->>Depends: call build-depends (runs-on, effective-runs-on)
Depends->>Cache: compute/restore cache using effective-runs-on
Cache-->>Depends: cache restore result
Depends->>Build: start build with restored cache
Build->>Cache: save cache with computed key
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)
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 7cd3bee) |
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
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
2467289 to
ea00c4b
Compare
|
pls consider 7529644 |
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
ea00c4b to
6cebe46
Compare
|
Considered and folded in UdjinM6s suggested follow-up. The branch now includes the x86 canary / x86 UBSan split plus the linux64_nowallet ARM move in 6cebe46. |
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
2 times, most recently
from
45e8401 to
0b362e3
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b362e37eb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
0b362e3 to
ff5ff35
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
CI-only PR migrating primary build+test jobs to native ARM runners. The prior-review blocker around previous-release handling on ARM is fixed on this SHA: get_previous_releases.py has explicit aarch64 tarball handling, PREVIOUS_RELEASES_TAGS excludes the unsupported v0.12.1.5 path on arm64, cache keys are partitioned by runner.arch, and check_host has an exact-match fallback for the vendorless aarch64-linux-gnu triplet. Remaining agent observations are low-signal CI-structure nits or intentional tuning, not actionable. Approving.
Reviewed commit: ea00c4b
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR migrates primary Linux CI from x86 to ARM runners, adds an x86 canary lane, and centralizes HOST detection. All mechanics verified against code. The most important issue: the PR description says multiprocess stayed on ARM, but the workflow actually moves both build and test for multiprocess to amd64 — this silently drops ARM coverage of the multiprocess/v2transport configuration and should either be reverted or explicitly documented. Other findings are minor (arg-order swap with behavioral implications, dead runs-on input, low-priority readability/inheritance nits).
Reviewed commit: ff5ff35
🟡 2 suggestion(s) | 💬 5 nitpick(s)
🤖 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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 232-351: multiprocess lane moved from ARM to x86 — contradicts PR description and drops ARM coverage
`src-linux64_multiprocess` (build.yml:232-243) now depends on the new x86-only `depends-linux64_multiprocess-x86` (build.yml:154-163) and runs on `runner-amd64`, and `test-linux64_multiprocess` (build.yml:343-351) likewise runs on `runner-amd64`. Only the TSAN lane still consumes the ARM-side `depends-linux64_multiprocess`. This contradicts the PR description's claim that "multiprocess + tsan were already on ARM — no change" and removes ARM coverage of the one lane that exercises the multiprocess/v2transport configuration, so any ARM-only regression there will now be invisible to CI. The new `linux64_x86canary` lane already provides x86 smoke coverage, so moving multiprocess back off ARM does not serve that goal. Either restore multiprocess to ARM runners (keeping the x86 canary as the sole x86 coverage addition), or — if the move is intentional (e.g. multiprocess is currently unstable on ARM) — update the PR description and commit message to state that multiprocess was moved to x86 and explain why.
In `ci/dash/test_integrationtests.sh`:
- [SUGGESTION] line 47: PASS_ARGS / TEST_RUNNER_EXTRA order swapped — silently flips precedence for all CI targets
The diff swaps the order from `... ${TEST_RUNNER_EXTRA} ... $PASS_ARGS $EXTRA_ARGS` to `... $PASS_ARGS ${TEST_RUNNER_EXTRA} ... $EXTRA_ARGS`. With argparse, later occurrences of overlapping flags (e.g. `--exclude ...`) take precedence, so `TEST_RUNNER_EXTRA` (set per-target in `00_setup_env_native_*.sh`) now overrides `PASS_ARGS` (the args forwarded via `INTEGRATION_TESTS_ARGS` from test-src.yml) for every existing target — linux64, sqlite, ubsan, tsan, multiprocess, nowallet — not just the new x86canary lane. If this was intentional so that target-specific excludes (like aarch64 qt5's `feature_unsupported_utxo_db` exclusion) survive external overrides, split it into its own commit with an explanation; otherwise revert the swap, since it's a silent behavioral change hidden inside a runner-migration commit.
| @@ -266,7 +265,7 @@ jobs: | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| src-linux64_tsan: | |||
| name: linux64_tsan-build | |||
| @@ -284,14 +283,27 @@ jobs: | |||
| src-linux64_ubsan: | |||
| name: linux64_ubsan-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-linux64] | |||
| needs: [check-skip, container, depends-linux64-x86] | |||
| if: ${{ vars.SKIP_LINUX64_UBSAN == '' }} | |||
| with: | |||
| build-target: linux64_ubsan | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| depends-key: ${{ needs.depends-linux64-x86.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64-x86.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64-x86.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-linux64_x86canary: | |||
| name: linux64_x86canary-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-linux64-x86] | |||
| if: ${{ vars.SKIP_LINUX64_X86CANARY == '' }} | |||
| with: | |||
| build-target: linux64_x86canary | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-linux64-x86.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64-x86.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64-x86.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-mac: | |||
| @@ -326,7 +338,7 @@ jobs: | |||
| bundle-key: ${{ needs.src-linux64.outputs.key }} | |||
| build-target: linux64 | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| test-linux64_multiprocess: | |||
| name: linux64_multiprocess-test | |||
| @@ -336,7 +348,7 @@ jobs: | |||
| bundle-key: ${{ needs.src-linux64_multiprocess.outputs.key }} | |||
| build-target: linux64_multiprocess | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
There was a problem hiding this comment.
🟡 Suggestion: multiprocess lane moved from ARM to x86 — contradicts PR description and drops ARM coverage
src-linux64_multiprocess (build.yml:232-243) now depends on the new x86-only depends-linux64_multiprocess-x86 (build.yml:154-163) and runs on runner-amd64, and test-linux64_multiprocess (build.yml:343-351) likewise runs on runner-amd64. Only the TSAN lane still consumes the ARM-side depends-linux64_multiprocess. This contradicts the PR description's claim that "multiprocess + tsan were already on ARM — no change" and removes ARM coverage of the one lane that exercises the multiprocess/v2transport configuration, so any ARM-only regression there will now be invisible to CI. The new linux64_x86canary lane already provides x86 smoke coverage, so moving multiprocess back off ARM does not serve that goal. Either restore multiprocess to ARM runners (keeping the x86 canary as the sole x86 coverage addition), or — if the move is intentional (e.g. multiprocess is currently unstable on ARM) — update the PR description and commit message to state that multiprocess was moved to x86 and explain why.
source: ['claude', '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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 232-351: multiprocess lane moved from ARM to x86 — contradicts PR description and drops ARM coverage
`src-linux64_multiprocess` (build.yml:232-243) now depends on the new x86-only `depends-linux64_multiprocess-x86` (build.yml:154-163) and runs on `runner-amd64`, and `test-linux64_multiprocess` (build.yml:343-351) likewise runs on `runner-amd64`. Only the TSAN lane still consumes the ARM-side `depends-linux64_multiprocess`. This contradicts the PR description's claim that "multiprocess + tsan were already on ARM — no change" and removes ARM coverage of the one lane that exercises the multiprocess/v2transport configuration, so any ARM-only regression there will now be invisible to CI. The new `linux64_x86canary` lane already provides x86 smoke coverage, so moving multiprocess back off ARM does not serve that goal. Either restore multiprocess to ARM runners (keeping the x86 canary as the sole x86 coverage addition), or — if the move is intentional (e.g. multiprocess is currently unstable on ARM) — update the PR description and commit message to state that multiprocess was moved to x86 and explain why.
| set +e | ||
| # shellcheck disable=SC2086 | ||
| LD_LIBRARY_PATH="$DEPENDS_DIR/$HOST/lib" ./test/functional/test_runner.py --ci --attempts=3 --ansi --combinedlogslen=4000 --timeout-factor="${TEST_RUNNER_TIMEOUT_FACTOR}" ${TEST_RUNNER_EXTRA} --failfast --nocleanup --tmpdir="$(pwd)/testdatadirs" $PASS_ARGS $EXTRA_ARGS | ||
| 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 |
There was a problem hiding this comment.
🟡 Suggestion: PASS_ARGS / TEST_RUNNER_EXTRA order swapped — silently flips precedence for all CI targets
The diff swaps the order from ... ${TEST_RUNNER_EXTRA} ... $PASS_ARGS $EXTRA_ARGS to ... $PASS_ARGS ${TEST_RUNNER_EXTRA} ... $EXTRA_ARGS. With argparse, later occurrences of overlapping flags (e.g. --exclude ...) take precedence, so TEST_RUNNER_EXTRA (set per-target in 00_setup_env_native_*.sh) now overrides PASS_ARGS (the args forwarded via INTEGRATION_TESTS_ARGS from test-src.yml) for every existing target — linux64, sqlite, ubsan, tsan, multiprocess, nowallet — not just the new x86canary lane. If this was intentional so that target-specific excludes (like aarch64 qt5's feature_unsupported_utxo_db exclusion) survive external overrides, split it into its own commit with an explanation; otherwise revert the swap, since it's a silent behavioral change hidden inside a runner-migration commit.
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 `ci/dash/test_integrationtests.sh`:
- [SUGGESTION] line 47: PASS_ARGS / TEST_RUNNER_EXTRA order swapped — silently flips precedence for all CI targets
The diff swaps the order from `... ${TEST_RUNNER_EXTRA} ... $PASS_ARGS $EXTRA_ARGS` to `... $PASS_ARGS ${TEST_RUNNER_EXTRA} ... $EXTRA_ARGS`. With argparse, later occurrences of overlapping flags (e.g. `--exclude ...`) take precedence, so `TEST_RUNNER_EXTRA` (set per-target in `00_setup_env_native_*.sh`) now overrides `PASS_ARGS` (the args forwarded via `INTEGRATION_TESTS_ARGS` from test-src.yml) for every existing target — linux64, sqlite, ubsan, tsan, multiprocess, nowallet — not just the new x86canary lane. If this was intentional so that target-specific excludes (like aarch64 qt5's `feature_unsupported_utxo_db` exclusion) survive external overrides, split it into its own commit with an explanation; otherwise revert the swap, since it's a silent behavioral change hidden inside a runner-migration commit.
| depends-linux64: | ||
| name: linux64 (native) | ||
| uses: ./.github/workflows/build-depends.yml | ||
| needs: [check-skip, container, cache-sources] | ||
| if: ${{ vars.SKIP_ARM_LINUX == '' }} | ||
| if: | | ||
| vars.SKIP_LINUX64 == '' || | ||
| vars.SKIP_LINUX64_FUZZ == '' || | ||
| vars.SKIP_LINUX64_SQLITE == '' | ||
| with: | ||
| build-target: aarch64-linux | ||
| build-target: linux64 | ||
| container-path: ${{ needs.container.outputs.path }} | ||
| base-image-digest: ${{ needs.check-skip.outputs.base-image-digest }} | ||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | ||
| effective-runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} |
There was a problem hiding this comment.
💬 Nitpick: runs-on is dead when effective-runs-on is also supplied on depends-linux64
depends-linux64 passes both runs-on: runner-amd64 and effective-runs-on: runner-arm64. In build-depends.yml every consumer (check-cache.runs-on at line 40, build.runs-on at line 106, and the cache-key prefix at line 74) reads inputs.effective-runs-on || inputs.runs-on, so the runner-amd64 value is never actually used — the job executes on ARM and the cache key contains runner-arm64. The call site reads as if the fallback matters, which it doesn't. Either drop runs-on here and set the ARM runner directly, or add a comment explaining what runs-on means when effective-runs-on is also set (logical target vs physical runner).
💡 Suggested change
| depends-linux64: | |
| name: linux64 (native) | |
| uses: ./.github/workflows/build-depends.yml | |
| needs: [check-skip, container, cache-sources] | |
| if: ${{ vars.SKIP_ARM_LINUX == '' }} | |
| if: | | |
| vars.SKIP_LINUX64 == '' || | |
| vars.SKIP_LINUX64_FUZZ == '' || | |
| vars.SKIP_LINUX64_SQLITE == '' | |
| with: | |
| build-target: aarch64-linux | |
| build-target: linux64 | |
| container-path: ${{ needs.container.outputs.path }} | |
| base-image-digest: ${{ needs.check-skip.outputs.base-image-digest }} | |
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |
| effective-runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |
| depends-linux64: | |
| name: linux64 (native) | |
| uses: ./.github/workflows/build-depends.yml | |
| needs: [check-skip, container, cache-sources] | |
| if: | | |
| vars.SKIP_LINUX64 == '' || | |
| vars.SKIP_LINUX64_FUZZ == '' || | |
| vars.SKIP_LINUX64_SQLITE == '' | |
| with: | |
| build-target: linux64 | |
| container-path: ${{ needs.container.outputs.path }} | |
| base-image-digest: ${{ needs.check-skip.outputs.base-image-digest }} | |
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} |
source: ['claude']
| source ./ci/test/00_setup_env_native_qt5.sh | ||
|
|
||
| # Base-only functional tests: the ARM lane already runs --extended, so we | ||
| # only need a fast smoke suite on x86 to catch wallet/core regressions | ||
| # that might depend on x86 behavior (alignment, SSE, glibc-on-x86 quirks). | ||
| # Drop --extended and --previous-releases; base tests don't need them. | ||
| # Drop --coverage too: the coverage check requires every RPC to be | ||
| # exercised (e.g. pruneblockchain needs feature_pruning), and those | ||
| # tests are extended-only — the ARM lane owns RPC coverage tracking. | ||
| export TEST_RUNNER_EXTRA="--exclude feature_pruning,feature_dbcrash" | ||
| export DOWNLOAD_PREVIOUS_RELEASES="false" | ||
| unset PREVIOUS_RELEASES_TAGS |
There was a problem hiding this comment.
💬 Nitpick: x86 canary inherits RUN_UNIT_TESTS=false from qt5 — skips the tests most likely to catch x86-specific issues
By sourceing 00_setup_env_native_qt5.sh (which sets RUN_UNIT_TESTS=false / RUN_UNIT_TESTS_SEQUENTIAL=true at lines 21-22), x86canary doesn't run unit tests. The stated intent of the canary (comments at lines 14-20: "catch wallet/core regressions that might depend on x86 behavior — alignment, SSE, glibc-on-x86 quirks") is exactly the kind of thing unit tests, especially crypto/serialization tests, are best at surfacing. Running only the slimmed --exclude feature_pruning,feature_dbcrash functional suite on x86 leaves most low-level x86-specific logic untested. Consider re-enabling RUN_UNIT_TESTS=true for the canary to get cheap, fast coverage of the behavior this lane is meant to guard. Non-blocking, but worth reconsidering the design.
source: ['claude']
| with: | ||
| path: | | ||
| releases | ||
| key: releases-${{ hashFiles('ci/test/00_setup_env_native_qt5.sh', 'test/get_previous_releases.py') }} | ||
| key: releases-${{ runner.arch }}-${{ hashFiles('ci/test/00_setup_env_native_qt5.sh', 'test/get_previous_releases.py') }} |
There was a problem hiding this comment.
💬 Nitpick: Releases cache keyed by runner.arch but still gated on build-target == 'linux64'
The cache step correctly includes ${{ runner.arch }} in the key so aarch64 and x86 binaries don't collide, but if: inputs.build-target == 'linux64' means only the primary linux64 target benefits from this cache. linux64_x86canary sets DOWNLOAD_PREVIOUS_RELEASES=false, so it's fine today — but if another target on another runner ever enables previous-release downloads, it'll silently re-download every run. Either expand the gate (non-trivial since DOWNLOAD_PREVIOUS_RELEASES is decided inside the container script), or add a comment noting the cache is intentionally linux64-only.
source: ['claude']
| elif platform in ["aarch64-linux-gnu"]: | ||
| print(f"Binaries not available for {tag} on {platform}") | ||
| return 1 |
There was a problem hiding this comment.
💬 Nitpick: New aarch64 branch in tag < v0.12.3 block is unreachable in CI
The new elif platform in ["aarch64-linux-gnu"]: ... return 1 sits inside elif tag < "v0.12.3":, so it only fires for v0.12.1.5. 00_setup_env_native_qt5.sh (line 17) already limits PREVIOUS_RELEASES_TAGS on aarch64 to tags ≥ v0.15, so this branch is never reached in CI. It's a reasonable defensive guard for manual runs, but since the behavior is identical to the x86_64-apple-darwin/arm64-apple-darwin arm right next to it, factoring them together is clearer.
💡 Suggested change
| elif platform in ["aarch64-linux-gnu"]: | |
| print(f"Binaries not available for {tag} on {platform}") | |
| return 1 | |
| elif platform in ["x86_64-apple-darwin", "arm64-apple-darwin", "aarch64-linux-gnu"]: | |
| print(f"Binaries not available for {tag} on {platform}") | |
| return 1 |
source: ['claude']
| # Tests that run natively detect the host based on architecture. | ||
| # We use explicit triplets rather than config.guess to ensure they match | ||
| # the triplets used by depends (e.g. aarch64-linux-gnu, not aarch64-unknown-linux-gnu). | ||
| if [ -z "$HOST" ]; then | ||
| case "$(uname -m)" in | ||
| aarch64) | ||
| export HOST=aarch64-linux-gnu | ||
| ;; | ||
| x86_64) | ||
| export HOST=x86_64-pc-linux-gnu | ||
| ;; | ||
| *) | ||
| if command -v dpkg >/dev/null 2>&1; then | ||
| arch="$(dpkg --print-architecture)" | ||
| if [ "${arch}" = "arm64" ]; then | ||
| export HOST=aarch64-linux-gnu | ||
| elif [ "${arch}" = "amd64" ]; then | ||
| export HOST=x86_64-pc-linux-gnu | ||
| fi | ||
| fi | ||
| # Final fallback to config.guess | ||
| export HOST=${HOST:-$("$BASE_ROOT_DIR/depends/config.guess")} | ||
| ;; | ||
| esac | ||
| fi |
There was a problem hiding this comment.
💬 Nitpick: Catch-all branch may fall back to config.guess triplet that doesn't match depends directory
In the *) branch, if uname -m is neither aarch64 nor x86_64, dpkg is then consulted; if dpkg returns something other than arm64 or amd64, HOST stays unset and the final export HOST=${HOST:-$("$BASE_ROOT_DIR/depends/config.guess")} takes over. For exotic archs that fallback yields strings like armv7l-unknown-linux-gnueabihf, which typically don't match any depends/$HOST/ directory — so subsequent $DEPENDS_DIR/$HOST/lib usage will fail. Not a regression (old single-line fallback had the same issue), but since this PR is explicitly centralizing HOST detection to avoid the aarch64-unknown-linux-gnu vs aarch64-linux-gnu mismatch, consider either documenting that exotic archs aren't supported or emitting a warning from the catch-all. Low priority.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The ARM migration is only partially implemented. The main linux64 and linux64_sqlite lanes were moved to arm64 as intended, but the workflow still leaves linux64_multiprocess and linux64_ubsan on amd64 even though the commit message describes ubsan as migrated and implies the remaining x86 jobs are only nowallet, mac, win64, and lint.
Reviewed commit: ff5ff35
🟡 2 suggestion(s)
🤖 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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 154-351: The regular multiprocess lane was moved off ARM, leaving no non-TSAN arm64 coverage for that configuration
This change introduces `depends-linux64_multiprocess-x86` on amd64 and rewires both `src-linux64_multiprocess` and `test-linux64_multiprocess` to use it and run on `runner-amd64`. The existing arm64 `depends-linux64_multiprocess` job is now only consumed by `linux64_tsan`. That is a real coverage regression: the only normal multiprocess/v2transport build-and-test path no longer runs on arm64, and TSAN is not a substitute because it exercises a different sanitizer configuration. If the intent is to keep multiprocess on x86, the commit message and PR summary need to say so explicitly; otherwise this lane should stay on arm64 so the migration does not silently drop native ARM coverage.
- [SUGGESTION] lines 130-391: UBSAN was not actually migrated to arm64
`depends-linux64-x86` is the only depends cache feeding `src-linux64_ubsan`, and both `src-linux64_ubsan` and `test-linux64_ubsan` still run on `runner-amd64`. That directly contradicts the commit message, which says `depends-linux64` and its consumers, including `ubsan`, were moved to ARM and lists only `nowallet`, `mac`, `win64`, and `lint` as the jobs remaining on x86. Either move the UBSAN lane onto arm64 or correct the change description to state that UBSAN is intentionally still x86-only.
| @@ -192,18 +203,6 @@ jobs: | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-aarch64-linux: | |||
| name: aarch64-linux-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-aarch64-linux] | |||
| with: | |||
| build-target: aarch64-linux | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-aarch64-linux.outputs.key }} | |||
| depends-host: ${{ needs.depends-aarch64-linux.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-aarch64-linux.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-linux64: | |||
| name: linux64-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| @@ -215,7 +214,7 @@ jobs: | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| src-linux64_fuzz: | |||
| name: linux64_fuzz-build | |||
| @@ -228,20 +227,20 @@ jobs: | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| src-linux64_multiprocess: | |||
| name: linux64_multiprocess-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-linux64_multiprocess, lint] | |||
| needs: [check-skip, container, depends-linux64_multiprocess-x86, lint] | |||
| if: ${{ vars.SKIP_LINUX64_MULTIPROCESS == '' }} | |||
| with: | |||
| build-target: linux64_multiprocess | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-linux64_multiprocess.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64_multiprocess.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64_multiprocess.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
| depends-key: ${{ needs.depends-linux64_multiprocess-x86.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64_multiprocess-x86.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64_multiprocess-x86.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-linux64_nowallet: | |||
| name: linux64_nowallet-build | |||
| @@ -266,7 +265,7 @@ jobs: | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| src-linux64_tsan: | |||
| name: linux64_tsan-build | |||
| @@ -284,14 +283,27 @@ jobs: | |||
| src-linux64_ubsan: | |||
| name: linux64_ubsan-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-linux64] | |||
| needs: [check-skip, container, depends-linux64-x86] | |||
| if: ${{ vars.SKIP_LINUX64_UBSAN == '' }} | |||
| with: | |||
| build-target: linux64_ubsan | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| depends-key: ${{ needs.depends-linux64-x86.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64-x86.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64-x86.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-linux64_x86canary: | |||
| name: linux64_x86canary-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-linux64-x86] | |||
| if: ${{ vars.SKIP_LINUX64_X86CANARY == '' }} | |||
| with: | |||
| build-target: linux64_x86canary | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-linux64-x86.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64-x86.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64-x86.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-mac: | |||
| @@ -326,7 +338,7 @@ jobs: | |||
| bundle-key: ${{ needs.src-linux64.outputs.key }} | |||
| build-target: linux64 | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| test-linux64_multiprocess: | |||
| name: linux64_multiprocess-test | |||
| @@ -336,7 +348,7 @@ jobs: | |||
| bundle-key: ${{ needs.src-linux64_multiprocess.outputs.key }} | |||
| build-target: linux64_multiprocess | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
There was a problem hiding this comment.
🟡 Suggestion: The regular multiprocess lane was moved off ARM, leaving no non-TSAN arm64 coverage for that configuration
This change introduces depends-linux64_multiprocess-x86 on amd64 and rewires both src-linux64_multiprocess and test-linux64_multiprocess to use it and run on runner-amd64. The existing arm64 depends-linux64_multiprocess job is now only consumed by linux64_tsan. That is a real coverage regression: the only normal multiprocess/v2transport build-and-test path no longer runs on arm64, and TSAN is not a substitute because it exercises a different sanitizer configuration. If the intent is to keep multiprocess on x86, the commit message and PR summary need to say so explicitly; otherwise this lane should stay on arm64 so the migration does not silently drop native ARM coverage.
source: ['claude', '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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 154-351: The regular multiprocess lane was moved off ARM, leaving no non-TSAN arm64 coverage for that configuration
This change introduces `depends-linux64_multiprocess-x86` on amd64 and rewires both `src-linux64_multiprocess` and `test-linux64_multiprocess` to use it and run on `runner-amd64`. The existing arm64 `depends-linux64_multiprocess` job is now only consumed by `linux64_tsan`. That is a real coverage regression: the only normal multiprocess/v2transport build-and-test path no longer runs on arm64, and TSAN is not a substitute because it exercises a different sanitizer configuration. If the intent is to keep multiprocess on x86, the commit message and PR summary need to say so explicitly; otherwise this lane should stay on arm64 so the migration does not silently drop native ARM coverage.
| @@ -142,17 +144,26 @@ jobs: | |||
| name: linux64_multiprocess | |||
| uses: ./.github/workflows/build-depends.yml | |||
| needs: [check-skip, container, cache-sources] | |||
| if: | | |||
| vars.SKIP_LINUX64_MULTIPROCESS == '' || | |||
| vars.SKIP_LINUX64_TSAN == '' | |||
| if: ${{ vars.SKIP_LINUX64_TSAN == '' }} | |||
| with: | |||
| build-target: linux64_multiprocess | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| base-image-digest: ${{ needs.check-skip.outputs.base-image-digest }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| depends-linux64_multiprocess-x86: | |||
| name: linux64_multiprocess (x86) | |||
| uses: ./.github/workflows/build-depends.yml | |||
| needs: [check-skip, container, cache-sources] | |||
| if: ${{ vars.SKIP_LINUX64_MULTIPROCESS == '' }} | |||
| with: | |||
| build-target: linux64_multiprocess | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| base-image-digest: ${{ needs.check-skip.outputs.base-image-digest }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| depends-linux64_nowallet: | |||
| name: x86_64-pc-linux-gnu_nowallet | |||
| name: linux64_nowallet (native) | |||
| uses: ./.github/workflows/build-depends.yml | |||
| needs: [check-skip, container, cache-sources] | |||
| if: ${{ vars.SKIP_LINUX64_NOWALLET == '' }} | |||
| @@ -192,18 +203,6 @@ jobs: | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-aarch64-linux: | |||
| name: aarch64-linux-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-aarch64-linux] | |||
| with: | |||
| build-target: aarch64-linux | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-aarch64-linux.outputs.key }} | |||
| depends-host: ${{ needs.depends-aarch64-linux.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-aarch64-linux.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-linux64: | |||
| name: linux64-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| @@ -215,7 +214,7 @@ jobs: | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| src-linux64_fuzz: | |||
| name: linux64_fuzz-build | |||
| @@ -228,20 +227,20 @@ jobs: | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| src-linux64_multiprocess: | |||
| name: linux64_multiprocess-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-linux64_multiprocess, lint] | |||
| needs: [check-skip, container, depends-linux64_multiprocess-x86, lint] | |||
| if: ${{ vars.SKIP_LINUX64_MULTIPROCESS == '' }} | |||
| with: | |||
| build-target: linux64_multiprocess | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-linux64_multiprocess.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64_multiprocess.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64_multiprocess.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
| depends-key: ${{ needs.depends-linux64_multiprocess-x86.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64_multiprocess-x86.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64_multiprocess-x86.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-linux64_nowallet: | |||
| name: linux64_nowallet-build | |||
| @@ -266,7 +265,7 @@ jobs: | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| src-linux64_tsan: | |||
| name: linux64_tsan-build | |||
| @@ -284,14 +283,27 @@ jobs: | |||
| src-linux64_ubsan: | |||
| name: linux64_ubsan-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-linux64] | |||
| needs: [check-skip, container, depends-linux64-x86] | |||
| if: ${{ vars.SKIP_LINUX64_UBSAN == '' }} | |||
| with: | |||
| build-target: linux64_ubsan | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-linux64.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64.outputs.dep-opts }} | |||
| depends-key: ${{ needs.depends-linux64-x86.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64-x86.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64-x86.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-linux64_x86canary: | |||
| name: linux64_x86canary-build | |||
| uses: ./.github/workflows/build-src.yml | |||
| needs: [check-skip, container, depends-linux64-x86] | |||
| if: ${{ vars.SKIP_LINUX64_X86CANARY == '' }} | |||
| with: | |||
| build-target: linux64_x86canary | |||
| container-path: ${{ needs.container.outputs.path }} | |||
| depends-key: ${{ needs.depends-linux64-x86.outputs.key }} | |||
| depends-host: ${{ needs.depends-linux64-x86.outputs.host }} | |||
| depends-dep-opts: ${{ needs.depends-linux64-x86.outputs.dep-opts }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| src-mac: | |||
| @@ -326,7 +338,7 @@ jobs: | |||
| bundle-key: ${{ needs.src-linux64.outputs.key }} | |||
| build-target: linux64 | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| test-linux64_multiprocess: | |||
| name: linux64_multiprocess-test | |||
| @@ -336,7 +348,7 @@ jobs: | |||
| bundle-key: ${{ needs.src-linux64_multiprocess.outputs.key }} | |||
| build-target: linux64_multiprocess | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
|
|
|||
| test-linux64_nowallet: | |||
| name: linux64_nowallet-test | |||
| @@ -356,7 +368,7 @@ jobs: | |||
| bundle-key: ${{ needs.src-linux64_sqlite.outputs.key }} | |||
| build-target: linux64_sqlite | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | |||
|
|
|||
| test-linux64_tsan: | |||
| name: linux64_tsan-test | |||
| @@ -377,3 +389,14 @@ jobs: | |||
| build-target: linux64_ubsan | |||
| container-path: ${{ needs.container-slim.outputs.path }} | |||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | |||
There was a problem hiding this comment.
🟡 Suggestion: UBSAN was not actually migrated to arm64
depends-linux64-x86 is the only depends cache feeding src-linux64_ubsan, and both src-linux64_ubsan and test-linux64_ubsan still run on runner-amd64. That directly contradicts the commit message, which says depends-linux64 and its consumers, including ubsan, were moved to ARM and lists only nowallet, mac, win64, and lint as the jobs remaining on x86. Either move the UBSAN lane onto arm64 or correct the change description to state that UBSAN is intentionally still x86-only.
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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 130-391: UBSAN was not actually migrated to arm64
`depends-linux64-x86` is the only depends cache feeding `src-linux64_ubsan`, and both `src-linux64_ubsan` and `test-linux64_ubsan` still run on `runner-amd64`. That directly contradicts the commit message, which says `depends-linux64` and its consumers, including `ubsan`, were moved to ARM and lists only `nowallet`, `mac`, `win64`, and `lint` as the jobs remaining on x86. Either move the UBSAN lane onto arm64 or correct the change description to state that UBSAN is intentionally still x86-only.
ff5ff35 to
7177e12
Compare
|
Follow-up on the latest automated review:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified Codex's single suggestion: the regular multiprocess CI lane (build + test) is now fully on amd64, while only the TSAN variant retains arm64 multiprocess coverage. This is an intentional decision per the PR description (ARM instability), but the suggestion to track restoring native ARM canary coverage is reasonable. No blocking issues.
Reviewed commit: 7177e12
🟡 1 suggestion(s)
🤖 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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 143-351: Non-TSAN multiprocess lane no longer has native ARM coverage
After this migration, `depends-linux64_multiprocess` on arm64 (lines 143–152) is only triggered when `SKIP_LINUX64_TSAN == ''`, and the only consumer of that arm64 depends cache is the TSAN build/test pair (`src-linux64_tsan`/`test-linux64_tsan` at lines 270–281 and 373–381). The regular multiprocess/v2transport lane has been redirected to the new amd64-only `depends-linux64_multiprocess-x86` and runs both build (lines 232–243) and test (lines 343–351) on `runner-amd64`. The PR description states multiprocess was intentionally kept on x86 due to instability observed during ARM validation, but this means any ARM-only regression in the non-TSAN multiprocess configuration will escape CI. TSAN does not substitute for the regular build/test lane (different sanitizer, different runtime characteristics). Consider tracking restoration of native ARM coverage for the non-TSAN multiprocess configuration once the underlying instability is understood, or explicitly document this gap so it is not forgotten.
| bundle-key: ${{ needs.src-linux64_multiprocess.outputs.key }} | ||
| build-target: linux64_multiprocess | ||
| container-path: ${{ needs.container-slim.outputs.path }} | ||
| runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} | ||
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} |
There was a problem hiding this comment.
🟡 Suggestion: Non-TSAN multiprocess lane no longer has native ARM coverage
After this migration, depends-linux64_multiprocess on arm64 (lines 143–152) is only triggered when SKIP_LINUX64_TSAN == '', and the only consumer of that arm64 depends cache is the TSAN build/test pair (src-linux64_tsan/test-linux64_tsan at lines 270–281 and 373–381). The regular multiprocess/v2transport lane has been redirected to the new amd64-only depends-linux64_multiprocess-x86 and runs both build (lines 232–243) and test (lines 343–351) on runner-amd64. The PR description states multiprocess was intentionally kept on x86 due to instability observed during ARM validation, but this means any ARM-only regression in the non-TSAN multiprocess configuration will escape CI. TSAN does not substitute for the regular build/test lane (different sanitizer, different runtime characteristics). Consider tracking restoration of native ARM coverage for the non-TSAN multiprocess configuration once the underlying instability is understood, or explicitly document this gap so it is not forgotten.
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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 143-351: Non-TSAN multiprocess lane no longer has native ARM coverage
After this migration, `depends-linux64_multiprocess` on arm64 (lines 143–152) is only triggered when `SKIP_LINUX64_TSAN == ''`, and the only consumer of that arm64 depends cache is the TSAN build/test pair (`src-linux64_tsan`/`test-linux64_tsan` at lines 270–281 and 373–381). The regular multiprocess/v2transport lane has been redirected to the new amd64-only `depends-linux64_multiprocess-x86` and runs both build (lines 232–243) and test (lines 343–351) on `runner-amd64`. The PR description states multiprocess was intentionally kept on x86 due to instability observed during ARM validation, but this means any ARM-only regression in the non-TSAN multiprocess configuration will escape CI. TSAN does not substitute for the regular build/test lane (different sanitizer, different runtime characteristics). Consider tracking restoration of native ARM coverage for the non-TSAN multiprocess configuration once the underlying instability is understood, or explicitly document this gap so it is not forgotten.
|
Follow-up on the latest automated suggestion: I re-verified the current HEAD and I am not making another branch change here. The non-TSAN
So this review-follow-up is a no-change / already-documented outcome rather than another code edit. Restoring native ARM coverage for the normal multiprocess lane can be handled later once the underlying ARM instability is understood. |
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
7177e12 to
bdde6fc
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
CI infrastructure change migrating primary linux64 lanes to ARM runners with a retained x86 canary lane. The HOST detection centralization, runner-arch-partitioned cache keys, and PREVIOUS_RELEASES_TAGS handling are coherent. Two minor cleanup observations remain: the runs-on input becomes dead/misleading once effective-runs-on is supplied, and the new aarch64-linux-gnu branch in get_previous_releases.py lives inside an arm that is unreachable for the configured ARM tag set. Dropped Claude's third finding because (a) the commit message already documents the PASS_ARGS/TEST_RUNNER_EXTRA swap rationale and (b) the finding misidentified TSAN as UBSAN.
Reviewed commit: bdde6fc
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 127-128: `runs-on: amd64` is dead/misleading once `effective-runs-on: arm64` is also passed
In `depends-linux64`, `runs-on` is wired to `runner-amd64` while `effective-runs-on` is `runner-arm64`. In `build-depends.yml`, the job-level `runs-on:` (lines 40, 106) and the cache-key prefix (line 74) all evaluate `${{ inputs.effective-runs-on || inputs.runs-on }}`, so when `effective-runs-on` is non-empty, `inputs.runs-on` is never read anywhere. The `runner-amd64` value here is purely cosmetic and actively misleading — a reader could plausibly conclude this depends job runs on x86 and produces x86 binaries. It also creates inconsistency risk if a future edit touches one occurrence in `build-depends.yml` and not the other. Either drop `effective-runs-on` and pass `runs-on: runner-arm64` directly (simplest), or document `runs-on` as a nominal label kept for backward-compat metadata only.
| runs-on: ${{ needs.check-skip.outputs['runner-amd64'] }} | ||
| effective-runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }} |
There was a problem hiding this comment.
🟡 Suggestion: runs-on: amd64 is dead/misleading once effective-runs-on: arm64 is also passed
In depends-linux64, runs-on is wired to runner-amd64 while effective-runs-on is runner-arm64. In build-depends.yml, the job-level runs-on: (lines 40, 106) and the cache-key prefix (line 74) all evaluate ${{ inputs.effective-runs-on || inputs.runs-on }}, so when effective-runs-on is non-empty, inputs.runs-on is never read anywhere. The runner-amd64 value here is purely cosmetic and actively misleading — a reader could plausibly conclude this depends job runs on x86 and produces x86 binaries. It also creates inconsistency risk if a future edit touches one occurrence in build-depends.yml and not the other. Either drop effective-runs-on and pass runs-on: runner-arm64 directly (simplest), or document runs-on as a nominal label kept for backward-compat metadata only.
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 `.github/workflows/build.yml`:
- [SUGGESTION] lines 127-128: `runs-on: amd64` is dead/misleading once `effective-runs-on: arm64` is also passed
In `depends-linux64`, `runs-on` is wired to `runner-amd64` while `effective-runs-on` is `runner-arm64`. In `build-depends.yml`, the job-level `runs-on:` (lines 40, 106) and the cache-key prefix (line 74) all evaluate `${{ inputs.effective-runs-on || inputs.runs-on }}`, so when `effective-runs-on` is non-empty, `inputs.runs-on` is never read anywhere. The `runner-amd64` value here is purely cosmetic and actively misleading — a reader could plausibly conclude this depends job runs on x86 and produces x86 binaries. It also creates inconsistency risk if a future edit touches one occurrence in `build-depends.yml` and not the other. Either drop `effective-runs-on` and pass `runs-on: runner-arm64` directly (simplest), or document `runs-on` as a nominal label kept for backward-compat metadata only.
| elif platform in ["aarch64-linux-gnu"]: | ||
| print(f"Binaries not available for {tag} on {platform}") | ||
| return 1 |
There was a problem hiding this comment.
💬 Nitpick: New aarch64-linux-gnu branch is unreachable for the configured ARM tag set
The added branch sits inside the elif tag < "v0.12.3": arm. The ARM lane's PREVIOUS_RELEASES_TAGS (in ci/test/00_setup_env_native_qt5.sh:17) is v21.1.1 v20.1.1 v19.3.0 v18.2.2 v0.17.0.3 v0.16.1.1 v0.15.0.0; lexicographically none of these are < "v0.12.3" (e.g. "v0.15.0.0" > "v0.12.3"), so the new branch is dead for current usage. The actual reason no v0.12.x aarch64 fetch is attempted is that those tags are simply omitted from PREVIOUS_RELEASES_TAGS. The branch is harmless as defensive code, but if the intent is graceful degradation for a hypothetical future caller passing v0.12.x on aarch64, that intent should be commented; otherwise the lines can be removed without behavior change.
source: ['claude']
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
bdde6fc to
335ebd5
Compare
|
Handled the review follow-up from #7232 (review). I removed the misleading |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
ARM runner CI migration is coherent at head 335ebd5. The earlier effective-runs-on indirection finding is resolved (build.yml:127 now passes runner-arm64 directly). One carry-forward nitpick remains: the new aarch64-linux-gnu arm in test/get_previous_releases.py is unreachable for the only configured ARM caller, since none of that lane's PREVIOUS_RELEASES_TAGS are < v0.12.3. Both reviewers independently confirmed; verified against source.
Reviewed commit: 335ebd5
💬 1 nitpick(s)
| elif platform in ["aarch64-linux-gnu"]: | ||
| print(f"Binaries not available for {tag} on {platform}") | ||
| return 1 |
There was a problem hiding this comment.
💬 Nitpick: aarch64-linux-gnu branch is unreachable under the configured ARM tag set
The new aarch64-linux-gnu case is nested under elif tag < "v0.12.3": (test/get_previous_releases.py:127-135). The only ARM caller — ci/test/00_setup_env_native_qt5.sh:17 — exports PREVIOUS_RELEASES_TAGS="v21.1.1 v20.1.1 v19.3.0 v18.2.2 v0.17.0.3 v0.16.1.1 v0.15.0.0", and none of those tags are lexicographically < "v0.12.3" (e.g. "v0.15.0.0" > "v0.12.3"). The actual mechanism preventing v0.12.x aarch64 fetches is the tag-list omission, not this branch. Either drop the lines (no behavior change) or add a comment indicating it is defensive code for hypothetical future callers passing v0.12.x on aarch64.
source: ['claude', 'codex']
Move the primary linux64 path to ARM runners, making aarch64 the default native architecture for the main linux64, fuzz, sqlite, and nowallet lanes. The shared linux64 depends cache now feeds the main, fuzz, and sqlite ARM jobs, while nowallet builds its own no-wallet ARM depends cache and runs build+test natively on arm64. Keep a few lanes on x86 for now: - multiprocess (build + test): the normal lane was unstable on arm64 during validation, so keep the non-TSAN multiprocess path on amd64 for now while TSAN remains on ARM - ubsan (build + test) - linux64_x86canary (build + test): x86 smoke coverage - mac, win64: cross-compile targets - lint: architecture-independent 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. Move HOST detection into 00_setup_env.sh so native targets derive the same exact depends triplets from runner architecture, and keep PASS_ARGS before TEST_RUNNER_EXTRA in test_integrationtests.sh so target-specific test-runner flags can intentionally override the workflow defaults when they overlap. Update previous-release handling for the vendorless aarch64/x86_64 triplets used by the new native linux64 jobs.
thepastaclaw
force-pushed
the
ci/migrate-to-arm-runners
branch
from
335ebd5 to
7cd3bee
Compare
|
Follow-up on @UdjinM6's suggested
PR branch updated at |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR head 7cd3bee is in good shape. The ARM CI migration is internally consistent: ARM previous-release handling is correct, cache keys are partitioned by runner.arch, and the PR description now matches which lanes are intentionally on x86 (multiprocess, ubsan, x86canary) vs ARM (linux64, fuzz, sqlite, nowallet, tsan). Two low-signal cleanup nits remain: the new aarch64-linux-gnu branch in get_previous_releases.py is unreachable for the configured ARM tag set, and the effective-runs-on input on build-depends.yml is no longer used by any caller.
Reviewed commit: 7cd3bee
💬 2 nitpick(s)
| elif platform in ["aarch64-linux-gnu"]: | ||
| print(f"Binaries not available for {tag} on {platform}") | ||
| return 1 |
There was a problem hiding this comment.
💬 Nitpick: aarch64-linux-gnu branch in tag < "v0.12.3" block is unreachable for current callers
The new aarch64-linux-gnu arm at lines 133-135 is nested under elif tag < "v0.12.3":. The only ARM caller in CI is the qt5 lane, which sets PREVIOUS_RELEASES_TAGS="v21.1.1 v20.1.1 v19.3.0 v18.2.2 v0.17.0.3 v0.16.1.1 v0.15.0.0" (ci/test/00_setup_env_native_qt5.sh:17). None of those tags satisfy tag < "v0.12.3" lexicographically (e.g. "v0.15.0.0" > "v0.12.3"), so this branch is dead code in the current matrix. The actual mechanism preventing v0.12.x aarch64 fetches is the tag-list omission, not this branch. Either remove these lines (no behavior change) or add a one-line comment marking the branch as defensive coverage for hypothetical future callers passing v0.12.x tags on aarch64; without that, a reader is likely to misread it as the active guard.
source: ['claude']
| effective-runs-on: | ||
| description: "Optional runner label override used to execute the workflow jobs" | ||
| required: false | ||
| type: string |
There was a problem hiding this comment.
💬 Nitpick: effective-runs-on input is no longer used by any caller
After this PR's cleanup, every call site of build-depends.yml in build.yml passes only runs-on: and never effective-runs-on: (depends-linux64 was the last user and was switched to runner-arm64 directly). The effective-runs-on input here, plus the three ${{ inputs.effective-runs-on || inputs.runs-on }} resolutions at lines 40, 74, and 106, are now dead code and just leave room for future drift between the runner label and the cache-key prefix. Since the indirection no longer serves a purpose, the input and the three effective-runs-on || runs-on resolutions can be collapsed back to plain inputs.runs-on. Not a correctness issue at this head — flagging only as cleanup follow-up.
source: ['claude']
3 participants
Summary
Migrate the primary linux64 CI path from x86 to ARM runners, reducing costs and providing native aarch64 build+test coverage for the main lane.
Changes
Moved to ARM runners (
runner-arm64)depends-linux64(shared depends for linux64, fuzz, sqlite)src-linux64+test-linux64(primary native build+test)src-linux64_fuzz(fuzz build)src-linux64_sqlite+test-linux64_sqlite(SQLite build+test)linux64_nowallet(depends + build + test)src-linux64_tsan+test-linux64_tsan(TSAN stays on ARM)Remaining on x86 (
runner-amd64)linux64_multiprocess(build + test) — intentionally kept on x86 for now after ARM validation exposed unrelated instability in the normal multiprocess lanelinux64_ubsan(build + test)linux64_x86canary(build + test) — new x86 smoke lanemac,win64— cross-compile targetslint— architecture-independentRemoved
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 scriptHOST 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.Functional-test argument precedence
ci/dash/test_integrationtests.shnow keeps workflow-passed args beforeTEST_RUNNER_EXTRAso target-specific excludes/timeouts (for example the ARM qt5 previous-release exclusions) can intentionally override the shared defaults when they overlap.Validation
runner-arm64linux64_multiprocess,linux64_ubsan, andlinux64_x86canaryremain onrunner-amd6400_setup_env.shcovers aarch64/x86_64 with dpkg fallbackaarch64-linux-gnutags used by the ARM qt5 jobdepends-aarch64-linuxorsrc-aarch64-linuxremain inbuild.yml