Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Integrate wasmcli into wasmd #312

Merged
merged 1 commit into from
Nov 17, 2020
Merged

Integrate wasmcli into wasmd #312

merged 1 commit into from
Nov 17, 2020

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Nov 13, 2020 •
edited
Loading

Resolves #308

Good bye to wasmcli, coral, gaiaflex clients

@codecov
Copy link

codecov bot commented Nov 13, 2020 •
edited
Loading

Codecov Report

Merging #312 (ca040da) into master (93761ea) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #312   +/-   ##
=======================================
  Coverage   17.53%   17.53%           
=======================================
  Files          35       35           
  Lines       10519    10519           
=======================================
  Hits         1844     1844           
  Misses       8572     8572           
  Partials      103      103
Impacted Files Coverage Δ
app/app.go 88.38% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93761ea...ca040da. Read the comment docs.

@alpe alpe marked this pull request as ready for review November 16, 2020 13:39
@ethanfrey
Copy link
Member

I had one thought on this last night.

wasmd requires the lib_wasmvm.so runtime. That is only built for Linux and OSX. Meaning, we cannot compile wasmd for Windows. If we completely remove wasmcli, we will not have a windows based cli now.

I see two options here:

  • Present cosmjs as the only client interface for Windows
  • Maintain wasmcli for windows, while wasmd also works as an all-in-one toolkit in platforms that support it.

I lean toward the second. If so, we should test this (once) to make sure it works (Abel uses Windows), and support it only for Windows. Then again, maybe we just ask people to install WSL (Windows Subsystem for Linux), and the CLI tools would just work. (Does anyone running vanilla Windows without WSL actually use CLI tools??)

ethanfrey
ethanfrey approved these changes Nov 16, 2020
View reviewed changes
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

The merge looks good.

I wonder again about providing some WIndows CLI support. But unless anyone actually uses this without WSL, maybe we don't need to care.

@webmaster128 do you know if wasmer-reborn will compile on windows? (Which would mean we can eventually add full windows support for wasmd)

Makefile
@@ -54,7 +54,6 @@ build_tags_comma_sep := $(subst $(empty),$(comma),$(build_tags))

ldflags = -X github.com/cosmos/cosmos-sdk/version.Name=wasm \
-X github.com/cosmos/cosmos-sdk/version.AppName=wasmd \
-X github.com/CosmWasm/wasmd/cmd/wasmcli/version.ClientName=wasmcli \
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in the comment, maybe we don't remove the CLI stuff, just keep it for Windows?

cmd/wasmd/root.go Show resolved Hide resolved
contrib/local/02-contracts.sh
INIT="{\"verifier\":\"$(wasmcli keys show validator -a)\", \"beneficiary\":\"$(wasmcli keys show fred -a)\"}"
wasmcli tx wasm instantiate "$CODE_ID" "$INIT" --admin=$(wasmcli keys show validator -a) \
INIT="{\"verifier\":\"$(wasmd keys show validator -a)\", \"beneficiary\":\"$(wasmd keys show fred -a)\"}"
wasmd tx wasm instantiate "$CODE_ID" "$INIT" --admin=$(wasmd keys show validator -a) \
Copy link
Member

Choose a reason for hiding this comment

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

Yes, wasmd here everywhere, even if we keep the cli for windows

@alpe alpe mentioned this pull request Nov 17, 2020
Merged
Base automatically changed from go_cosmwasm_upgrade_309 to master November 17, 2020 11:53
@alpe alpe force-pushed the single_binary_308 branch from 3fbdbe3 to ca040da Compare November 17, 2020 11:55
@alpe alpe mentioned this pull request Nov 17, 2020
Closed
@webmaster128
Copy link
Member

@webmaster128 do you know if wasmer-reborn will compile on windows? (Which would mean we can eventually add full windows support for wasmd)

I will try to find out. Let's track this issue in CosmWasm/wasmvm#28. For the near future, we don't have Windows support.

@alpe alpe merged commit 06cd063 into master Nov 17, 2020
@alpe alpe deleted the single_binary_308 branch November 17, 2020 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanfrey ethanfrey ethanfrey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge wasmd and wasmcli into a single binary
3 participants
@alpe @ethanfrey @webmaster128