Add container support to stellar contract build - #2678
Conversation
4e80769 to
60e7f35
Compare
There was a problem hiding this comment.
Pull request overview
Adds containerized contract builds using Docker-compatible or Apple container engines.
Changes:
- Adds image-based builds, probing, resource limits, artifact collection, and interruption handling.
- Integrates asynchronous builds with deploy/upload flows.
- Adds CLI documentation, dependencies, and tests.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
FULL_HELP_DOCS.md |
Documents container build options. |
cmd/soroban-cli/src/commands/mod.rs |
Adds the container options heading. |
cmd/soroban-cli/src/commands/contract/upload.rs |
Awaits automatic builds. |
cmd/soroban-cli/src/commands/contract/mod.rs |
Awaits contract builds. |
cmd/soroban-cli/src/commands/contract/deploy/wasm.rs |
Awaits deploy-time builds. |
cmd/soroban-cli/src/commands/contract/build/container.rs |
Implements containerized builds. |
cmd/soroban-cli/src/commands/contract/build.rs |
Adds container flags and routing. |
cmd/soroban-cli/src/commands/container/shared.rs |
Extends shared engine helpers. |
cmd/soroban-cli/Cargo.toml |
Adds Linux UID/GID support. |
cmd/crates/soroban-test/tests/it/build.rs |
Tests generated container commands. |
Cargo.lock |
Locks the new dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Couple questions about scope and how this will evolve to be the default way to build to make sure what we build now will evolve well into that.
There are also a couple things claude noticed that I've let pass through verbatim because they appear meaningful problems to consider.
| let file = format!("{}.wasm", p.name.replace('-', "_")); | ||
| // The container may build for either wasm target depending on its rust | ||
| // version; fall back to the current host default for the reported path. | ||
| let src = [WASM_TARGET, WASM_TARGET_OLD] |
There was a problem hiding this comment.
collect_built_contracts probes WASM_TARGET before WASM_TARGET_OLD and takes the first path that exists, so a stale wasm from an earlier build with a different target triple can be reported/copied instead of the one the container just built.
Failure scenario: Build once locally/in-container with an older Rust toolchain producing target/wasm32-unknown-unknown/release/foo.wasm; later run stellar contract build --image <old-image> (also wasm32-unknown-unknown) so it overwrites that file, but a leftover target/wasm32v1-none/release/foo.wasm from a still-earlier newer build remains on disk. find(|path| path.exists()) checks WASM_TARGET (wasm32v1-none) first, finds the stale file, and reports/copies it as the freshly built contract — the caller (including contract deploy) silently gets an old wasm instead of the just-built one.
Generated by Claude Code
| /// default-member crates that build a cdylib, mirroring the local build's | ||
| /// package selection. May be empty (no cdylib default members), in which case | ||
| /// the caller falls back to a single no-`--package` build. | ||
| fn resolve_packages(cmd: &Cmd) -> Result<Vec<String>, Error> { |
There was a problem hiding this comment.
resolve_packages/collect_built_contracts don't replicate the local build's manifest-path-based single-package selection (build.rs's packages()), so --image builds a different package set than a local build with the same flags.
Failure scenario: In a workspace with several default-member cdylibs (e.g. contracts/a, contracts/b), run stellar contract build --manifest-path=contracts/a/Cargo.toml --image <img> with no --package. Locally this builds only package a (build.rs's packages() matches by manifest path). With --image, resolve_packages ignores manifest_path entirely and returns every default-member cdylib (a and b), and forwarded_build_args then forwards the same --manifest-path=contracts/a/Cargo.toml alongside --package=b for b's build — an inconsistent flag pairing that doesn't match what was requested. The same divergence occurs when running from a workspace subdirectory instead of using --manifest-path, which build.rs's doc comment says should build just that crate.
Generated by Claude Code
| cmd: &Cmd, | ||
| workspace_root: &Path, | ||
| ) -> Result<Vec<BuiltContract>, super::Error> { | ||
| let md = metadata(cmd).map_err(Error::from)?; |
There was a problem hiding this comment.
container::run invokes cargo metadata (via metadata(cmd)) up to three separate times per build — in resolve_workspace_root, resolve_packages, and collect_built_contracts — instead of computing it once and reusing it.
On a large workspace or a slow/networked filesystem, each cargo metadata subprocess call can add noticeable latency.
| /// digest-pinned image already on disk, and to work offline — e.g. air-gapped | ||
| /// verification against a pinned digest. Fails if the image isn't present. | ||
| #[arg(long, requires = "image", help_heading = HEADING_CONTAINER)] | ||
| pub no_image_pull: bool, |
There was a problem hiding this comment.
It would be more consistent with docker's build command, which I think defaults to no pull if the image is already local.
Anotehr thing to consider is that an airgapped feature probably needs to be about more than just the docker image, but also dependencies for cargo, etc. So we could chase a feature like this, but doing it just for the docker image seems incomplete so I'd chase it separately to the initial add of support.
| /// Container resource limits (`--cpus`, `--memory`) applied to the | ||
| /// `--image` build container. | ||
| #[command(flatten, next_help_heading = HEADING_CONTAINER)] | ||
| pub run_args: ContainerRunArgs, |
There was a problem hiding this comment.
--run-args seems a bit too generic so ambiguous what part of the build process it applies to, --container-run-args?
| /// Container connection options (`--engine`, `--docker-host`) used when | ||
| /// `--image` is set. `--docker-host` is honored only by the docker engine. | ||
| #[command(flatten, next_help_heading = HEADING_CONTAINER)] | ||
| pub container_args: ContainerArgs, |
There was a problem hiding this comment.
Should this be cotainer build args or this applies for both?
| #[arg(long, help_heading = HEADING_CONTAINER)] | ||
| pub image: Option<String>, |
There was a problem hiding this comment.
How can I build with container without choosing the container? Is there a way to do that?
I'm thinking about how we move to auto building with a container if docker is available.
What
Adds
stellar contract build --image <ref>, which builds a contract inside a container image instead of compiling locally. The working tree is bind-mounted at/source, the image's ownstellar contract buildruns there, and the resulting wasm lands on the host — including copies to--out-dirwhen set. Any tag or digest ref is accepted.It reuses the existing container-engine abstraction, so
--engine,--docker-host, and the default engine set bystellar container useall apply, along with resource limits (--cpus,--memory). Workspaces with several cdylibs build each package, sharing one container.Before forwarding flags, the image is probed once (a single throwaway container) for the CLI binary name (
sorobanvsstellar), its version, and the default rustup toolchain. Forwarded flags (--locked,--optimize,--optimize=false) are gated by the image's version so an older image doesn't fail on an unknown flag, andRUSTUP_TOOLCHAINis pinned to the image's own toolchain so arust-toolchain.tomlin the mounted source can't redirect the build.--no-image-pullbuilds against an image already present locally (offline/air-gapped, digest-pinned, or never pushed), and--print-commands-onlyemits a copy-pasteable reproduce line.On Linux the build container is deliberately run as the invoking user's uid:gid (
--user), so wasm written into the bind-mountedtarget/is owned by that user rather than root. Docker Desktop (macOS) and Apple'scontaineralready map ownership to the host user, so this is Linux-only. It assumes the image keepsCARGO_HOME/RUSTUP_HOMEwritable by non-root users, as the official image does.Why
Lets you build a contract inside a container image with a fixed CLI and Rust toolchain, without needing a matching Rust build toolchain locally — no local wasm target and no pinned
rustc. A localcargois still used for workspace discovery (package selection and locating the workspace root); fully removing that dependency by running discovery inside the image is left as a follow-up.Known limitations
The combined image probe requires
/bin/shandrustupin the image (as the official rust-based image provides). With--print-commands-onlynothing is probed, so the reproduce line assumes a currentstellarimage and omits the toolchain pin. Running an arbitrary image with root-owned toolchain dirs may fail the Linux uid:gid build.