chore: Add CI for ensuring icp-project builds on wasm - #776
Open
adamspofford-dfinity wants to merge 3 commits into
Open
chore: Add CI for ensuring icp-project builds on wasm#776adamspofford-dfinity wants to merge 3 commits into
adamspofford-dfinity wants to merge 3 commits into
Conversation
adamspofford-dfinity
added this pull request to stack #777
September 11, 2026 15:05
The point of the split was that `icp-project` should end up runnable inside a canister, and nothing was checking it. This adds the job that does — `cargo clippy -p icp-project --no-default-features --target wasm32-unknown-unknown`, on a target where reaching the host does not compile — and fixes everything it found. It has to be its own invocation. A `--workspace` build unifies the `host` feature back on because `icp-cli` enables it, so no whole-workspace command can express this. Which also means `host` has to be strictly additive: it may add implementations, never change what the rest of the crate does, or the job proves nothing about the binary we ship. That is now written down where the feature is declared. Two of the findings were capabilities the core genuinely needs and was taking directly, so they became seams: - `files::FileSystem::scratch_dir` returns a `ScratchDir` handle. `build_many` was calling `camino_tempfile::tempdir()`; a build pipeline does need somewhere one step can leave a wasm for the next step, and then the operation, to read back — so it is something to ask for. - `random::Random`, for `create.rs` picking a subnet when several are available. Its method is `index_below(count)` rather than a byte buffer, because a choice is what every caller wants and turning bytes into an unbiased index is exactly what each one would get subtly wrong. A canister reaches this through `raw_rand`, which is why it is async. The rest is honest gating. `operations::bundle` writes a `.tar.gz` to a disk and puts a plugin's declared directory in as a tree walked for symlinks, which no seam over `FileSystem` reproduces faithfully; `canister::script` spawns a subprocess, and `Builder`/`HostScripts` need it; `stop_signal` off-host waits forever, since there is no process to interrupt and the caller's `select!` arm simply never fires; and the `ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS` read is a host override of a default that stands on its own. One finding was outside this crate. `tokio`'s workspace entry carried `rt-multi-thread`, which no wasm target supports, and feature inheritance only ever adds — so `icp-project` could not opt out of it, and neither could `icp-events`, which this refactor has been claiming is wasm-clean all along. The workspace entry is now the common minimum, and `icp-cli` and `icp-sync-plugin` ask for `rt-multi-thread` where they need it. A second step checks the direction of the dependency itself: no `icp-app` in `icp-project`'s or `icp-sync-plugin`'s manifest, and no `pub use icp_project` facade in `icp-app`. Neither is something a build would catch — a cycle is what cargo would report, and there is nothing to report until someone writes one. Known limitation, left deliberately: `Builder` dispatches both prebuilt steps (which need only `wasm::Fetch` and `files`) and script steps (which need a subprocess), so gating it takes `prebuilt` with it and leaves `canister::build` off-host with a trait and no implementation. Splitting it the way `Syncer` is split — an injected build-script runner — is the fix, and is a design change the gate does not require, so it is follow-up rather than something to smuggle in here. Also dropped `serde_cbor` and `ic-utils`, which nothing in `icp-project` used. Verified: the new job's exact command is clean; 442 unit tests; and 100 integration tests across build (12), create (17), bundle (29), deploy (25) and sync (17) — the suites that cover the scratch directory, the subnet choice and the archive writer.
`RandomError` rendered its boxed cause with `#[snafu(display("{source}"))]`,
which makes the cause both the wrapper's own message and its reported source,
so it prints twice in every chain it reaches. `#[snafu(transparent)]` keeps the
message and drops the wrapper from the chain.
Last of the seam errors carrying a boxed cause.
Building icp-project for wasm32-unknown-unknown does not show a host reach on its own: the target ships a full std whose fs, process and env backends compile and fail only at runtime, so a bare std::fs::read_to_string passes the check that was meant to reject it. What the build does leave behind is one relocatable wasm object per codegen unit, and there every symbol the crate calls but does not define is an import from the `env` module. So read those imports back and reject any that name a corner of std only a host serves. That turns up two. consolidate_manifest asked the filesystem directly whether a network or environment manifest was there, and announce_workspace_root_once canonicalized with dunce. Both go through the files seam now, which leaves dunce host-only. Alongside: - BuildError was uninhabited without `host`, so a Build implemented off this machine had no way to report a failed step but to panic. It carries a boxed cause now, like every other seam's error. - camino-tempfile was host-only, so the crate's tests did not build with host off and nothing exercised that configuration at all. It is a dev-dependency now, the tests that drive host implementations are gated, and CI runs the remaining 183 with host off. - The dependency-direction guard read grep's "could not read the file" as "found nothing", so it would have gone on reporting success after a rename moved one of the paths it searches. - A Random implementation returning an out-of-range index was reported as "no available subnets found", hiding the seam's bug behind a plausible user-facing message.
adamspofford-dfinity
force-pushed
the
spofford/wasm-gate
branch
from
September 11, 2026 15:51
c75e852 to
050fb35
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Remaining direct filesystem probes and dependency-blind symbol inspection prevent reliable host-boundary enforcement.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds WASM compatibility checks and host abstraction boundaries for icp-project.
Changes:
- Adds no-host WASM CI validation and symbol inspection.
- Introduces randomness and filesystem seams for host-independent operation.
- Feature-gates host-only functionality and adjusts Tokio features.
File summaries
| File | Description |
|---|---|
scripts/check-no-host-reach.sh |
Adds host-symbol detection. |
crates/icp-sync-plugin/Cargo.toml |
Enables multithreaded Tokio runtime. |
crates/icp-project/src/store_artifact.rs |
Gates host-dependent tests. |
crates/icp-project/src/signal.rs |
Adds non-host signal behavior. |
crates/icp-project/src/random.rs |
Introduces randomness abstraction. |
crates/icp-project/src/project.rs |
Routes selected file checks through the filesystem seam. |
crates/icp-project/src/operations/mod.rs |
Gates bundle operations. |
crates/icp-project/src/operations/deploy.rs |
Injects randomness into creation. |
crates/icp-project/src/operations/create.rs |
Replaces direct random subnet selection. |
crates/icp-project/src/operations/bundle.rs |
Documents bundle creation. |
crates/icp-project/src/manifest/mod.rs |
Gates filesystem-based tests. |
crates/icp-project/src/lib.rs |
Exposes randomness and abstracts canonicalization. |
crates/icp-project/src/host.rs |
Adds randomness to host resources. |
crates/icp-project/src/canister/sync/script.rs |
Gates host subprocess support. |
crates/icp-project/src/canister/sync/plugin.rs |
Removes environment access off-host. |
crates/icp-project/src/canister/sync/mod.rs |
Gates host sync construction. |
crates/icp-project/src/canister/mod.rs |
Gates host script module. |
crates/icp-project/src/canister/build/mod.rs |
Separates host builder implementation. |
crates/icp-project/Cargo.toml |
Defines host-only dependencies and features. |
crates/icp-cli/src/commands/canister/create.rs |
Supplies host randomness. |
crates/icp-cli/Cargo.toml |
Enables multithreaded Tokio runtime. |
crates/icp-app/src/context/init.rs |
Initializes host randomness. |
Cargo.toml |
Reduces shared Tokio features. |
Cargo.lock |
Removes unused CBOR dependency. |
.github/workflows/checks.yml |
Adds no-host WASM CI job. |
.claude/CLAUDE.md |
Documents the host boundary and checks. |
Review details
- Files reviewed: 25/26 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Item::Path(path) => { | ||
| let path = pdir.join(path); | ||
| if !path.exists() || !path.is_file() { | ||
| if !files.is_file(&path).await { |
|
|
||
| # An rlib also carries cargo's metadata members, which `llvm-nm` reports as | ||
| # holding no symbols; that is expected, so keep it out of the log. | ||
| symbols=$("$nm" --demangle --undefined-only "$rlib" 2> >(grep -v ': no symbols$' >&2)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack created with GitHub Stacks CLI • Give Feedback 💬