-
Notifications
You must be signed in to change notification settings - Fork 111
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
Define a go_package
for protobuf, rename to a more unique ipns-record.proto
#789
Conversation
- Use relative source to reduce static values Signed-off-by: Derek Nola <[email protected]>
Signed-off-by: Derek Nola <[email protected]>
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
@dereknola does the updated protobuf actually fix your issues? I suspect that it might be coming from other protobufs where the package name might be too generic such as go-libp2p-kad-dht and go-libp2p-record for example. I already have opened PRs for these two repos, I'll update the package name, and push to get them merged. |
@guillaumemichel Yes, the PR I've submitted does fix our issue in K3s. I've got a branch here where the boxo dependecy is replaced with my own version (i.e. this current PR commit). Here is the record grep on that branch
I appreciate you opening the other two PRs. Its possible that if those got merged it would also fix our issue, I haven't tried replacing those. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dereknola for your contribution!
Looks good to me! What seems to matter is the go_package
option, filename shouldn't create conflict, though renaming it to ipns-record.proto
adds clarity 👍🏻
TIL: https://protobuf.dev/programming-guides/proto3/#packages
In Go, the
package
directive is ignored, and the generated.pb.go
file is in the package named after the correspondinggo_proto_library
Bazel rule. For open source projects, you must provide either ago_package
option or set the Bazel-M
flag.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #789 +/- ##
==========================================
- Coverage 60.48% 60.47% -0.01%
==========================================
Files 245 245
Lines 31138 31138
==========================================
- Hits 18833 18830 -3
- Misses 10625 10628 +3
Partials 1680 1680
|
This is an attempt to address #788
Note in the first commit that I first attempted to follow the online recommendations from:
And only define a unique
option go_package
. This failed to resolve the issue as the go compile still seems to complain and is very upset that the file itself is still calledrecord.proto
.Thus I also renamed to file itself to something less generic. Note that this should not affect external use, as the package is still
pb
and since #339, external usage of the protobuf is around theipns
package, not theipns/pb
package directly.If someone with more knowledge around protobuf generation can see a better way to address the issue or improve this PR, please let me know.