refactor: remove unnecessary deps from 08-wasm by reusing top level simapp binary #7445

colin-axner · 2024-10-14T09:47:35Z

Description

The general idea:

top level simapp should contain all our modules and support a binary for all e2e tests
adds wasm to top level simapp, along with build requirements in workflow + Dockerfile
left 08-wasm Docker image for release building (instead of adding more conditionals into the workflows for the switch over point)
e2e's run off CI should use the simd image, running against older branches requires using existing release tag, or building new image via manual wasm build
left wasm e2e workflow to prevent wasm e2e test from being run on every pr

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.

colin-axner · 2024-10-14T09:51:24Z

simapp/app.go

@@ -569,6 +614,7 @@ func NewSimApp(
 mockModule,

 // IBC light clients
+ wasm.NewAppModule(app.WasmClientKeeper), // TODO(damian): see if we want to pass the lightclient module here, keeper is used in AppModule.RegisterServices etc


copied this over, is this todo still relevant? I'm not sure I see how the light client module could be passed?

…osmos/ibc-go into colin/7231-rm-wasm-callbacks-bin-logic

colin-axner · 2024-10-14T13:39:25Z

.github/workflows/build-simd-image-from-tag.yml

should I try to remove all the custom wasm workflows? I feel like now that we have a single place for a simapp, we should just be building a single simd. What do people think?

yea! would be fine in follow up if you'd prefer.

colin-axner · 2024-10-14T13:39:54Z

.github/workflows/docker.yml

would be nice if someone could double check all the workflow changes, especially release.yml

should also add the install python step here? (unless I am not seeing something)

colin-axner · 2024-10-14T13:40:26Z

.github/workflows/build-simd-image-from-tag.yml

+ - uses: actions/setup-python@v5
+ with:
+ python-version: '3.10'
+ - name: Install dependencies


I guess this is necessary to use the script to get the libwasm version and checksum

colin-axner · 2024-10-14T13:40:44Z

modules/light-clients/08-wasm/Dockerfile

should I simply delete this file now?

can do with https://github.com/cosmos/ibc-go/pull/7445/files#r1799530332

colin-axner · 2024-10-14T13:41:22Z

Dockerfile

@@ -24,7 +35,7 @@ COPY go.sum .

 RUN go mod download

-RUN make build
+RUN cd simapp && GOOS=linux GOARCH=amd64 go build -mod=readonly -tags "netgo ledger muslc" -ldflags '-X github.com/cosmos/cosmos-sdk/version.Name=sim -X github.com/cosmos/cosmos-sdk/version.AppName=simd -X github.com/cosmos/cosmos-sdk/version.Version= -X github.com/cosmos/cosmos-sdk/version.Commit= -X "github.com/cosmos/cosmos-sdk/version.BuildTags=netgo ledger muslc," -w -s -linkmode=external -extldflags "-Wl,-z,muldefs -static"' -trimpath -o /go/build/ ./...


It looks like -linkmode=external -extldflags "-Wl,-z,muldefs -static" is not usually included when we run make build. Should I try to include these changes there as well? Folks okay if I hardcode it for now?

did a quick peek into changing this, but ran into a little difficulties, so I'll let someone be my guest if they want to adjust it

can open an issue but personally fine with hardcoding it for now, seems like a nice to have someone can pick up in future.

DimitrisJim

left some initial comments, loving the direction these changes are headed in

DimitrisJim · 2024-10-14T14:04:11Z

Dockerfile

@@ -24,7 +35,7 @@ COPY go.sum .

 RUN go mod download

-RUN make build
+RUN cd simapp && GOOS=linux GOARCH=amd64 go build -mod=readonly -tags "netgo ledger muslc" -ldflags '-X github.com/cosmos/cosmos-sdk/version.Name=sim -X github.com/cosmos/cosmos-sdk/version.AppName=simd -X github.com/cosmos/cosmos-sdk/version.Version= -X github.com/cosmos/cosmos-sdk/version.Commit= -X "github.com/cosmos/cosmos-sdk/version.BuildTags=netgo ledger muslc," -w -s -linkmode=external -extldflags "-Wl,-z,muldefs -static"' -trimpath -o /go/build/ ./...


can open an issue but personally fine with hardcoding it for now, seems like a nice to have someone can pick up in future.

DimitrisJim · 2024-10-14T15:02:11Z

.github/workflows/docker.yml

should also add the install python step here? (unless I am not seeing something)

DimitrisJim · 2024-10-14T15:09:43Z

.github/workflows/build-simd-image-from-tag.yml

yea! would be fine in follow up if you'd prefer.

DimitrisJim · 2024-10-14T15:10:01Z

modules/light-clients/08-wasm/Dockerfile

can do with https://github.com/cosmos/ibc-go/pull/7445/files#r1799530332

DimitrisJim · 2024-10-14T15:21:44Z

modules/light-clients/08-wasm/testing/simapp/simd/main.go

-
-func main() {
- rootCmd := cmd.NewRootCmd()
- if err := svrcmd.Execute(rootCmd, "", simapp.DefaultNodeHome); err != nil {


shouldn't this stay in until we remove all references to the wasm dockerfile? (Think it is still referenced in some fork workflows)

I like that direction. I can split this work up:

add wasm into top level simapp

switch over wasm workflows to main workflow (I think there's some consideration around image building off release branches)

remove simd in wasm directory + Dockerfile (removes unnecessary deps)

I went through the reference in the fork workflow and it looks like running wasm tests on forks is currently disabled

colin-axner · 2024-10-16T11:11:24Z

modules/light-clients/08-wasm/go.mod

@@ -9,18 +9,12 @@ replace github.com/cosmos/ibc-go/v9 => ../../../
 replace github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7

 require (
- cosmossdk.io/api v0.7.6


damn, 7 dependencies 🪓

…sm-callbacks-bin-logic

refactor: move 08-wasm to use top level simapp for binary

8445958

colin-axner marked this pull request as ready for review October 14, 2024 09:47

colin-axner requested review from AdityaSripal, damiannolan, chatton, DimitrisJim, srdtrk, gjermundgaraba and bznein as code owners October 14, 2024 09:47

colin-axner marked this pull request as draft October 14, 2024 09:47

colin-axner commented Oct 14, 2024

View reviewed changes

refactor: add wasm build info to top level docker build file

0fbe2f1

colin-axner marked this pull request as ready for review October 14, 2024 09:56

colin-axner marked this pull request as draft October 14, 2024 09:56

fix: update workflow build file

117f1ed

colin-axner marked this pull request as ready for review October 14, 2024 10:04

colin-axner marked this pull request as draft October 14, 2024 10:04

chore: update workflow files to include wasm args

8330d9d

colin-axner marked this pull request as ready for review October 14, 2024 10:15

colin-axner marked this pull request as draft October 14, 2024 10:15

fix: test docker build with libwasm args

7312fac

colin-axner marked this pull request as ready for review October 14, 2024 10:23

colin-axner marked this pull request as draft October 14, 2024 10:23

fix: small adjustments to workflow file

71fb1b1

colin-axner marked this pull request as ready for review October 14, 2024 10:28

colin-axner marked this pull request as draft October 14, 2024 10:28

fix? dockerfile

b4c7258

colin-axner marked this pull request as ready for review October 14, 2024 10:41

colin-axner marked this pull request as draft October 14, 2024 10:41

chore: slight adjustment to workflow

ebfa82a

Merge branch 'main' into colin/7231-rm-wasm-callbacks-bin-logic

e5616d9

colin-axner marked this pull request as ready for review October 14, 2024 12:52

colin-axner marked this pull request as draft October 14, 2024 12:52

colin-axner added 2 commits October 14, 2024 15:37

fix: dockerfile

30ca97b

Merge branch 'colin/7231-rm-wasm-callbacks-bin-logic' of github.com:c…

4ddb0fb

…osmos/ibc-go into colin/7231-rm-wasm-callbacks-bin-logic

colin-axner marked this pull request as ready for review October 14, 2024 13:38

colin-axner marked this pull request as draft October 14, 2024 13:38

colin-axner commented Oct 14, 2024

View reviewed changes

colin-axner marked this pull request as ready for review October 14, 2024 13:52

DimitrisJim reviewed Oct 14, 2024

View reviewed changes

colin-axner marked this pull request as draft October 15, 2024 08:10

colin-axner mentioned this pull request Oct 16, 2024

imp: add 08-wasm to top level simapp #7471

Closed

10 tasks

colin-axner added 2 commits October 16, 2024 13:07

chore: refactor workflows

32184df

refactor: remove more unnecessary deps

af4fe10

colin-axner commented Oct 16, 2024

View reviewed changes

colin-axner marked this pull request as ready for review October 16, 2024 11:14

colin-axner marked this pull request as draft October 16, 2024 11:21

Merge branch 'main' of github.com:cosmos/ibc-go into colin/7231-rm-wa…

1cd2408

…sm-callbacks-bin-logic

colin-axner marked this pull request as ready for review October 17, 2024 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove unnecessary deps from 08-wasm by reusing top level simapp binary #7445

refactor: remove unnecessary deps from 08-wasm by reusing top level simapp binary #7445

colin-axner commented Oct 14, 2024 •

edited

Loading

colin-axner Oct 14, 2024

colin-axner Oct 14, 2024

DimitrisJim Oct 14, 2024

colin-axner Oct 14, 2024

DimitrisJim Oct 14, 2024

colin-axner Oct 14, 2024

colin-axner Oct 14, 2024

DimitrisJim Oct 14, 2024

colin-axner Oct 14, 2024

colin-axner Oct 14, 2024

DimitrisJim Oct 14, 2024

DimitrisJim left a comment

DimitrisJim Oct 14, 2024

DimitrisJim Oct 14, 2024

DimitrisJim Oct 14, 2024

DimitrisJim Oct 14, 2024

DimitrisJim Oct 14, 2024

colin-axner Oct 15, 2024

colin-axner Oct 17, 2024

colin-axner Oct 16, 2024

refactor: remove unnecessary deps from 08-wasm by reusing top level simapp binary #7445

Are you sure you want to change the base?

refactor: remove unnecessary deps from 08-wasm by reusing top level simapp binary #7445

Conversation

colin-axner commented Oct 14, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Oct 14, 2024 •

edited

Loading