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.

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

New build system #210

Merged
merged 25 commits into from
Jun 19, 2024
Merged

New build system #210

merged 25 commits into from
Jun 19, 2024

Conversation

wojtek-coreum
Copy link
Collaborator

@wojtek-coreum wojtek-coreum commented Jun 11, 2024

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 Reviewable

@wojtek-coreum wojtek-coreum requested a review from a team as a code owner June 11, 2024 11:50
@wojtek-coreum wojtek-coreum requested review from masihyeganeh, dzmitryhil, miladz68 and ysv and removed request for a team June 11, 2024 11:50
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @wojtek-coreum, and @ysv)


Makefile line 27 at r1 (raw file):

.PHONY: znet-start
znet-start:
	$(BUILDER) znet start --profiles=bridge-xrpl

I think it makes sense to allow profiles to be passed as arg

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 12 of 16 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


Makefile line 27 at r1 (raw file):

Previously, miladz68 (milad) wrote…

I think it makes sense to allow profiles to be passed as arg

makefiles don't support args, only variables which is horrible

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

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


relayer/cmd/cli/cli.go line 30 at r4 (raw file):

)

//go:generate ../../../bin/mockgen -destination=cli_mocks_test.go -package=cli_test . BridgeClient,Runner

Can we add mockgen to the PATH in the makefile and use just mockgen here?

Copy link
Contributor

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 16 files at r1, 1 of 9 files at r2, 24 of 25 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


build/go.mod line 40 at r4 (raw file):

require (
	cloud.google.com/go v0.112.0 // indirect

Was the project using these dependencies before?
Or this much dependencies are added just because of new build system?

miladz68
miladz68 previously approved these changes Jun 14, 2024
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 9 files at r2, 24 of 25 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wojtek-coreum and @ysv)

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @masihyeganeh, and @ysv)


build/go.mod line 40 at r4 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Was the project using these dependencies before?
Or this much dependencies are added just because of new build system?

I guess it's because of imported coreum builder


relayer/cmd/cli/cli.go line 30 at r4 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Can we add mockgen to the PATH in the makefile and use just mockgen here?

Good idea! Implemented here: https://reviewable.io/reviews/CoreumFoundation/crust/416

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 16 files at r1, 1 of 9 files at r2, 24 of 25 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @wojtek-coreum, and @ysv)


relayer/cmd/cli/cli.go line 30 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Good idea! Implemented here: https://reviewable.io/reviews/CoreumFoundation/crust/416

The reference PR is approved. Waiting the changes here.

Copy link
Contributor

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

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


build/go.mod line 40 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I guess it's because of imported coreum builder

So, now we are downloading and building 100 more dependencies for no good reason?

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh and @ysv)


build/go.mod line 40 at r4 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

So, now we are downloading and building 100 more dependencies for no good reason?

Be did it before anyway because cored has been built anyway - just by the other builder

masihyeganeh
masihyeganeh previously approved these changes Jun 14, 2024
Copy link
Contributor

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

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


build/go.mod line 40 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Be did it before anyway because cored has been built anyway - just by the other builder

How about binary size? can you compare it between two versions?
I marked my messages as non-blocking, but I'm not satisfied with this change

@wojtek-coreum wojtek-coreum dismissed stale reviews from masihyeganeh and miladz68 via 2c6575f June 14, 2024 11:03
Copy link
Contributor

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wojtek-coreum and @ysv)

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

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


relayer/cmd/cli/cli.go line 30 at r4 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The reference PR is approved. Waiting the changes here.

Done.

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @masihyeganeh, and @ysv)


build/go.mod line 40 at r4 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

How about binary size? can you compare it between two versions?
I marked my messages as non-blocking, but I'm not satisfied with this change

but why do we care? The total size is reduced for sure, because before we used two binaries, now we use one.

masihyeganeh
masihyeganeh previously approved these changes Jun 14, 2024
Copy link
Contributor

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil and @ysv)

miladz68
miladz68 previously approved these changes Jun 14, 2024
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 9 files at r7.
Reviewable status: 33 of 41 files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @miladz68, and @ysv)

dzmitryhil
dzmitryhil previously approved these changes Jun 17, 2024
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 33 of 41 files reviewed, all discussions resolved (waiting on @masihyeganeh, @miladz68, and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 7 files at r5, 6 of 9 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh, @miladz68, and @ysv)

Copy link
Contributor

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 9 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @wojtek-coreum, and @ysv)


bin/coreumbridge-xrpl-builder line 16 at r8 (raw file):


  pushd build > /dev/null
  GOVCS='public:git|hg|bzr' go build -trimpath -o "$BUILD_BIN" ./cmd/builder

Do all our users still need to have the bazaar client?

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @miladz68, and @ysv)


bin/coreumbridge-xrpl-builder line 16 at r8 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Do all our users still need to have the bazaar client?

Yes

@wojtek-coreum wojtek-coreum merged commit 6ee6d01 into master Jun 19, 2024
6 of 7 checks passed
@wojtek-coreum wojtek-coreum deleted the wojtek/ci branch June 19, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants