Skip to content

fix: reconcile project bundling with assets sync step removal#585

Merged
lwshang merged 2 commits into
mainfrom
lwshang/fix
Jun 4, 2026
Merged

fix: reconcile project bundling with assets sync step removal#585
lwshang merged 2 commits into
mainfrom
lwshang/fix

Conversation

@lwshang
Copy link
Copy Markdown
Contributor

@lwshang lwshang commented Jun 4, 2026

Problem

CI on main is red. The merge of #530 (project bundling) did not reconcile with #582 (feat(sync)!: remove assets sync step type), which had merged just before it. #530's branch predated #582, so it still referenced the deleted adapter::assets module and SyncStep::Assets variant. The result is a main that does not compile:

error[E0432]: unresolved import `adapter::assets`
  --> crates/icp/src/manifest/mod.rs:19:5

This single compile error caused the checks, Test, and Validate Examples jobs to fail. The bundle_tests integration suite would additionally fail at runtime, since four of its tests drive the now-rejected type: assets step.

Fix

Mirror #582's removal in the bundle path:

  • Drop the stale adapter::assets re-export in manifest/mod.rs.
  • Remove asset-step handling from bundle.rs (the assets::DirField import, the SyncStep::Assets match arms, prepare_asset_step, the asset_dirs field and its archive-write loop). type: assets no longer deserializes, so this code was unreachable.
  • Convert the four asset-based bundle tests to plugin sync steps, which the bundle code already handles. bundle_and_deploy now uses the local example sync plugin (proven end-to-end by sync_plugin_registers_seed_data) instead of the retired @dfinity/asset-canister@v2.1.0 recipe — v2.1.0 emits the removed step, and the recipe's deploy+serve path isn't exercised in CI (validate-examples only runs project show).
  • Hoist build_sync_plugin_example into the shared test common module so sync_tests and bundle_tests share it.

Verification (local)

  • cargo build --workspace and cargo test --workspace --no-run with RUSTFLAGS="-D warnings"
  • cargo clippy --workspace --all-targets ✅ / cargo fmt --all --check
  • cargo test --workspace --lib --bins
  • cargo test --test bundle_tests — all 10 pass, including bundle_and_deploy end-to-end (deploy from extracted bundle; bundled plugin runs and seeds the canister) ✅

🤖 Generated with Claude Code

The merge of #530 (project bundling) did not account for #582, which
removed the `assets` sync step type, leaving `main` unable to compile:
`bundle.rs` and `manifest/mod.rs` still referenced the deleted
`adapter::assets` module and `SyncStep::Assets` variant.

Mirror #582's removal in the bundle path:
- drop the stale `adapter::assets` re-export
- remove the asset-step handling from bundle.rs (import, match arms,
  `prepare_asset_step`, `asset_dirs` field and its archive-write loop);
  `type: assets` no longer deserializes, so this was dead code
- convert the four asset-based bundle tests to plugin sync steps, which
  the bundle code already handles. `bundle_and_deploy` now uses the
  local example sync plugin (proven end-to-end by
  `sync_plugin_registers_seed_data`) instead of the retired
  `@dfinity/asset-canister@v2.1.0` recipe
- hoist `build_sync_plugin_example` into the shared test `common` module

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the main branch build break caused by stale references to the removed assets sync step type (type: assets) that remained in the project-bundling path and its integration tests. It aligns bundling and tests with the post-#582 manifest surface, where type: assets is rejected and plugin-based sync is the supported mechanism.

Changes:

  • Removed the stale adapter::assets re-export and stripped all asset-sync bundling logic from icp project bundle.
  • Updated bundle integration tests to use plugin sync steps (including verifying bundled plugin execution) instead of the retired assets step.
  • Hoisted build_sync_plugin_example into the shared integration-test common module for reuse across suites.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/icp/src/manifest/mod.rs Drops the obsolete adapter::assets re-export to match the removed assets sync step.
crates/icp-cli/src/operations/bundle.rs Removes unreachable asset-step bundling code paths; bundling now only packages plugin sync inputs.
crates/icp-cli/tests/bundle_tests.rs Converts bundle tests from assets sync to plugin sync and updates archive/manifest assertions accordingly.
crates/icp-cli/tests/sync_tests.rs Switches to the shared build_sync_plugin_example helper.
crates/icp-cli/tests/common/mod.rs Adds shared build_sync_plugin_example helper to build the example canister + sync plugin once for tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/icp-cli/tests/bundle_tests.rs Outdated
Comment thread crates/icp-cli/tests/bundle_tests.rs Outdated
Address review feedback: the converted plugin-sync tests use the `dirs`
list field, but two doc comments still said `dir` (singular).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lwshang lwshang marked this pull request as ready for review June 4, 2026 19:10
@lwshang lwshang requested a review from a team as a code owner June 4, 2026 19:10
@lwshang lwshang enabled auto-merge (squash) June 4, 2026 19:10
@lwshang lwshang merged commit c3caf1a into main Jun 4, 2026
90 checks passed
@lwshang lwshang deleted the lwshang/fix branch June 4, 2026 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants