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

pb: avoid protobuf warning due to common filename #1519

Merged
merged 1 commit into from
Sep 27, 2020
Merged

pb: avoid protobuf warning due to common filename #1519

merged 1 commit into from
Sep 27, 2020

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Sep 14, 2020

If a Go program imports both badger (v1) and badger/v2, a warning will
be produced at init time:

WARNING: proto: file "pb.proto" is already registered
A future release will panic on registration conflicts. See:
https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

The problem is the "pb.proto" filename; it's registered globally, which
makes it very likely to cause conflicts with other protobuf-generated
packages in a Go binary.

Coincidentally, this is a problem with badger's pb package in the v1 module,
since that too uses the name "pb.proto". Instead, call the file
"badgerpb2.proto", which should be unique enough, and it's also the name
we use for the Protobuf package.

Finally, update gen.sh to work out of the box via just "bash gen.sh"
without needing extra tweaks, thanks to the "paths=source_relative"
option. It forces output files to be produced next to the input files,
which is what we want.


This change is Reviewable

If a Go program imports both badger (v1) and badger/v2, a warning will
be produced at init time:

	WARNING: proto: file "pb.proto" is already registered
	A future release will panic on registration conflicts. See:
	https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

The problem is the "pb.proto" filename; it's registered globally, which
makes it very likely to cause conflicts with other protobuf-generated
packages in a Go binary.

Coincidentally, this is a problem with badger's pb package in the v1 module,
since that too uses the name "pb.proto". Instead, call the file
"badgerpb2.proto", which should be unique enough, and it's also the name
we use for the Protobuf package.

Finally, update gen.sh to work out of the box via just "bash gen.sh"
without needing extra tweaks, thanks to the "paths=source_relative"
option. It forces output files to be produced next to the input files,
which is what we want.
@mvdan
Copy link
Contributor Author

mvdan commented Sep 14, 2020

The alternative was to use subdirectories as part of the filename, like pb/badger/v2.proto or something like that. I personally find the method in this PR a little easier, since there are less directories to mess with.

@mvdan
Copy link
Contributor Author

mvdan commented Sep 24, 2020

Friendly ping @jarifibrahim, since you reviewed my earlier protobuf PRs.

Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @mvdan. It looks good to me.
@NamanJain8 please review.

Copy link
Contributor

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @mvdan!!

@jarifibrahim jarifibrahim merged commit 3e6a4b7 into hypermodeinc:master Sep 27, 2020
gammazero added a commit to ipfs/go-ds-badger2 that referenced this pull request Sep 30, 2020
This resolves a protobuf namespace conflict that would happen when both badger1 and badger2 are linked into the same binary.  For more information on this issue see: hypermodeinc/badger#1519
jarifibrahim pushed a commit that referenced this pull request Oct 2, 2020
If a Go program imports both badger (v1) and badger/v2, a warning will
be produced at init time:

	WARNING: proto: file "pb.proto" is already registered
	A future release will panic on registration conflicts. See:
	https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

The problem is the "pb.proto" filename; it's registered globally, which
makes it very likely to cause conflicts with other protobuf-generated
packages in a Go binary.

Coincidentally, this is a problem with badger's pb package in the v1 module,
since that too uses the name "pb.proto". Instead, call the file
"badgerpb2.proto", which should be unique enough, and it's also the name
we use for the Protobuf package.

Finally, update gen.sh to work out of the box via just "bash gen.sh"
without needing extra tweaks, thanks to the "paths=source_relative"
option. It forces output files to be produced next to the input files,
which is what we want.
@mvdan
Copy link
Contributor Author

mvdan commented Jan 5, 2021

@jarifibrahim hi again :) will there be a release soon with this fix? We're still getting the warnings four months later, so it would be nice to be able to upgrade to a newer release tag. We could update to the latest v2 master, but I imagine you don't want to encourage that as it might not be as stable as a proper release.

@mvdan
Copy link
Contributor Author

mvdan commented Jan 22, 2021

Friendly ping - I can see there have been v3 releases, but v2 is sort of broken in this state. Is it entirely unmaintained now? cc @aman-bansal

@mvdan
Copy link
Contributor Author

mvdan commented Jul 20, 2021

Ping once again @jarifibrahim @NamanJain8 @manishrjain - since March, the protobuf Go library has panicked on namespace conflicts: https://github.com/protocolbuffers/protobuf-go/releases/tag/v1.26.0#v1.26-fatal-namespace-conflicts

This is making Badger unusable for any large project in Go. Yes, the project itself can upgrade to v3 with some effort, but dependencies tend to end up pulling in v1 and v2 indirectly, and those will panic at init time, blowing up the entire thing.

The fix is very simple, as far as I can tell. Someone just needs to backport this commit into the latest v2 tag, and publish a new bugfix release.

The only possible alternative I see, unfortunately, is a hard fork and abusing replace directives.

What IPFS did was start using a commit from master, but I don't see that as a good solution by any means. That commit is not even related to the v2 releases, and might include v3 changes.

NamanJain8 pushed a commit that referenced this pull request Jul 20, 2021
If a Go program imports both badger (v1) and badger/v2, a warning will
be produced at init time:

	WARNING: proto: file "pb.proto" is already registered
	A future release will panic on registration conflicts. See:
	https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

The problem is the "pb.proto" filename; it's registered globally, which
makes it very likely to cause conflicts with other protobuf-generated
packages in a Go binary.

Coincidentally, this is a problem with badger's pb package in the v1 module,
since that too uses the name "pb.proto". Instead, call the file
"badgerpb2.proto", which should be unique enough, and it's also the name
we use for the Protobuf package.

Finally, update gen.sh to work out of the box via just "bash gen.sh"
without needing extra tweaks, thanks to the "paths=source_relative"
option. It forces output files to be produced next to the input files,
which is what we want.

(cherry picked from commit 3e6a4b7)
@NamanJain8
Copy link
Contributor

NamanJain8 commented Jul 20, 2021

Hey @mvdan, really thanks for reporting this issue. Also, we apologize that your issue somehow got missed by the team.
I have raised the PR #1731. It would address your concern. I will get it reviewed and create a new release.

@mvdan
Copy link
Contributor Author

mvdan commented Jul 20, 2021

Thank you! Can't wait to be able to fix this properly downstream.

NamanJain8 added a commit that referenced this pull request Jul 21, 2021
If a Go program imports both badger (v1) and badger/v2, a warning will
be produced at init time:

	WARNING: proto: file "pb.proto" is already registered
	A future release will panic on registration conflicts. See:
	https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

The problem is the "pb.proto" filename; it's registered globally, which
makes it very likely to cause conflicts with other protobuf-generated
packages in a Go binary.

Coincidentally, this is a problem with badger's pb package in the v1 module,
since that too uses the name "pb.proto". Instead, call the file
"badgerpb2.proto", which should be unique enough, and it's also the name
we use for the Protobuf package.

Finally, update gen.sh to work out of the box via just "bash gen.sh"
without needing extra tweaks, thanks to the "paths=source_relative"
option. It forces output files to be produced next to the input files,
which is what we want.

(cherry picked from commit 3e6a4b7)

Co-authored-by: Daniel Martí <[email protected]>
@NamanJain8
Copy link
Contributor

Hey folks, do check out v2.2007.3 that addresses this protobuf namespace conflict issue. 🎉

@mvdan
Copy link
Contributor Author

mvdan commented Jul 22, 2021

Thank you @NamanJain8! Confirming that it fixes the issue in one of the projects, and I've sent a PR to fix go-ipfs as well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants