-
Notifications
You must be signed in to change notification settings - Fork 106
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: ipns protobuf namespace conflict #794
base: main
Are you sure you want to change the base?
Conversation
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. |
70a5a10
to
8875c82
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #794 +/- ##
==========================================
- Coverage 60.52% 60.50% -0.02%
==========================================
Files 245 245
Lines 31130 31130
==========================================
- Hits 18841 18835 -6
- Misses 10613 10616 +3
- Partials 1676 1679 +3
|
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.
That works for K3s, I'm not seeing any conflicts when I inject your PR commit.
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.
Might be better to avoid makefile. See comments.
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.
Lets merge this before the next boxo version and kubo RC2. We can decide between Makefile
and go:generate
in a later PR.
d443b97
to
b91a277
Compare
b91a277
to
fbe6ba1
Compare
Using go:generate also means it can be run from a top-level directory, |
Context
To solve protobuf namespace conflicts, #789 renamed
record.proto
toipns-record.proto
breaking protobuf dependency import since the filename has now changed. I think there is a better way to solve #788.Proposed solution
While the
go_package
option set in the.proto
file isn't included in the registered protobuf name, we can use a customMakefile
to register the full package name. This seems like a cleaner approach, since it registers the protobuf with the full package path, instead of justipns-record.proto
, and it doesn't break existingrecord.proto
imports.Protobuf package source before and after.
Also see libp2p/go-libp2p-record#62
Would that work for you @dereknola?