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

build: add proto build system #95

Closed
wants to merge 16 commits into from

Conversation

tuantran1702
Copy link
Contributor

This pull request introduces a new protobuf build system and fixes existing formatting/linting errors of protobuf files.

Closes #90

Change

  • Add protobuf formatting, linting and generation command
  • Fix linting errors

@nghuyenthevinh2000
Copy link

Proto build working:

Screenshot 2023-12-18 at 11 53 01

A note on ghcr.io access denied:

  1. Create access token in profile developer settings for docker download
  2. echo "$ACCESS_TOKEN" | docker login ghcr.io -u USERNAME --password-stdin

More information on: https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-container-registry#authenticating-to-the-container-registry

@tuantran1702
Copy link
Contributor Author

tuantran1702 commented Dec 18, 2023

It seems that the failing tests are related to the unused modules(nft, marketplace and addressbook), and not related to the proto build process.
image

@nghuyenthevinh2000
Copy link

nghuyenthevinh2000 commented Dec 18, 2023

@nbaum @jgtormo I have reviewed this pr, can you have a look and merge it? Any questions related to how to re - run the proto build scripts and ci failing should be helpful

@nghuyenthevinh2000
Copy link

@tropicaldog please set this to ready

@tuantran1702 tuantran1702 marked this pull request as ready for review December 18, 2023 08:54
@jgtormo
Copy link
Member

jgtormo commented Dec 18, 2023

should we remove the nft, marketplace and addressbook modules before we merge this one in, to avoid failing tests?

@nghuyenthevinh2000
Copy link

nghuyenthevinh2000 commented Dec 19, 2023

should we remove the nft, marketplace and addressbook modules before we merge this one in, to avoid failing tests?

yes, let's just keep this pr here until nft, marketplace, and addrbook modules removed. @tropicaldog can you create another pr for this issue: #97

@nghuyenthevinh2000
Copy link

@jgtormo please review and merge this one

@tuantran1702 tuantran1702 marked this pull request as draft January 2, 2024 10:51
@tuantran1702 tuantran1702 reopened this Jan 3, 2024
@tuantran1702
Copy link
Contributor Author

After some discussion, it seems that this PR doesn't support format and linting v0.45.x version of cosmos-sdk, so I will close this PR now.

This proto build system just doesn't work well with the x/group added directly to the cudos-node(as part of unforking process).

@jgtormo please confirm if we can temporarily disable the x/group . Since the x/group is be available in SDK v0.46 and later, we can use it again when the upgrade to 0.47.x is complete.

@nghuyenthevinh2000 nghuyenthevinh2000 mentioned this pull request Jan 4, 2024
14 tasks
@tuantran1702 tuantran1702 marked this pull request as ready for review January 4, 2024 11:03
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.

Add a proto build system
3 participants