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.proto warning #3525

Closed
dijiwiz opened this issue Jun 26, 2020 · 25 comments · Fixed by #3526
Closed

pb.proto warning #3525

dijiwiz opened this issue Jun 26, 2020 · 25 comments · Fixed by #3526
Assignees
Labels
bug 🐞 Something isn't working upstream ⬆️ Relates to some dependency of this project
Milestone

Comments

@dijiwiz
Copy link

dijiwiz commented Jun 26, 2020

I was upgrading to v2.1 and the build seemed to go fine.

When I attempted to print up the version to confirm I was shown this warning message:

2020/06/26 17:58:17 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

v2.1.0 h1:MC4d65RCVaEKy1iOFjsD51mybOwS8qdEVBi7ESDhUfE=

It seems to run fine but thought that may be of interest to the project.

@mohammed90
Copy link
Member

According to hypermodeinc/dgraph#5290, this is due to upgrading the github.com/golang/protobuf package and can be fixed by downgrading it. Needs further digging to understand the nuance of what's going on there. Nonetheless, the fix seems simple.

@dijiwiz
Copy link
Author

dijiwiz commented Jun 27, 2020

Thank you!

@dijiwiz dijiwiz closed this as completed Jun 27, 2020
@mohammed90
Copy link
Member

I'll keep it open. We need to resolve it by inspecting our dependency graph and pick appropriate packages version.

The error originates from github.com/dgraph-io/dgraph, which is in our dep graph due to github.com/smallstep/nosql.

$ go mod why github.com/dgraph-io/badger
# github.com/dgraph-io/badger
github.com/caddyserver/caddy/v2/modules/caddypki/acmeserver
github.com/smallstep/nosql
github.com/smallstep/nosql/badger/v1
github.com/dgraph-io/badger

@mohammed90 mohammed90 reopened this Jun 27, 2020
@linquize
Copy link
Contributor

c9049bd causes the issue.

@mholt
Copy link
Member

mholt commented Jun 27, 2020

That's weird, those are just patch updates. There should be no breaking changes in those.

@francislavoie
Copy link
Member

I think github.com/smallstep/nosql v0.3.1 is to blame.

Technically it's not a breaking change in isolation for that lib to update its protobuf dependency, but the problem is that we also depend on CEL which also uses protobuf, but a version from a different repo.

I think those weren't marked as conflicting dependencies by the go mod dependency solver so we have two different versions pulled in of protobuf now.

I'm just thinking out loud based on the evidence I think is apparent but I can't say for sure, I don't know how go mod does its dep resolution, just making guesses here.

I think #3526 is related.

@mohammed90
Copy link
Member

mohammed90 commented Jun 28, 2020

Sorry, my investigative skills were subpar. It isn't coming from dgraph. dgraph experienced the same issue because they have their own pb.proto and depend on badger which also has pb.proto. We're seeing it from badger. I think this is because our go.sum has multiple versions of badger, which is transitive from github.com/smallstep/nosql, who also see the same in their go.sum.

caddy/go.sum

Lines 168 to 173 in c9049bd

github.com/dgraph-io/badger v1.5.3 h1:5oWIuRvwn93cie+OSt1zSnkaIQ1JFQM8bGlIv6O6Sts=
github.com/dgraph-io/badger v1.5.3/go.mod h1:VZxzAIRPHRVNRKRo6AXrX9BJegn6il06VMTZVJYCIjQ=
github.com/dgraph-io/badger v1.6.1 h1:w9pSFNSdq/JPM1N12Fz/F/bzo993Is1W+Q7HjPzi7yg=
github.com/dgraph-io/badger v1.6.1/go.mod h1:FRmFw3uxvcpa8zG3Rxs0th+hCLIuaQg8HlNV5bjgnuU=
github.com/dgraph-io/badger/v2 v2.0.1-rc1.0.20200413122845-09dd2e1a4195 h1:n8KbImHW5qZCXv1y3tHjz5yz418/eTxeRJZ2ZuDm1ZU=
github.com/dgraph-io/badger/v2 v2.0.1-rc1.0.20200413122845-09dd2e1a4195/go.mod h1:3KY8+bsP8wI0OEnQJAKpd4wIJW/Mm32yw2j/9FUVnIM=

Downgrading github.com/smallstep/nosql to v0.3.0 resolves it, if this is a route we want to take. I checked the diff between v0.3.1 and v0.3.0, and there isn't anything noteworthy besides updating badger and accommodating for the API change of badger.

@mohammed90
Copy link
Member

Perhaps golang/protobuf#1122 is loosely related

@mholt
Copy link
Member

mholt commented Jun 29, 2020

@mohammed90 Thanks for the investigation. Downgrading nosql is fine for now.

I'm still trying to understand the crux of the origin of the problem. I won't have much time to dig into this particular issue for a while since I'm busy working on the website backend. Is this due to a combination of our own dependencies, or is the problem upstream in another dependency?

@mohammed90
Copy link
Member

There's nothing for us to do in our codebase, neither in smallstep/nosql. It's just the mere dependency on badger. It's resolved in badger as of PR dgraph-io/badger#1293. So to resolve it from the root, smallstep/nosql should depend on a commit dgraph-io/badger@8097259 or any commit after it.

If someone can convince dgraph-io/badger to do a point release just for that, that'd be great for everyone.

@francislavoie francislavoie linked a pull request Jun 29, 2020 that will close this issue
@mholt
Copy link
Member

mholt commented Jun 29, 2020

Alright, cool. We'll make sure this gets out in a 2.1.1 release soon.

@mvdan
Copy link

mvdan commented Sep 14, 2020

For what it's worth, I'm fixing this upstream at dgraph-io/badger#1519, which should remove the warning when a Go binary includes both badger v1 and v2.

@dijiwiz
Copy link
Author

dijiwiz commented Jan 1, 2021

It's back! I tried to compile the new 2.3.0 code and am getting the same message.

It had gone away on the 2.2 branch.

@francislavoie
Copy link
Member

francislavoie commented Jan 1, 2021

@dijiwiz make sure to use the latest version of Go when building. And if that still doesn't work, please post your build logs and more detail.

@mholt
Copy link
Member

mholt commented Jan 1, 2021

Yeah it's not showing up for me.

@dijiwiz
Copy link
Author

dijiwiz commented Jan 1, 2021

I am using Go 1.5.6. I blew off my go.sum and module directory and did a go mod tidy and rebuilt. It works now.

Previously I had updated the go.mod version and ran a go mod tidy.

Thanks for the help and the program. Happy new year!

@TristonianJones
Copy link
Contributor

This may or may not be helpful, but cel-go v0.7.* supports the latest version of the Golang protobuf API. I notice that the CEL dependency is at v0.6.0. Perhaps a version bump of CEL will help?

@mvdan
Copy link

mvdan commented Mar 31, 2021

In case anyone is confused, the latest badgerdb v2 release is from September 2020, and my fix went in a few weeks later, so right now using the latest v1 and v2 tags still gets you the warning. They haven't responded to my comments, so I think v2 just isn't maintained anymore. If you're still building v1 together with v2, the only fixes seem to be either pulling the last v2 commit, or jumping to v3 directly.

@mholt mholt added the upstream ⬆️ Relates to some dependency of this project label Mar 31, 2021
@mvdan
Copy link

mvdan commented Jul 20, 2021

pulling the last v2 commit

In hindsight, that's not actually possible, as there's no v2 branch. If you pull a commit from master shortly after the latest v2 release, and including my proto fix, then it's likely that you're also pulling in unrelated changes or stuff from v3. So that wouldn't really be running "badger v2", but rather "badger at random master commit".

The protobuf library in Go started panicking on this namespace conflict since March, and it's really starting to bite me at work. I've posted one more ping to beg that they backport the commit and do a bugfix release. Upvotes or supporting comments are welcomed, if you're feeling the conflict pain and would like to see it happen. Otherwise, it seems like I'm the only one bothering them with this fatal bug :)

@mholt
Copy link
Member

mholt commented Jul 20, 2021

Yay, looks like they might finally be on it.

@mvdan
Copy link

mvdan commented Jul 22, 2021

They finally did it! Tears of joy!

@ThinkChaos
Copy link

Can we get a release with that fixes this please?
v2.4.3's dependency graph still uses badger v2.2007.2 which has the issue.

@francislavoie
Copy link
Member

@ThinkChaos badger is an indirect dependency, we're waiting for smallstep/nosql#12 to totally resolve this.

@ThinkChaos
Copy link

Ah sorry I missed that! This issue should probably stay open then.

@ThinkChaos
Copy link

ThinkChaos commented Aug 17, 2021

I opened smallstep/nosql#13 to bump their dependency.
In the mean time anyone can add --with github.com/smallstep/nosql@v0.3.7 to their xcaddy invocation to bypass the issue.

Update my PR there has been merged. I updated the --with above to use the official repo's new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working upstream ⬆️ Relates to some dependency of this project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants