fix(docker): add g++ to web build layer for cc-rs C++ detection - #7534
Conversation
The 'cargo web build' step compiles native crates via cc-rs, which requires a C++ compiler. The first apt-get install layer only had pkg-config, causing 'ToolNotFound: failed to find tool c++' during Docker builds. Add g++ to the web-build apt install layer in Dockerfile and Dockerfile.debian. Fixes zeroclaw-labs#7533
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
PR Review — #7534 fix(docker): add g++ to web build layer for cc-rs C++ detection
Reviewer: WareWolf-MoonWall
PR: #7534 — fix(docker): add g++ to web build layer for cc-rs C++ detection (+2/-0, Dockerfile + Dockerfile.debian)
Round: 1 (first review)
Prior blocks: none
CI: passing
Verdict: approve
Summary
Adds g++ to the apt-get install block in both Dockerfiles, placed before pkg-config, to satisfy cc-rs's C++ compiler detection at build time. Fixes #7533 (ToolNotFound for "c++").
Findings
🟢 Fix is correct and minimal
cc-rs requires a C++ compiler on the host image and the images didn't have one. Adding g++ to the same cache layer as pkg-config is the right placement — it doesn't bust the layer cache unnecessarily and keeps the compiler alongside the rest of the build toolchain. No other changes needed.
🟢 Both Dockerfiles updated in lockstep
The change is identical in Dockerfile and Dockerfile.debian, as expected for a dependency that both images share. No drift introduced.
🟡 PR template incomplete
The Security & Privacy Impact and Compatibility sections are absent. Both have trivially-No/Yes answers for a toolchain-only image change, but the template still requires them to be filled before merge.
🟡 No local Docker build output in the PR body
CI green via the build pipeline is sufficient to approve, but for a Dockerfile change the ideal validation evidence is the tail of a docker buildx build --no-cache . run showing a clean exit. Worth including in the template's Validation section for future Dockerfile PRs.
Audacity88
left a comment
There was a problem hiding this comment.
Checked PR #7534's current diff, linked issue #7533, WareWolf-MoonWall's approval, visible CI checks, Docker Image PR Check trigger paths, and current master Dockerfile context.
The code change itself is the right shape: both Dockerfiles add g++ in the web-builder apt layer before cargo web build, matching the cc-rs "c++" failure mode in #7533. Standard Quality Gate CI passed, and WareWolf-MoonWall approved based on source correctness plus green standard CI.
🔴 Blocking — Docker build validation is needed for Dockerfile changes
WareWolf-MoonWall treated the missing Docker build output as a warning rather than a blocker. I’m taking the stricter path here because this PR’s entire purpose is fixing a Dockerfile build failure; for that scope, actual Docker build evidence needs to be present or explicitly waived as source-review-only validation.
This PR fixes a Docker build failure, so merge evidence should prove the changed Dockerfiles build successfully. The Docker Image PR Check workflow is path-filtered to Dockerfile.ci, Dockerfile.debian.ci, workflow files, and scripts/ci/prepare_docker_context.sh; it does not run for root Dockerfile or Dockerfile.debian changes.
The PR body currently claims both Dockerfiles pass in CI, but the visible checks are the standard Quality Gate suite. Those checks passed and are useful, but they do not exercise these changed Dockerfiles.
Please add Docker build output to the PR body's Validation Evidence section, for example:
docker build -f Dockerfile -t zeroclaw:test .
docker build -f Dockerfile.debian -t zeroclaw-debian:test .Paste the tail showing successful image creation, or update the PR body to explicitly say this is source-review-only validation with no local Docker build so maintainers can decide whether standard CI plus source correctness is enough for this low-risk toolchain addition.
|
👋 @Audacity88 — validation evidence section updated:
If maintainers prefer actual Docker build output, happy to add it — I just do not have a local Docker build environment. |
Audacity88
left a comment
There was a problem hiding this comment.
Context checked for this re-review: current head 4e0045e, the updated PR body, linked bug #7533, the two-line Dockerfile diff, root Dockerfile web-builder context, Docker Image PR Check trigger paths, WareWolf-MoonWall's approval, and the visible CI state.
The code change remains the right shape: both root Dockerfiles add g++ in the web-builder apt layer before cargo web build, which directly matches the cc-rs failed to find tool "c++" failure in #7533.
✅ Resolved — Validation evidence now states the Docker build gap
My earlier block asked for either Docker build output or an explicit source-review-only validation statement so maintainers could decide whether to accept the low-risk toolchain fix without local Docker evidence.
The PR body now does that: it says the Docker Image PR Check is path-filtered away from these root Dockerfiles, states that local Docker build validation was skipped, and frames the evidence as source correctness plus green standard CI. That satisfies the blocker I raised.
🟢 What looks good — Minimal fix matches the reported failure
Adding g++ alongside pkg-config in both web-builder stages is the smallest change that gives cc-rs a C++ compiler before cargo web build. The two Dockerfiles stay in lockstep, application code and runtime configuration are unchanged, and the visible Quality Gate checks are green.
This approval is for the source correctness of the package-layer fix plus the now-honest validation disclosure; it is not independent evidence that I ran a local Docker build.
singlerider
left a comment
There was a problem hiding this comment.
Reviewed at head 4e0045e against current master, specifically checking this against the install-consolidation drift gate.
🟢 Correct fix, and in the right (non-generated) region
Adding g++ to the web-build stage's apt install block in both Dockerfile and Dockerfile.debian is the right fix — cc-rs probes for a C++ compiler during the web build, and without g++ that detection fails. I confirmed this does not conflict with the install-consolidation work: xtask/src/generate/container.rs only injects the --features / ZEROCLAW_CARGO_FLAGS line between the # >>> generated:<zone> ... <<< sentinels; it does not regenerate the whole Dockerfile. The web-build apt block (pkg-config, now + g++) sits outside those generated zones, so it is hand-maintained, won't be clobbered by cargo generate installers, and won't trip the drift gate. This is the correct layer to edit for a build-dependency change.
The change is symmetric across both Dockerfiles, minimal (+2/0), and risk: low. @WareWolf-MoonWall and @Audacity88 both approved.
Approving — correct dependency fix, applied in the non-generated portion of both container files so it's compatible with the spec-driven installer pipeline.
Summary
The
cargo web buildstep compiles native crates via cc-rs, which requires a C++ compiler. The firstapt-get installlayer in bothDockerfileandDockerfile.debianonly hadpkg-config, causing:Fix
Add
g++to the web-build apt install layer so that cc-rs can find a C++ compiler beforecargo web buildruns.Changed files
Dockerfile— addg++to first apt install layerDockerfile.debian— sameFixes #7533
Validation Evidence
g++placementg++is placed in the same cache layer aspkg-config, avoiding unnecessary layer cache bustsDocker Image PR Checkworkflow is path-filtered toDockerfile.ci/Dockerfile.debian.ciand does not run for rootDockerfilechanges — this is source-review-only validationDocker Image PR Checkworkflow path filter excludes root Dockerfiles.Security & Privacy Impact
g++is a standard build toolchain package, not a runtime dependencyCompatibility
Rollback
Low-risk PR:
git revert <sha>is the plan.