fix(deploy): discover guest binaries from an independently-workspaced guest crate - #262
Conversation
… guest crate Fixes logos-co#231. lgs deploy only searched target/riscv-guest and methods/target for guest .bin artefacts. A guest crate carrying its own [workspace] (so it builds independently of the parent, e.g. for a docker-based risc0 toolchain) emits to methods/guest/target/... instead, which was never searched — lgs deploy reported "missing program binary" even though the binary existed. Adds methods/guest/target as a third search root, with a regression test covering the layout.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
weboko
left a comment
There was a problem hiding this comment.
Verified end-to-end against a real localnet, not just by reading the diff.
What I ran (default template project, pinned LEZ cf3639d8, real sequencer_service + wallet):
- Reproduced #231 on
master: moved a builthello_world.bintomethods/guest/target/riscv32im-risc0-zkvm-elf/docker/and ranlgs deploy hello_world→
missing binary hello_world.bin (searched: …/target/riscv-guest, …/methods/target). - Same tree, this branch →
wallet deploy-program …/methods/guest/target/riscv32im-risc0-zkvm-elf/docker/hello_world.bin,program_id: 5c81dd93…,Succeeded: 1. - Followed through to a real transaction:
wallet topup+cargo run --bin run_hello_world <account>submittedtx_hash=ccfb6a3a…and the account nonce advanced — so the program deployed from the new root is genuinely live, not just discovered. - Missing-binary message now lists all three roots.
- Full suite green: 587 lib / 186 CLI / 3 test-node / 5 doctests.
The change itself is right, and the depth/bucket ranking means the new root cannot shadow a canonical release/ artefact today.
One thing to resolve before this lands, because of #260
#260 (approved, same constant, same function) adds a third rank above release: a docker path component directly under the riscv32im* triple wins outright. The layout this PR adds as a search root is exactly that shape:
methods/guest/target/riscv32im-risc0-zkvm-elf/docker/<program>.bin
^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^
So after the rebase, a .bin sitting in this newly-searched root is promoted to the highest rank — above a fresh host-toolchain build. And #260's cleanup (clear_docker_guest_artifacts) only removes target/riscv-guest-docker, so nothing ever clears methods/guest/target.
Concretely: the user in #231 — who has a hand-run cargo risczero build under methods/guest/target — then switches to plain lgs build, and lgs deploy keeps shipping the stale artefact indefinitely. That breaks the invariant #260 states in its ADR entry ("the last lgs build decides what lgs deploy ships").
Two ways out, either is fine:
- rank
dockerhighest only when the match came fromGUEST_DOCKER_TARGET_DIR(scaffold's own deterministic output) rather than from any search root — the adjacency rule then only has to guard that one tree; or - have
clear_docker_guest_artifactsalso clearmethods/guest/target/**/dockerwhen it runs.
Worth a regression test either way, since the two PRs each look correct alone and only interact after the merge. The constant and its doc comment will also conflict textually — expect to redo the rebase by hand rather than take either side.
Secondary, non-blocking: methods/guest is hardcoded, while the guest subcrate name is really [package.metadata.risc0].methods in methods/Cargo.toml (defaulting to guest). #260 already grows a risc0_guest_manifests() that reads exactly that. Deriving the search roots from it instead would cover a renamed guest crate too — reasonable as a follow-up rather than in this PR.
Generated by Claude Code
Fixes #231.
lgs deployonly searchedtarget/riscv-guestandmethods/targetfor guest.binartefacts. A guest crate carrying its own[workspace](so it builds independently of the parent, e.g. for a docker-based risc0 toolchain) emits tomethods/guest/target/riscv32im-risc0-zkvm-elf/docker/<name>.bininstead, which was never searched —lgs deployreportedmissing program binaryeven though the binary existed.This adds
methods/guest/targetas a third search root, per the fix suggested in the issue, with a regression test (finds_binary_in_independent_guest_workspace_layout) covering the layout.