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

multi: migrate protoc to buf #350

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

shelter2759
Copy link
Contributor

@shelter2759 shelter2759 commented Feb 6, 2025

Issue number: resolves #343


What is the current behavior?

  • In the peerswaprpc folder, the code is generated by protoc.

What is the new behavior?

  • Introduce buf and migrate from protoc.
  • Modify devcontainer.json to install buf.
  • Create buf.yaml and buf.gen.yaml.
  • Modify makefile and Dockerfile that use protoc command to buf command.
  • Delete unnecessary gen_protos.sh files
  • Generate code in peerswaprpc folder using buf.

Other information

@shelter2759 shelter2759 changed the title multi: migrate protoc to buf WIP:multi: migrate protoc to buf Feb 6, 2025
@shelter2759 shelter2759 changed the title WIP:multi: migrate protoc to buf [WIP]multi: migrate protoc to buf Feb 6, 2025
@shelter2759 shelter2759 marked this pull request as ready for review February 6, 2025 14:34
@shelter2759 shelter2759 changed the title [WIP]multi: migrate protoc to buf multi: migrate protoc to buf Feb 6, 2025
@YusukeShimizu
Copy link
Contributor

YusukeShimizu commented Feb 7, 2025

Thank you!
I really appreciate your effort in catching up with the insufficient documentation and helping out.
I ran the CI tests and the build is failing, so could you please take a look?

Up to now, we’ve been able to generate the proto files with the following commands:

cd peerswaprpc
make all-rpc-docker

For this PR, how do you plan to generate the proto files? If possible, it would be great if you could add instructions to the documentation.

From what I’ve observed, the build failure appears to be due to a mismatch between the version of the generated gRPC code and the gRPC library we’re using. I see two potential approaches to address this:

Approach 1: Update the library versions

Updating the Go dependencies (e.g. google.golang.org/grpc) to v1.64.0 or later should align with the generated code, thus resolving the error.
(Reference: #297 addresses dependency updates, so possibly incorporating that would fix the issue.)

Approach 2: Pin the version to match the existing environment

This involves fixing the version of the buf plugin. By using the existing make all-rpc-docker process to install a specific version of the protoc-gen tool, we should be able to generate proto files compatible with the current setup.
In this PR, it might help to first generate code with the fixed version (matching the existing environment) to ensure the build passes.

@shelter2759
Copy link
Contributor Author

@YusukeShimizu
Thank you for suggesting the approach.
I have adopted Approach 2 that you proposed and modified the Makefile and Dockerfile to generate files using buf with the same commands as the existing ones.
0bd9afa

I want to run build tests in GitHub Actions. Do I need to wait until it runs automatically or is there any other way to run it?

I’m not yet familiar with the rules of this project, so I apologize for asking so many beginner questions. I don’t mean to cause trouble, but I would greatly appreciate your response.

@YusukeShimizu
Copy link
Contributor

In this repository, the Approval for running fork pull request workflows from contributors setting is configured as Require approval for first-time contributors. Only users who have never had a commit or pull request merged into this repository will require approval to run workflows..

It appears that the action was not triggered for commit 0bd9afa . The cause is unclear, but could you please resolve the conflicts and push again?

@shelter2759
Copy link
Contributor Author

@YusukeShimizu
Apologies for the force push.
I have reverted the deletion of gen_proto.sh, which was the cause of the conflict, and kept the file.

Thank you for your understanding. Please let me know if there’s anything else I should address.

@YusukeShimizu
Copy link
Contributor

Thank you for not only migrating to buf but also thoughtfully considering the .devcontainer setup.
It seems stable at this point, and I think that’s great.

In the future, if we can incorporate buf into ‘nix develop’ and reduce our reliance on Docker, we can further lower the barrier to setting things up.

@YusukeShimizu
Copy link
Contributor

This change is an update to the development experience and is not expected to affect the behavior, so I will merge it.

@YusukeShimizu YusukeShimizu merged commit f9348d1 into ElementsProject:master Feb 8, 2025
8 checks passed
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.

Introduce Buf for Proto Generation
2 participants