Build system

[wojtek-coreum]

Description

Reviewers checklist:

• Try to write more meaningful comments with clear actions to be taken.
• Nit-picking should be unblocking. Focus on core issues.

Authors checklist

• Provide a concise and meaningful description
• Review the code yourself first, before making the PR.
• Annotate your PR in places that require explanation.
• Think and try to split the PR to smaller PR if it is big.

---

This change is 

[ysv]

Reviewed 27 of 27 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

---

build/go.mod line 7 at r1 (raw file):

require (
	github.com/CoreumFoundation/coreum-tools v0.4.1-0.20240223065112-97400ae15d8e
	github.com/CoreumFoundation/crust/build v0.0.0-20240223131002-860a2548b628

Could you upgrade to this version of crust or newer in scope of this PR ?
ef2b09e2d127bed100fafb4bbe7edebb8e101c79

here is an example of build changes needed in case you do this in current PR: https://github.com/CoreumFoundation/faucet/pull/78/files

or if you prefer I can do it separately myself because it is in scope of my task

---

build/bridge/image/template.go line 1 at r1 (raw file):

package image

this code is very similar to the one in coreum.

shall we add a todo & try to move it somewhere ?

[dzmitryhil]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)

---

Makefile line 56 at r1 (raw file):

      cosmwasm/optimizer:0.15.0
	mkdir -p $(BUILD_DIR)
	cp $(CONTRACT_DIR)/artifacts/coreumbridge_xrpl.wasm $(BUILD_DIR)/coreumbridge_xrpl.wasm

What about the makefile ? What will keep here ?

---

.github/workflows/relayer-ci.yml line 44 at r1 (raw file):

          - ci_step: "integration tests contract"
            command: |
              coreumbridge-xrpl-builder build/contract build/integration-tests/contract images

To let the cache work correcly we need single build for all steps even if the part of the build result is not used by one of the step.

---

.github/workflows/relayer-ci.yml line 84 at r1 (raw file):

          repository: CoreumFoundation/crust
          path: crust
          # FIXME (wojciech): change to master once https://reviewable.io/reviews/CoreumFoundation/crust/365

We don't need to cahnge to master we need to change to correct commit hash or tag.

---

.github/workflows/relayer-ci.yml line 127 at r1 (raw file):

        with:
          path: ~/.cache/golangci-lint
          key: ${{ runner.os }}-linter-cache-2-${{ steps.goversion.outputs.GO_VERSION }}

Check the CI in master, it's different now.

---

build/index.go line 23 at r1 (raw file):

	"build/relayer":                     bridge.BuildRelayer,
	"build/contract":                    bridge.BuildSmartContract,
	"build/integration-tests":           bridge.BuildAllIntegrationTests,

Do we really need to build tests ? How about simply run all tests from the folders, similar to what we do in the makefile ?

---

build/bridge/contract.go line 31 at r1 (raw file):

// CompileSmartContract returns function compiling smart contract.
func CompileSmartContract(codeDirPath string) build.CommandFunc {

But you already use the rust.BuildSmartContact, why not to move it now?

[wojtek-coreum]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dzmitryhil and @miladz68)

---

.github/workflows/relayer-ci.yml line 84 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

We don't need to cahnge to master we need to change to correct commit hash or tag.

I mean changing to latest hash in master

---

build/index.go line 23 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Do we really need to build tests ? How about simply run all tests from the folders, similar to what we do in the makefile ?

This PR is about using in this repo the same build system we use everywhere. Changing the way tests are run are out of scope and might be done separately

[wojtek-coreum]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dzmitryhil and @miladz68)

---

Makefile line 56 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about the makefile ? What will keep here ?

Makefile will be upgraded once we agree on the builder

[dzmitryhil]

Reviewable status: 19 of 27 files reviewed, 7 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

---

.github/workflows/relayer-ci.yml line 44 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

To let the cache work correcly we need single build for all steps even if the part of the build result is not used by one of the step.

This part is still not addressed.

---

.github/workflows/relayer-ci.yml line 25 at r2 (raw file):

          - ci_step: "lint"
            command: "coreumbridge-xrpl-builder lint"
            go-cache: true

Why do you need the go-cache flag if it always true ?

---

.github/workflows/relayer-ci.yml line 120 at r2 (raw file):

          path: |
            ~/.cache/crust/wasm
            !/.cache/crust/wasm/code-hashes.json

We don't need to cache the wasm since we cache the crust fully.

---

build/index.go line 24 at r2 (raw file):

	"build/contract":                    bridge.BuildSmartContract,
	"build/integration-tests":           bridge.BuildAllIntegrationTests,
	"build/integration-tests/xrpl":      bridge.BuildIntegrationTests(bridge.TestXRPL),

Taking into account that we build everything in the CI, IMO we don't need separate build for each test backage.

---

build/bridge/image/Dockerfile.tmpl line 1 at r2 (raw file):

FROM --platform=$TARGETPLATFORM {{ .From }}

What do we do with the Dockerfile which is in the relayer root? If you plan to remove it later let's have a task or FIXME to do it.

[wojtek-coreum]

Reviewable status: 19 of 27 files reviewed, 7 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

---

.github/workflows/relayer-ci.yml line 25 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you need the go-cache flag if it always true ?

Done.

---

.github/workflows/relayer-ci.yml line 120 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

We don't need to cache the wasm since we cache the crust fully.

this line excludes the file from cache

---

build/bridge/contract.go line 31 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But you already use the rust.BuildSmartContact, why not to move it now?

Done.

---

build/bridge/image/Dockerfile.tmpl line 1 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What do we do with the Dockerfile which is in the relayer root? If you plan to remove it later let's have a task or FIXME to do it.

it will be removed when I update the Makefile

[dzmitryhil]

Reviewable status: 13 of 27 files reviewed, 3 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

---

.github/workflows/relayer-ci.yml line 120 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

this line excludes the file from cache

This part? So probably it should be in the crust cache then?

[ysv]

Reviewed 8 of 8 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)

[ysv]

Reviewed 15 of 15 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)

[wojtek-coreum]

Reviewable status: 21 of 29 files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

---

.github/workflows/relayer-ci.yml line 44 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

This part is still not addressed.

Now we download all the dependencies before forking jobs, so cache should work

---

build/index.go line 24 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Taking into account that we build everything in the CI, IMO we don't need separate build for each test backage.

Done.

[ysv]

Reviewed 6 of 8 files at r5, 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

[dzmitryhil]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)

---

.github/workflows/relayer-ci.yml line 34 at r6 (raw file):

          - ci_step: "integration tests contract"
            command: |
              coreumbridge-xrpl-builder build/contract images

Let's move the call of the all builders and znet to makefile instead. We already have build-dev-env command which does exactly this

---

.github/workflows/relayer-ci.yml line 36 at r6 (raw file):

              coreumbridge-xrpl-builder build/contract images
              coreum-builder build images
              crust znet start --profiles=1cored,xrpl --timeout-commit 0.5s

The shat can be moved to the makefile as well

---

.github/workflows/relayer-ci.yml line 37 at r6 (raw file):

              coreum-builder build images
              crust znet start --profiles=1cored,xrpl --timeout-commit 0.5s
              coreumbridge-xrpl-builder integration-tests/contract

Why do you start and build later?

---

build/index.go line 27 at r6 (raw file):

	"generate":          {Fn: bridge.Generate, Description: "Generates artifacts"},
	"images":            {Fn: bridge.BuildRelayerDockerImage, Description: "Builds relayer docker image"},
	"integration-tests": {Fn: bridge.RunAllIntegrationTests, Description: "Runs integration tests"},

Haven't we changed the build to run?

---

build/index.go line 36 at r6 (raw file):

	"lint":           {Fn: bridge.Lint, Description: "lints code"},
	"release":        {Fn: bridge.ReleaseRelayer, Description: "Releases relayer binary"},
	"release/images": {Fn: bridge.ReleaseRelayerImage, Description: "Releases relayer docker image"},

Do you use it in the github CI ? IF not let's add at least task, otherwise it useless.

[wojtek-coreum]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil and @miladz68)

---

.github/workflows/relayer-ci.yml line 34 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's move the call of the all builders and znet to makefile instead. We already have build-dev-env command which does exactly this

No, this is the makefile which will call the builders. And CI calls the builder too

---

.github/workflows/relayer-ci.yml line 37 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you start and build later?

znet starts the environment and this line runs integration tests on top of that environment. It was decided that integration tests should be run by each repo (not by the znet)

---

build/index.go line 27 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Haven't we changed the build to run?

Done.

---

build/index.go line 36 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Do you use it in the github CI ? IF not let's add at least task, otherwise it useless.

in next step e will call it from github action like we do for coreum

[dzmitryhil]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

---

.github/workflows/relayer-ci.yml line 34 at r6 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

No, this is the makefile which will call the builders. And CI calls the builder too

The build command here uses multiple sub-command, so to reduce code duplication, it make sense IMO to use makefile to simplify it. Or use additional build command to do it. Also the makefile can set default PATH to coreumbridge-xrpl-builder so you won't have to set it up manually.

@ysv @miladz68 WDYT ?

[ysv]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)

---

.github/workflows/relayer-ci.yml line 34 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The build command here uses multiple sub-command, so to reduce code duplication, it make sense IMO to use makefile to simplify it. Or use additional build command to do it. Also the makefile can set default PATH to coreumbridge-xrpl-builder so you won't have to set it up manually.

@ysv @miladz68 WDYT ?

yes, I agree makefile makes sense
In the milistone ticket we have task to add makefiles to probably all repos.
At least inside coreum it should be as we agree but IMO it makes sense to have it everywhere

For now I'm ok to either keep it like this or add makefile in this PR

[miladz68]

Reviewed 9 of 27 files at r1, 4 of 8 files at r2, 1 of 6 files at r3, 5 of 15 files at r4, 6 of 8 files at r5, 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil and @wojtek-coreum)

---

.github/workflows/relayer-ci.yml line 34 at r6 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

yes, I agree makefile makes sense
In the milistone ticket we have task to add makefiles to probably all repos.
At least inside coreum it should be as we agree but IMO it makes sense to have it everywhere

For now I'm ok to either keep it like this or add makefile in this PR

I agree that Makefile should be interface to interact with the builder.

[dzmitryhil]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

---

.github/workflows/relayer-ci.yml line 34 at r6 (raw file):

Previously, miladz68 (milad) wrote…

I agree that Makefile should be interface to interact with the builder.

Agreed to do it later.

[dzmitryhil]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

[miladz68]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

[ysv]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

[ysv]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)