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

Move building system from crust #757

Merged
merged 11 commits into from
Jan 25, 2024
Merged

Move building system from crust #757

merged 11 commits into from
Jan 25, 2024

Conversation

wojtek-coreum
Copy link
Collaborator

@wojtek-coreum wojtek-coreum commented Dec 22, 2023

Description

Files in build/coreum are simply copied from crust/build/coreum.
Other files in this PR are copied and adjusted.

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 December 22, 2023 08:46
@wojtek-coreum wojtek-coreum requested review from dzmitryhil, miladz68 and ysv and removed request for a team December 22, 2023 08:46
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a205eca) 33.04% compared to head (2ecd2e3) 33.04%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #757   +/-   ##
=======================================
  Coverage   33.04%   33.04%           
=======================================
  Files         170      170           
  Lines       48255    48255           
=======================================
  Hits        15948    15948           
  Misses      29278    29278           
  Partials     3029     3029           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ysv ysv 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: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

a discussion (no related file):
I have a few general questions which would be nice to clarify before we move forward e.g:

  1. how and by which component (crust/coreum) build dependencies are installed and managed (golang, rust etc) ?
  2. what if e.g we have golang 1.20 in faucet and 1.21 in coreum ?
  3. where do we store linter config ?

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: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @miladz68 and @ysv)

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

I have a few general questions which would be nice to clarify before we move forward e.g:

  1. how and by which component (crust/coreum) build dependencies are installed and managed (golang, rust etc) ?
  2. what if e.g we have golang 1.20 in faucet and 1.21 in coreum ?
  3. where do we store linter config ?

And one more question from me:
4. Is it OK that coreum depends on the crust or it should be a different repo?


Copy link
Contributor

@ysv ysv 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: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @miladz68)

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: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @miladz68 and @ysv)

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

And one more question from me:
4. Is it OK that coreum depends on the crust or it should be a different repo?

  1. Script bin/coreum in coreum repo does everything. But it delegates tasks to code imported from crust. If tool is needed it is installed automatically using the version from the imported crust.
  2. https://reviewable.io/reviews/CoreumFoundation/crust/339#- solves this by keeping dependencies versioned by the version of imported crust
  3. It is stored in the binary and saved under versioned directory

@dzmitryhil dzmitryhil requested a review from ysv January 16, 2024 10:01
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 24 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)


bin/coreum line 1 at r3 (raw file):

#!/bin/bash

so all build commands go through this wrapper similar to the way it works in crust ?
Maybe, to be more explicit it should be named smth like cored-builder or coreum-builder or build instead of builder ?


build/index.go line 18 at r3 (raw file):

	"build/integration-tests/upgrade": coreum.BuildIntegrationTests(coreum.TestUpgrade),
	"generate":                        coreum.Generate,
	"images":                          coreum.BuildCoredDockerImage,

nit: I'm not sure if images in plural is applicable here since you build a single one

Copy link
Contributor

@ysv ysv 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, 3 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)

a discussion (no related file):
@wojtek-coreum could you pls create an umbrella task in backlog with description of all steps needed to finalize this.
Pls don't forget that we also want to have Makefile in Coreum


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, 3 unresolved discussions (waiting on @miladz68 and @ysv)


build/index.go line 18 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: I'm not sure if images in plural is applicable here since you build a single one

I want to avid situation where in one repo you have image and in another images. Command images builds all the available images, single image is just an edge case.

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, 3 unresolved discussions (waiting on @miladz68 and @ysv)


bin/coreum line 1 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

so all build commands go through this wrapper similar to the way it works in crust ?
Maybe, to be more explicit it should be named smth like cored-builder or coreum-builder or build instead of builder ?

Done.

Copy link
Contributor

@ysv ysv 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 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)

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 @miladz68 and @ysv)

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

@wojtek-coreum could you pls create an umbrella task in backlog with description of all steps needed to finalize this.
Pls don't forget that we also want to have Makefile in Coreum

https://app.clickup.com/t/86873z34d


Copy link
Contributor

@ysv ysv 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

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.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

@wojtek-coreum wojtek-coreum merged commit 6129c3e into master Jan 25, 2024
10 checks passed
@wojtek-coreum wojtek-coreum deleted the wojtek/build branch January 25, 2024 07:24
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.

3 participants