Conversation
There was a problem hiding this comment.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
There was a problem hiding this comment.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
There was a problem hiding this comment.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
There was a problem hiding this comment.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
There was a problem hiding this comment.
🟡 Changes recommended
A critical runtime dependency issue and a moderate failed-run cleanup issue remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds native Linux ARM64 Docker builds and multi-platform manifest publishing, while updating container build dependencies.
Changes:
- Adds AMD64 and ARM64 Docker matrix builds.
- Publishes combined multi-platform manifests.
- Updates Debian, Rust, and Docker compatibility documentation.
File summaries
| File | Summary |
|---|---|
Dockerfile |
Updates builder/runtime images; critically, libudev.so.1 is missing for libhwloc15. |
CHANGELOG.md |
Documents Docker base-image and ARM64 changes. |
.github/workflows/docker.yml |
Adds architecture-specific builds and manifest publishing; failed runs can leave temporary tags behind (moderate). |
Review details
Suppressed comments (1)
.github/workflows/docker.yml:191
- This cleanup step uses the default
success()condition, so it is skipped whenever the manifest preflight orimagetools createfails (and the wholepublishjob is skipped if anydockermatrix entry fails). The architecture-specific images have already been pushed at that point, so failed runs leave$BUILD_TAG-amd64/$BUILD_TAG-arm64tags in Docker Hub indefinitely; add a failure-safe cleanup path (covering both the publish and build failures) for these run-scoped tags.
- name: Remove temporary architecture tags
# A cleanup failure must not invalidate a successfully published image.
continue-on-error: true
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
PR title now matches the required format.
PR title now matches the required format.
2894aab to
411cd90
Compare
63e6739 to
03f3c1d
Compare
PR title now matches the required format.
| platforms: linux/${{ matrix.platform.arch }} | ||
| push: ${{ env.PUBLISH == 'true' }} | ||
| tags: ${{ steps.meta.outputs.tags }} | ||
| tags: filecoin/${{ matrix.image }}:${{ env.BUILD_TAG }}-${{ matrix.platform.arch }} |
There was a problem hiding this comment.
Per 2026-09-10 verbal, this is the recommended change from agent.
Recommended change. Docker's documented pattern for multi-platform builds across separate runners pushes each arch image by digest instead of by a temporary tag, then assembles the index from the digests. That removes the build-<run_id>-* tags, the "Remove temporary architecture tags" step below, and the need for a Docker Hub token with delete rights. It also has no failure-path cleanup problem, because nothing tagged is ever created except the final tags.
Sketch:
- name: Build image
id: build
uses: docker/build-push-action@v5
with:
context: .
platforms: linux/${{ matrix.platform.arch }}
labels: ${{ steps.meta.outputs.labels }}
outputs: type=image,name=filecoin/${{ matrix.image }},push-by-digest=true,name-canonical=true,push=${{ env.PUBLISH == 'true' }}
build-args: |
${{ matrix.network != 'mainnet' && format('GOFLAGS=-tags={0}', matrix.network) || ''}}
- if: env.PUBLISH == 'true'
run: |
mkdir -p "$RUNNER_TEMP/digests"
echo -n "${{ steps.build.outputs.digest }}" > "$RUNNER_TEMP/digests/${{ matrix.platform.arch }}"
- if: env.PUBLISH == 'true'
uses: actions/upload-artifact@v4
with:
name: digests-${{ matrix.image }}-${{ matrix.network }}-${{ matrix.platform.arch }}
path: ${{ runner.temp }}/digests/*and in publish, actions/download-artifact with pattern: digests-${{ matrix.image }}-${{ matrix.network }}-* + merge-multiple: true, then
docker buildx imagetools create "${tag_args[@]}" \
"$IMAGE@$(cat digests/amd64)" "$IMAGE@$(cat digests/arm64)"The existing --dry-run + jq -e preflight works unchanged on digest sources. Reference: https://docs.docker.com/build/ci/github-actions/multi-platform/#distribute-build-across-multiple-runners
Motivation for insisting: the tag-deletion step is currently failing on every publish (see my comment on that step), and the digest approach makes that whole class of problem go away rather than patching it.
| | sort | unique == ["amd64", "arm64"] | ||
| ' "$RUNNER_TEMP/manifest.json" | ||
| docker buildx imagetools create "${tag_args[@]}" "${sources[@]}" | ||
| - name: Remove temporary architecture tags |
There was a problem hiding this comment.
I think you're aware of this agent flag.
Let me know if we need to set a new PAT in lotus (although I would want to make sure you have perms to do this yourself).
Only relevant if you keep the temp-tag approach instead of push-by-digest (see my comment on the tags: line above; with digests this whole step is deleted).
This step does not work today. In the one real publish run on this branch (job 102569293673, Publish Docker (lotus / mainnet) in run 34381379467) the Docker Hub DELETE returned 403 and continue-on-error: true swallowed it:
curl: (22) The requested URL returned error: 403
##[error]Process completed with exit code 22.
All twelve build-34381379467-{network}-{arch} tags are still on Docker Hub for filecoin/lotus and filecoin/lotus-all-in-one. As written, every master push, nightly and release adds another twelve permanent tags per run.
Likely cause: the DOCKERHUB_TOKEN PAT is read/write only; tag deletion needs the delete scope (or a bot user with delete rights on the org repos), so someone with access to the filecoin Hub org would have to rotate the secret. You would also need an if: always()-style cleanup for the failure path, since a failed imagetools create (or a failed build leg) currently leaves the arch tags behind too.
Either way, please do not keep continue-on-error: true silent: let the step fail, or at least echo "::warning::..." so a broken cleanup shows in the run summary.
|
|
||
| publish: | ||
| name: Publish Docker (${{ matrix.image }} / ${{ matrix.network }}) | ||
| needs: docker |
There was a problem hiding this comment.
It looks like there's a behaviour change. IIUC, before this PR each network published independently. Now publish needs every one of the twelve docker matrix legs, so a single failure (say 2k/arm64) blocks filecoin/lotus:master too.
If that is intended, fine, but maybe say so in PR description?
AI solution if not the intended path:
if: ${{ !cancelled() && needs.docker.result != 'cancelled' && (github.event.inputs.publish == 'true' || github.event_name != 'pull_request') }}lets the surviving networks publish, and the existing --dry-run + jq -e preflight already fails cleanly for a network whose arch image is missing.
| username: ${{ vars.DOCKERHUB_USERNAME }} | ||
| password: ${{ secrets.DOCKERHUB_TOKEN }} | ||
| - name: Build and push if channel is set (channel=${{ steps.channel.outputs.channel }}) | ||
| - name: Build image |
There was a problem hiding this comment.
AI flag:
Nothing in CI ever runs the image, so runtime library gaps in lotus-base are only discovered by users (this is what the libudev1 question below was about). Cheap smoke test for the PR path:
- name: Build image
uses: docker/build-push-action@v5
with:
...
load: ${{ env.PUBLISH != 'true' }}
- if: env.PUBLISH != 'true'
name: Smoke test
run: docker run --rm "filecoin/${{ matrix.image }}:${{ env.BUILD_TAG }}-${{ matrix.platform.arch }}" lotus --version(load and push/digest outputs are mutually exclusive on this action, so on publish runs you would pull by digest first, or just rely on the PR run.) It also catches the case where the prebuilt libfilcrypto picks CPU features the runner has but the target does not; the arm64 prebuilt is selected from the runner's /proc/cpuinfo. Related: has anyone run filecoin/lotus:cf64f7ffd under Docker Desktop on Apple Silicon? That is the use case in #13783 and it is the one place a SIGILL would show up.
There was a problem hiding this comment.
Yes. I tested it with Curio devnet using this image to build lotus and miner containers.
There was a problem hiding this comment.
We can probably test in foc-devnet as well.
| ##################################### | ||
| FROM debian:trixie AS lotus-base | ||
| MAINTAINER Lotus Development Team | ||
| RUN apt-get update && apt-get install -y --no-install-recommends libudev1 \ |
There was a problem hiding this comment.
I believe this can be dropped.
In my agent discussions:
debian:trixie already ships libudev1 in the base image (dpkg -s libudev1 succeeds in a fresh debian:trixie container, and ldd libhwloc.so.15 resolves libudev.so.1 without installing anything).
| CARGO_HOME=/usr/local/cargo \ | ||
| PATH=/usr/local/cargo/bin:$PATH \ | ||
| RUST_VERSION=1.86.0 | ||
| RUST_VERSION=1.94.0 |
There was a problem hiding this comment.
Agent flag. I leave to you on whether this is relevant right now.
Is this bump needed? Both arches fetch the prebuilt libfilcrypto (the arm64 build log shows successfully installed prebuilt libfilcrypto from filecoin-ffi v1.36.1), so the Rust toolchain is only used when someone builds with FFI_BUILD_FROM_SOURCE=1. If you keep it, note that the rustup-init download a few lines down is still pinned to rustup 1.25.1 (2022) with hard-coded checksums; a 2026 toolchain installed by a 2022 rustup is untested here. I'd leave RUST_VERSION alone in this PR so the Dockerfile diff is limited to the arm64 change.
There was a problem hiding this comment.
1.86.0 is pretty old, IIRC we're at least 1.94.0 across everything now so I'm fine with this as a housekeeping task
There was a problem hiding this comment.
Even FFI_BUILD_FROM_SOURCE AFAIK will overwrite it by rust_toolchain file in the ffi repo.
| BUILD_TAG: &build-tag build-${{ github.run_id }}-${{ matrix.network }} | ||
| steps: | ||
| - id: channel | ||
| - &channel |
There was a problem hiding this comment.
Nit: YAML anchors are supported by Actions now and the runs prove it parses, but this is the first use in the repo. A one-line comment ("shared with the publish job below via YAML anchors") would save the next reader a search.
Related Issues
Fixes #13783
Proposed Changes
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that: