Skip to content

ci: local Linux check-ubuntu reproducer (#813 follow-up) - #824

Merged
zackees merged 1 commit into
mainfrom
ci/docker-linux-verify-813
Jun 29, 2026
Merged

ci: local Linux check-ubuntu reproducer (#813 follow-up)#824
zackees merged 1 commit into
mainfrom
ci/docker-linux-verify-813

Conversation

@zackees

@zackeeszackees commented Jun 29, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds ci/docker-linux-verify/ — one-shot Docker reproducer for the
    check-ubuntu.yml lane (cargo check + clippy + subprocess lint +
    cargo test).
  • Reuses the existing fbuild-mac-cross image as the base
    (ubuntu:24.04 + soldr + zccache + uv) so we don't add a second
    Dockerfile to maintain.
  • Named Docker volumes back the cargo /target and CARGO_HOME so
    no-op rebuilds are seconds instead of the 4-6 min host-bind-mount
    cost under WSL2 9P.

Why

While admin-merging #823 (the full-async tokio migration for #813)
from a Windows dev box, I needed a fast local proof that the Linux
lane would be green before pushing. The mac-cross Dockerfile already
shipped everything check-ubuntu needs — this PR formalizes a
one-command 'verify on Linux locally' loop on top of it.

Test plan

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added a local Linux verification workflow that lets contributors run the same checks as the Linux CI lane inside Docker before pushing changes.
    • Includes an interactive shell option, cache reset support, and clearer setup for faster repeat runs.
  • Documentation

    • Added step-by-step instructions for running the workflow, including full verification commands and notes on expected limitations.

Adds ci/docker-linux-verify/ — a one-shot reproducer for the
check-ubuntu.yml lane (cargo check + clippy + subprocess lint +
cargo test) that runs locally inside Docker on a Windows host.
Reuses the existing fbuild-mac-cross image (already ubuntu:24.04
with soldr + zccache + uv pre-installed) so there is no second
Dockerfile to keep in sync.
Named Docker volumes back the cargo /target and CARGO_HOME so
no-op rebuilds are single-digit seconds instead of the 4-6 min
that host bind-mounts incur under WSL2's 9P mtime rewriting.
Used to verify the #813 async migration is platform-clean before
admin-merging on Windows — the Linux lane on the merged commit
will go green for the same reason.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a new ci/docker-linux-verify/ directory with three files: a Python script (verify.py) that orchestrates Docker image/volume management and container invocation, a shell script (verify.sh) that runs the CI verification steps inside the container, and a README documenting usage and conventions.

Changes

Local Linux CI verification harness

Layer / File(s)Summary
verify.py Docker orchestrator
ci/docker-linux-verify/verify.py
Defines REPO_ROOT, IMAGE, and named-volume constants; implements _run(), _ensure_image(), _wipe_volumes(), _docker_run(), and main() to manage image builds, volume lifecycle, and container invocation with --shell, --wipe, and --rebuild-image flags.
verify.sh inner CI steps
ci/docker-linux-verify/verify.sh
Runs inside the container with strict shell settings and Cargo env vars; executes soldr toolchain ensure, cargo check, cargo clippy -D warnings, subprocess-spawn lint via find_direct_subprocess.py --fail, and cargo test.
README
ci/docker-linux-verify/README.md
Documents purpose, usage commands, named-volume conventions, incremental-build rationale, and the four verification steps with a scope caveat about the per-MCU build matrix.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A Docker den, a Linux lane,
Named volumes guard the cargo chain,
--shell to hop, --wipe to clear,
The CI stays forever green, my dear!
All GREEN, the rabbit cheers! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title accurately summarizes the new local Linux check-ubuntu reproducer and is specific enough for history scanning.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/docker-linux-verify-813

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/docker-linux-verify/verify.py`:
- Around line 65-69: The _wipe_volumes helper is treating every non-zero result
from docker volume rm as a missing volume, which hides real failures like
“volume in use” or Docker errors. Update _wipe_volumes to distinguish the “does
not exist” case from other failures by inspecting the command result from _run,
and only print the skip message when the volume is actually absent. For any
other failure, surface the error and make the wipe path fail so --wipe does not
incorrectly report success; use the existing _wipe_volumes and _run symbols to
keep the fix localized.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4ab48dc6-c27a-4899-b23d-bcc82ef2f41b

📥 Commits

Reviewing files that changed from the base of the PR and between bcabaa8 and 3b6efa1.

📒 Files selected for processing (3)
  • ci/docker-linux-verify/README.md
  • ci/docker-linux-verify/verify.py
  • ci/docker-linux-verify/verify.sh

Comment on lines +65 to +69
def _wipe_volumes() -> None:
for vol in (VOLUME_TARGET, VOLUME_CARGO_HOME):
rc = _run(["docker", "volume", "rm", vol], check=False)
if rc != 0:
print(f" (volume {vol} did not exist — skipping)", flush=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Don’t collapse every volume-removal failure into “did not exist.”

Line 67 can also fail when the volume is still in use or Docker itself errors. In those cases this branch still prints a benign skip message and, with --wipe alone, exits 0, so users can think they forced a cold rebuild while the old cache is still intact.

Proposed fix
 def _wipe_volumes() -> None:
for vol in (VOLUME_TARGET, VOLUME_CARGO_HOME):
- rc = _run(["docker", "volume", "rm", vol], check=False)- if rc != 0:- print(f" (volume {vol} did not exist — skipping)", flush=True)+ result = subprocess.run(+ ["docker", "volume", "rm", vol],+ check=False,+ capture_output=True,+ text=True,+ )+ if result.returncode == 0:+ continue+ if "no such volume" in result.stderr.lower():+ print(f" (volume {vol} did not exist — skipping)", flush=True)+ continue+ sys.stderr.write(result.stderr)+ sys.exit(result.returncode)
📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def_wipe_volumes() ->None:
forvolin (VOLUME_TARGET, VOLUME_CARGO_HOME):
rc=_run(["docker", "volume", "rm", vol], check=False)
ifrc!=0:
print(f" (volume {vol} did not exist — skipping)", flush=True)
def_wipe_volumes() ->None:
forvolin (VOLUME_TARGET, VOLUME_CARGO_HOME):
result=subprocess.run(
["docker", "volume", "rm", vol],
check=False,
capture_output=True,
text=True,
)
ifresult.returncode==0:
continue
if"no such volume"inresult.stderr.lower():
print(f" (volume {vol} did not exist — skipping)", flush=True)
continue
sys.stderr.write(result.stderr)
sys.exit(result.returncode)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/docker-linux-verify/verify.py` around lines 65 - 69, The _wipe_volumes
helper is treating every non-zero result from docker volume rm as a missing
volume, which hides real failures like “volume in use” or Docker errors. Update
_wipe_volumes to distinguish the “does not exist” case from other failures by
inspecting the command result from _run, and only print the skip message when
the volume is actually absent. For any other failure, surface the error and make
the wipe path fail so --wipe does not incorrectly report success; use the
existing _wipe_volumes and _run symbols to keep the fix localized.

@zackees
zackees merged commit 2667e28 into mainJun 29, 2026
83 of 91 checks passed
zackees added a commit that referenced this pull request Jun 29, 2026
Release the #813 full-async tokio runtime migration. The work landed
across PR #823 (the migration itself, admin-merged), #824 (the local
Docker Linux verify infra), and #825 (cargo fmt + LOC gate fixes for
the migration's wide-ranging edits).
This is the first release on the unified async runtime — every
per-TU compile, every per-platform deploy, every subprocess
invocation now drives through the daemon's single tokio runtime,
making tokio-console visibility complete and removing the rayon /
std::thread::scope hybrid that the migration replaced.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

1 participant

@zackees