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

fix: protobuf generation process #800

Closed
guillaumemichel opened this issue Jan 21, 2025 · 2 comments
Closed

fix: protobuf generation process #800

guillaumemichel opened this issue Jan 21, 2025 · 2 comments
Labels
dif/easy Someone with a little familiarity can pick up effort/hours Estimated to take one or several hours kind/maintenance Work required to avoid breaking changes or harm to project's status quo P2 Medium: Good to have, but can wait until someone steps up

Comments

@guillaumemichel
Copy link
Contributor

Follow up to #794

The protobuf code (.pb.go) has to be generated only once after a change in the .proto file, then the .pb.go file is pushed to the repo and nodes never need to generate. Even when not depending on make the protobuf code still has a dependency on protoc for go generate ./... to work.

IMO it is better that the protobuf code gets updated only after a conscious decision from the person modifying the .proto file. Being able to overwrite all .pb.go files using go generate ./... at the root of the repo could have unintended side effects, such as changing the protoc version with which the code was generated, depending on the version that is installed on the machine running go generate ./....

It may possible to avoid non-go dependencies (make, protoc) by depending directly on protobuf-go, but then we would need a dedicated package for the protobuf code generation.

Since protobuf code is very rarely generated, IMO it is fine to use either go generate, a Makefile, or even a dedicated go code, but we should prevent accidental generation. Generating the protobuf code only from the pb directory should be simple and convenient enough.

cc: @gammazero

@guillaumemichel guillaumemichel added the need/triage Needs initial labeling and prioritization label Jan 21, 2025
Copy link

welcome bot commented Jan 21, 2025

Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within two business days. If this does not happen, feel free to leave a comment.
Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

  • "Priority" labels will show how urgent this is for the team.
  • "Status" labels will show if this is ready to be worked on, blocked, or in progress.
  • "Need" labels will indicate if additional input or analysis is required.

Finally, remember to use https://discuss.ipfs.io if you just need general support.

@guillaumemichel guillaumemichel added P2 Medium: Good to have, but can wait until someone steps up dif/easy Someone with a little familiarity can pick up effort/hours Estimated to take one or several hours kind/maintenance Work required to avoid breaking changes or harm to project's status quo and removed need/triage Needs initial labeling and prioritization labels Jan 21, 2025
@guillaumemichel
Copy link
Contributor Author

Closing because current protobuf generation process is considered good enough. There are no reasons to run go generate ./... from the root of the repo and maintainers can look out for modified .pb.go files when reviewing PRs.

See go-libp2p protobuf generation process for reference.

In the future it would be great to standardize protobuf generation across all repos, using a single script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dif/easy Someone with a little familiarity can pick up effort/hours Estimated to take one or several hours kind/maintenance Work required to avoid breaking changes or harm to project's status quo P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Development

No branches or pull requests

1 participant