CosmWasm / wasmvm

Go bindings to the running cosmwasm contracts with wasmer
Apache License 2.0
173 stars 99 forks source link

Add no-cgo support #377

Closed webmaster128 closed 1 year ago

webmaster128 commented 1 year ago

This is a follow-up of #373. It implements the same ideas, but with the following additions:

jhernandezb commented 1 year ago

I only get 1 issue now

# github.com/CosmWasm/wasmd/x/wasm/keeper
../../../go/pkg/mod/github.com/!cosm!wasm/wasmd@v0.29.2/x/wasm/keeper/keeper.go:128:24: undefined: cosmwasm.NewVM
webmaster128 commented 1 year ago

Thank you for checking this, @jhernandezb

keeper/keeper.go:128:24: undefined: cosmwasm.NewVM

This is expected IMO. The moment wasmvm is built without cgo it cannot provide functionality that requires cgo. Creating a VM instance is not possible. So I assume if wasmd also wants to provide a no-cgo build it has to deactivate such calls in wasmd.

jhernandezb commented 1 year ago

I guess that there should be a keeper.go for cgo and non-cgo at least the NewKeeper() method

jhernandezb commented 1 year ago

I got it working with these changes https://github.com/public-awesome/wasmd/commit/cffe53fa0eeee0a9735ac2dd6d419d9d1d25eccf

I think the LibwasmvmVersion() can actually be fixed from this repo and it will essentially be just the keeper change.

webmaster128 commented 1 year ago

I think the LibwasmvmVersion() can actually be fixed from this repo

Can it? LibwasmvmVersion() calls into the libwasmvm at runtime to get its version via C/cgo.

jhernandezb commented 1 year ago

It could error for non_cgo but maybe it's something consumers would not expect?

Consumers could also handle the cgo/non cgo build as I did in my branch.

webmaster128 commented 1 year ago

It could error for non_cgo but maybe it's something consumers would not expect?

It would be a bit inconsistent to return an error. In theory all the other functions should then also return an error instead of being unavailable. I think we should not mix

But one thing that we could do is make "none" success output option. Then "none" means no version of libwasmvm was loaded and any other version is coming from the library. Looking at how this is used it might be a nice way to reduce the burden on the wasmd codebase.

webmaster128 commented 1 year ago

Hmm, creating a "none" output is also adding a new case. Looking at the wasmd diff again I think it is very bad having to duplicate the whole CLI command file just because you don't want to compile one line. Since you want an error there, I think the most pragmatic solution is is to have an erroring implementation here.

faddat commented 1 year ago

Hey guys, this is amazing.

Thank you!