ci: add riscv64 C++ CI job on RISE native runners#1
Conversation
Signed-off-by: Bruno Verachten <gounthar@gmail.com>
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
|
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:
📝 WalkthroughWalkthroughAdded a RISC‑V Docker build matrix entry ( Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (PR)
participant GH as GitHub Actions
participant Runner as ubuntu-24.04-riscv (self-hosted)
participant Image as ubuntu-cpp (clang/llvm 18)
participant DockerSvc as Docker daemon
participant Compose as Docker Compose CLI (binary)
Dev->>GH: Push workflow update
GH->>Runner: Schedule job (matrix: riscv64)
Runner->>Image: Pull/start build container
Runner->>Runner: Run "Setup Python" (export $HOME/.local/bin to GITHUB_PATH)
Runner->>Compose: Download/install `docker-compose-linux-riscv64` (chmod + move)
Runner->>Compose: docker compose version
Runner->>DockerSvc: docker info
Runner->>DockerSvc: docker buildx ls
Runner->>GH: Report job status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/cpp.yml (1)
138-139: Prefer exact match for the RISC-V branch in this condition.
contains(matrix.runs-on, 'ubuntu-24.04-riscv')is substring-based for string values. Exact equality is more precise here.Proposed refinement
- name: Setup Python on self-hosted runner if: | contains(matrix.runs-on, 'self-hosted') || - contains(matrix.runs-on, 'ubuntu-24.04-riscv') + matrix.runs-on == 'ubuntu-24.04-riscv'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/cpp.yml around lines 138 - 139, The condition uses contains(matrix.runs-on, 'ubuntu-24.04-riscv') which matches substrings; change it to an exact equality check (matrix.runs-on == 'ubuntu-24.04-riscv') so the RISC-V branch is matched precisely — update the conditional expression that currently reads contains(matrix.runs-on, 'ubuntu-24.04-riscv') to use equality instead.
🤖 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/cpp.yml:
- Around line 105-111: Cache keys for Docker layers currently use only the image
name and can collide across architectures; update the workflow's cache key
generation in the Docker/cache steps to include the matrix.arch (and optional
matrix.ubuntu) so keys become architecture-aware. Locate the matrix entries
(e.g., the riscv64 entry with "arch: riscv64" and "image: ubuntu-cpp") and
change the cache key expression used in the cache or actions/cache steps to
incorporate "${{ matrix.arch }}" (and/or "${{ matrix.ubuntu }}") alongside the
image name so separate caches are created per-architecture.
- Line 109: Replace the loose matrix value usage "runs-on: ubuntu-24.04-riscv"
with an exact equality check in the conditional that currently reads uses
contains(matrix.runs-on, 'self-hosted') || ...; update that condition to:
contains(matrix.runs-on, 'self-hosted') || matrix.runs-on ==
'ubuntu-24.04-riscv' so it matches the exact-matching style used for
matrix.runs-on == 'ubuntu-latest'.
---
Nitpick comments:
In @.github/workflows/cpp.yml:
- Around line 138-139: The condition uses contains(matrix.runs-on,
'ubuntu-24.04-riscv') which matches substrings; change it to an exact equality
check (matrix.runs-on == 'ubuntu-24.04-riscv') so the RISC-V branch is matched
precisely — update the conditional expression that currently reads
contains(matrix.runs-on, 'ubuntu-24.04-riscv') to use equality instead.
There was a problem hiding this comment.
Pull request overview
Adds a new native RISC-V (riscv64) entry to the Arrow C++ GitHub Actions workflow to validate builds/tests on RISE runners, following the existing Docker/archery-based pattern used for other architectures.
Changes:
- Added a new matrix configuration for
riscv64targetingubuntu-24.04-riscvwith LLVM/clang-tools 18. - Extended the “Setup Python on self-hosted runner” step condition to also run on the RISC-V runner label.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - arch: riscv64 | ||
| clang-tools: 18 | ||
| image: ubuntu-cpp | ||
| llvm: 18 | ||
| runs-on: ubuntu-24.04-riscv | ||
| title: RISCV64 Ubuntu 24.04 C++ | ||
| ubuntu: 24.04 |
There was a problem hiding this comment.
The new riscv64 job will likely run with reduced filesystem test coverage: on riscv64, ci/scripts/install_minio.sh and ci/scripts/install_gcs_testbench.sh currently skip installation for unsupported architectures, and ci/scripts/cpp_test.sh then excludes arrow-s3fs-test / arrow-gcsfs-test when minio / storage-testbench aren’t present. If the intent is a “regular” C++ CI job, consider adding riscv64 support for these dependencies (or making the reduced coverage explicit in the workflow/job config).
| - arch: riscv64 | |
| clang-tools: 18 | |
| image: ubuntu-cpp | |
| llvm: 18 | |
| runs-on: ubuntu-24.04-riscv | |
| title: RISCV64 Ubuntu 24.04 C++ | |
| ubuntu: 24.04 | |
| # NOTE: On riscv64, MinIO and the GCS storage testbench are not installed, | |
| # so S3/GCS filesystem tests (arrow-s3fs-test / arrow-gcsfs-test) run with | |
| # reduced coverage or are skipped. This job is intentionally marked as such. | |
| - arch: riscv64 | |
| clang-tools: 18 | |
| image: ubuntu-cpp | |
| llvm: 18 | |
| runs-on: ubuntu-24.04-riscv | |
| title: RISCV64 Ubuntu 24.04 C++ (reduced filesystem tests) | |
| ubuntu: 24.04 | |
| filesystem-tests: reduced |
.github/workflows/cpp.yml
Outdated
| if: | | ||
| contains(matrix.runs-on, 'self-hosted') | ||
| contains(matrix.runs-on, 'self-hosted') || | ||
| contains(matrix.runs-on, 'ubuntu-24.04-riscv') |
There was a problem hiding this comment.
The Python-setup condition is now coupled to a specific runner label (contains(matrix.runs-on, 'ubuntu-24.04-riscv')). For maintainability (e.g., if the runner label changes or additional RISC-V labels are introduced), consider keying this off the matrix data (matrix.arch == 'riscv64') or a dedicated matrix flag (e.g., needs-python-apt: true) instead of matching on the runs-on string.
| contains(matrix.runs-on, 'ubuntu-24.04-riscv') | |
| matrix.arch == 'riscv64' |
Signed-off-by: Bruno Verachten <gounthar@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/cpp.yml (1)
105-111:⚠️ Potential issue | 🟠 MajorPrevent cross-architecture Docker cache collisions for
ubuntu-cpp.With the new
riscv64entry, botharm64v8andriscv64shareimage: ubuntu-cpp, but Line 128 keys cache by image only. That can restore incompatible.dockercache data across architectures.Proposed fix
- name: Cache Docker Volumes uses: actions/cache@v5 with: path: .docker - key: ${{ matrix.image }}-${{ hashFiles('cpp/**') }} - restore-keys: ${{ matrix.image }}- + key: ${{ matrix.image }}-${{ matrix.arch }}-${{ hashFiles('cpp/**') }} + restore-keys: | + ${{ matrix.image }}-${{ matrix.arch }}- + ${{ matrix.image }}-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/cpp.yml around lines 105 - 111, The Docker cache for the "ubuntu-cpp" image can collide across architectures (arm64v8 vs riscv64); modify the cache key used for the "ubuntu-cpp" job so it includes the architecture (e.g., append matrix.arch or runner.arch to the cache key string) or include arch in the cache scope/name used where "image: ubuntu-cpp" is referenced so caches are separated per arch; update the cache step that currently keys by image only to incorporate the arch identifier to prevent restoring incompatible .docker cache data for the "ubuntu-cpp" job with arch: riscv64.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/cpp.yml:
- Around line 105-111: The Docker cache for the "ubuntu-cpp" image can collide
across architectures (arm64v8 vs riscv64); modify the cache key used for the
"ubuntu-cpp" job so it includes the architecture (e.g., append matrix.arch or
runner.arch to the cache key string) or include arch in the cache scope/name
used where "image: ubuntu-cpp" is referenced so caches are separated per arch;
update the cache step that currently keys by image only to incorporate the arch
identifier to prevent restoring incompatible .docker cache data for the
"ubuntu-cpp" job with arch: riscv64.
Signed-off-by: Bruno Verachten <gounthar@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/cpp.yml (1)
105-111:⚠️ Potential issue | 🟠 MajorCache key is still architecture-agnostic and can cross-restore wrong Docker cache.
With the new
riscv64entry sharingimage: ubuntu-cpp, Line 128/Line 129 keys can collide acrossarm64v8andriscv64.Proposed fix
- name: Cache Docker Volumes uses: actions/cache@v5 with: path: .docker - key: ${{ matrix.image }}-${{ hashFiles('cpp/**') }} - restore-keys: ${{ matrix.image }}- + key: ${{ matrix.image }}-${{ matrix.arch }}-${{ matrix.ubuntu }}-${{ hashFiles('cpp/**') }} + restore-keys: | + ${{ matrix.image }}-${{ matrix.arch }}-${{ matrix.ubuntu }}- + ${{ matrix.image }}-${{ matrix.arch }}- + ${{ matrix.image }}-
…4 repos) Signed-off-by: Bruno Verachten <gounthar@gmail.com>
|
@coderabbitai Good catch on the cache key. However, this is a pre-existing issue in the upstream workflow - arm64v8 and amd64 already share the same I'll note this as a potential follow-up improvement when submitting upstream, but keep this PR focused on adding the riscv64 matrix entry. |
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to open a GitHub issue on this repository to track the cache key improvement as a follow-up? It could serve as a useful reference when you submit upstream. (^_^) ~🐇 ✏️ Learnings added
|
Dump existing daemon.json for debugging, then reset to a clean config. Fixes 'Client sent an HTTP request to an HTTPS server' from misconfigured registry mirrors on the runner. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
BuildKit causes 'HTTP request to HTTPS server' on RISE runner. No systemd available, so can't restart dockerd. Disable BuildKit for riscv64 (use legacy builder) and add debug output. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/cpp.yml (1)
149-153: Pin Docker Compose to a repo-controlled version.Resolving
releases/latestat runtime makes this leg non-reproducible and lets an upstream Compose release break Arrow CI without any repo change. Prefer a tested version in env/matrix and update it deliberately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/cpp.yml around lines 149 - 153, The workflow currently resolves COMPOSE_VERSION at runtime via releases/latest which makes the job non-reproducible; change it to use a repo-controlled version variable instead (e.g., a matrix/env variable like DOCKER_COMPOSE_VERSION) and stop querying the GitHub API. Update the steps that reference COMPOSE_VERSION (the curl/grep assignment and the download URL lines that use /${COMPOSE_VERSION}/docker-compose-linux-riscv64 and the chmod step) to read from that stable variable, and add a guard that fails the job with a clear error if the variable is unset so CI fails fast when the pinned version is missing.
🤖 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/cpp.yml:
- Around line 105-111: The riscv64 job leg ("arch: riscv64", "title: RISCV64
Ubuntu 24.04 C++") needs a longer GitHub Actions timeout to avoid flakes; add or
increase the workflow/job-level timeout (e.g., set timeout-minutes: 180 or at
least 120) for that job or the containing workflow so the riscv64 leg has
sufficient headroom for Python bootstrap, Compose setup, Docker restart and
Archery install before the build starts.
- Around line 158-166: The workflow currently prints and entirely replaces
/etc/docker/daemon.json via a heredoc (using sudo cat /etc/docker/daemon.json
and sudo tee /etc/docker/daemon.json), which exposes runner-local settings and
clobbers unrelated keys; instead update only the targeted key and avoid leaking
the original: read the file into a temp backup, use a JSON-aware tool (jq) to
merge or set .features["containerd-snapshotter"]=true into the existing JSON (or
create an object if absent), write the merged result to a temp file and
atomically move it to /etc/docker/daemon.json, then restart docker;
alternatively perform this change in runner provisioning so the workflow never
prints or permanently overwrites the host daemon.json.
---
Nitpick comments:
In @.github/workflows/cpp.yml:
- Around line 149-153: The workflow currently resolves COMPOSE_VERSION at
runtime via releases/latest which makes the job non-reproducible; change it to
use a repo-controlled version variable instead (e.g., a matrix/env variable like
DOCKER_COMPOSE_VERSION) and stop querying the GitHub API. Update the steps that
reference COMPOSE_VERSION (the curl/grep assignment and the download URL lines
that use /${COMPOSE_VERSION}/docker-compose-linux-riscv64 and the chmod step) to
read from that stable variable, and add a guard that fails the job with a clear
error if the variable is unset so CI fails fast when the pinned version is
missing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cpp.yml:
- Around line 149-154: The workflow installs an unpinned Compose binary into
/usr/local/lib/docker/cli-plugins using COMPOSE_VERSION resolved from
releases/latest which mutates the self-hosted runner; instead, pin a specific
Compose release (avoid using releases/latest), verify the binary checksum before
use, install it into a job-scoped directory (e.g. $RUNNER_TEMP or another
ephemeral path) rather than /usr/local/lib/docker/cli-plugins, and remove the
binary at the end of the job; update the steps that set COMPOSE_VERSION and the
target path (/usr/local/lib/docker/cli-plugins/docker-compose) to use the pinned
version, checksum verification, ephemeral install location, and a cleanup step,
or alternatively provision Compose into the runner image and remove the install
logic entirely.
- Around line 142-145: The workflow currently calls "python3 -m pip install -U
pip" and adds "$HOME/.local/bin" to GITHUB_PATH, which will fail under PEP 668;
instead create a job-local virtual environment (use python3 -m venv <envdir>),
install/upgrade pip into that venv, and add the venv's bin directory to
GITHUB_PATH so subsequent steps (e.g., "Setup Archery") use the venv; update the
steps surrounding the existing "python3 -m pip install -U pip" and the
"$HOME/.local/bin" echo to reference the venv directory you create.
Use ${archs[$arch]:-} default value to avoid 'unbound variable' error
when running on architectures not in the archs array (e.g. riscv64).
The script already handles missing arches gracefully with exit 0, but
set -eu causes it to crash before reaching that check.
Signed-off-by: Bruno Verachten <gounthar@gmail.com>
- Use --break-system-packages for pip (PEP 668 on Ubuntu 24.04) - Pin docker compose to v5.1.1 instead of floating latest - Use matrix.arch == 'riscv64' instead of matching on runner label - Remove debug output (docker info/buildx ls) Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Build reached 1223/2257 (54%) at 75min timeout. Estimated ~150min total for full build + tests on RISE runner. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Install Docker 29.3.0 from gounthar/docker-for-riscv64 apt repository. This replaces the Ubuntu-packaged Docker 26.1.5 which had BuildKit issues. Re-enables BuildKit (removes DOCKER_BUILDKIT=0 workaround). Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Docker 29.x install doesn't help because the daemon can't be restarted without systemd. The running daemon stays at 26.x with the BuildKit HTTP/HTTPS bug. DOCKER_BUILDKIT=0 with legacy builder works (proven to 63% in previous run). Need Ludovic to update runner image for a proper fix. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Test PR for validating the Arrow C++ CI on native riscv64 RISE runners.
Changes to
.github/workflows/cpp.yml:riscv64/ubuntu-24.04-riscv/ Ubuntu 24.04 / LLVM 18This follows the same pattern as the ARM64 entry (native runner, Docker-based build via archery).
Per pitrou's request on apache#49556: "Can we perhaps start by having a regular C++ CI job on RISC-V?"
Arrow C++ already validated on native riscv64 hardware (BananaPi F3, 1h13m build).
Summary by CodeRabbit