imp: adding 08-wasm build opts with libwasmvm linking disabled #5923
Conversation
damiannolan
requested review from
colin-axner,
AdityaSripal,
charleenfei,
chatton,
DimitrisJim,
srdtrk and
crodriguezvega
as code owners
|
One thing that would be nice to test before merging this is maybe we can try building the gm-demo importing this commit? |
which depends on some types in 08-wasm, right? We could also check binary size for sanity sake |
WalkthroughThe changes involve enhancing the 08-wasm module by adding new functionalities and modifying existing ones to better manage Wasm modules in a Cosmos SDK application. Key updates include the introduction of a new Makefile target for building wasm simapp Docker images, changes in the handling of Wasm engines, and the addition of scripts and configurations to support different build environments. These updates aim to improve the flexibility and usability of the 08-wasm module, particularly in scenarios involving WebAssembly virtual machines and Docker. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (7)
- Makefile (2 hunks)
- modules/light-clients/08-wasm/internal/ibcwasm/expected_interfaces.go (1 hunks)
- modules/light-clients/08-wasm/internal/ibcwasm/wasm_cgo.go (1 hunks)
- modules/light-clients/08-wasm/keeper/keeper.go (2 hunks)
- modules/light-clients/08-wasm/keeper/keeper_cgo.go (1 hunks)
- modules/light-clients/08-wasm/keeper/keeper_no_cgo.go (1 hunks)
- scripts/build-wasm-simapp-docker.sh (1 hunks)
Additional comments: 11
modules/light-clients/08-wasm/internal/ibcwasm/wasm_cgo.go (3)
- 1-1: The build tag
//go:build cgois correctly used to conditionally compile this file when CGO is enabled. This is a standard practice for Go projects that have CGO-specific code.- 5-5: The import of
wasmvmfromgithub.com/CosmWasm/wasmvmis appropriate for working with WebAssembly virtual machines. Ensure that the version ofwasmvmused is compatible with the rest of the project dependencies.- 7-7: The declaration
var _ WasmEngine = (*wasmvm.VM)(nil)is a common Go idiom to ensure at compile time thatwasmvm.VMimplements theWasmEngineinterface. This is a good practice to catch any interface implementation issues early.scripts/build-wasm-simapp-docker.sh (3)
- 3-3: The use of
set -eou pipefailis a best practice in bash scripting as it causes the script to exit on error, treats unset variables as an error, and causes pipelines to fail on the first error. This makes the script more robust and easier to debug.- 5-10: The
build_wasm_imagefunction correctly extracts the version and checksum forlibwasmusing a Python script and then uses these values to build a Docker image. Ensure that the Python scriptget-libwasm-version.pyis present in thescriptsdirectory and functions correctly.- 13-16: Setting a default tag for the Docker image if none is provided (
TAG="${1:-ibc-go-wasm-simd:latest}") is a user-friendly feature. It ensures that the script can be run without specifying a tag, usingibc-go-wasm-simd:latestas the default. This is a good practice for scripts intended to be used in various environments.modules/light-clients/08-wasm/keeper/keeper_no_cgo.go (1)
- 1-1: The build tag
//go:build !cgois correctly used to conditionally compile this file when CGO is disabled. This is a standard practice for Go projects that need to differentiate behavior based on the availability of CGO.modules/light-clients/08-wasm/keeper/keeper_cgo.go (1)
- 1-1: The build tag
//go:build cgois correctly used to conditionally compile this file when CGO is enabled. This ensures that the file's contents are only included in builds where CGO is available, aligning with the project's requirements.modules/light-clients/08-wasm/keeper/keeper.go (2)
- 3-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file
keeper.gocorrectly defines theKeeperstruct and its methods, which are crucial for the management of wasm code storage, migration, and client state within the08-wasmmodule. This centralization of functionality within theKeeperstruct is a good practice for maintainability and clarity of the module's operations.
- 3-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Given the AI-generated summary's mention of the removal of
NewKeeperWithVMandNewKeeperWithConfigfunctions, it's important to ensure that the consolidated logic in theKeeperstruct initialization does not negatively impact the module's functionality or introduce any regressions. This would require thorough testing, especially in scenarios where these functions were previously used.modules/light-clients/08-wasm/internal/ibcwasm/expected_interfaces.go (1)
- 8-13: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file
expected_interfaces.gocorrectly defines theWasmEngine,QueryRouter, andQueryPluginsIinterfaces, which are crucial for the operation of the08-wasmmodule. These interfaces abstract the interaction with wasm code and provide a framework for query functionality, contributing to the modularity and flexibility of the module.
modules/light-clients/08-wasm/internal/ibcwasm/expected_interfaces.go
Outdated
Show resolved
Hide resolved
|
Let's change this to not use the @charleenfei discovered when building binaries that require some |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/08-wasm/doc.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/08-wasm/doc.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Happy to consider this good to go if there is agreement to go ahead with CosmWasm/wasmvm#527 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/08-wasm/keeper/keeper_vm.go (1 hunks)
Additional comments: 2
modules/light-clients/08-wasm/keeper/keeper_vm.go (2)
- 18-64: The implementation of
NewKeeperWithVMcorrectly checks for nil arguments and panics with appropriate messages. This aligns with the PR objectives and ensures early failure with clear error messaging when the function is used without the necessary wasmvm dependencies. Good practice!- 67-84: The implementation of
NewKeeperWithConfigeffectively handles the instantiation of a new Wasm VM instance and delegates toNewKeeperWithVMfor keeper initialization. This approach is consistent with the PR objectives and ensures clear error messaging and early failure when necessary. Well done!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/08-wasm/keeper/keeper_vm.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/08-wasm/keeper/keeper_vm.go
| @@ -4,5 +4,12 @@ ClientMessage and types for the proxy light client module communicating | |||
| with underlying Wasm light clients. | |||
| This implementation is based off the ICS 08 specification | |||
| (https://github.com/cosmos/ibc/blob/main/spec/client/ics-008-wasm-client) | |||
|
|
|||
| By default the 08-wasm module requires cgo and libwasmvm dependencies available on the system. | |||
| @@ -0,0 +1,44 @@ | |||
| //go:build !cgo || nolink_libwasmvm | |||
There was a problem hiding this comment.
can rename these files to cgo & no_cgo again?
There was a problem hiding this comment.
I think no_vm is more representative of the purpose of these build tags
|
|
Will require bump of next wasmvm release. Merging! 🚀 |
* wip: messing with build options for wasm cgo * chore: mv type assertion to wasm_cgo with build flags * chore: mv make target to build section * chore: revert cgo enabled 0 build opt for testing * chore: rm unneeded file * update build tag * linter * update panic message * add Codec back * refactor: adapt build tags to match wasmvm * Update modules/light-clients/08-wasm/doc.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update modules/light-clients/08-wasm/keeper/keeper_vm.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: make lint-fix * chore: make lint-fix --------- Co-authored-by: Charly <[email protected]> Co-authored-by: Charly <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Cian Hatton <[email protected]> (cherry picked from commit 031c2b8) # Conflicts: # Makefile # modules/light-clients/08-wasm/keeper/keeper.go
…ort #5923) (#6548) * imp: adding 08-wasm build opts with libwasmvm linking disabled (#5923) * wip: messing with build options for wasm cgo * chore: mv type assertion to wasm_cgo with build flags * chore: mv make target to build section * chore: revert cgo enabled 0 build opt for testing * chore: rm unneeded file * update build tag * linter * update panic message * add Codec back * refactor: adapt build tags to match wasmvm * Update modules/light-clients/08-wasm/doc.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update modules/light-clients/08-wasm/keeper/keeper_vm.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: make lint-fix * chore: make lint-fix --------- Co-authored-by: Charly <[email protected]> Co-authored-by: Charly <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Cian Hatton <[email protected]> (cherry picked from commit 031c2b8) # Conflicts: # Makefile # modules/light-clients/08-wasm/keeper/keeper.go * chore: resolve merge conflicts from backport * chore: update changelog --------- Co-authored-by: Damian Nolan <[email protected]>
Description
Tested adding custom build directive
nolink_libwasmvmin rollkit app binary compilation.See: chatton/gm-demo#2 (with successful build discarding libwasmvm dependencies - NOTE: uses fork of wasmvm with build directives)
When the application binary is compiled removing the calls to
wasmkeeper.NewKeeperWithVMandwasmkeeper.NewKeeperWithConfigthe docker image can be run and the entrypoint executes successfully returning thesimdhelp text.closes: #5903
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
libwasmvm.Refactor
Chores
helptarget output.