-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Rev of proto-gen-go to ProtoPackageIsVersion3 causing breakage #763
Comments
@dsnet do you have any insight? |
I'm not sure what we can do on our end, this seems to be an issue really with the Go toolchain that there isn't an easy way (or at least I don't know of one) to build a binary at a specific revision. For the time being, you can do something like this: $ git clone [email protected]:golang/protobuf.git && (cd protobuf && git checkout v1.2.0 && go build -o $BIN_PATH/protoc-gen-go ./protoc-gen-go) && rm -r protobuf Be sure to have |
I'm going to close this as a duplicate of #751 since the general issue is that the instructions for how to install |
@dsnet Yeah... I can build the protoc-gen-go from my vendored dir (v1.2.0) but that version (unlike more recent ones) doesn’t seem to always generate the same go code for identical .proto. At what point did proto-gen-go start generating identical go for identical proto? |
The output of That said, the most likely cause of non-deterministic output is due to how |
@dsnet OK, that’s super helpful to know. Are the determined by the fully qualified path of included proto files? I ask, because we also just started using -I ../.../vendor in our protoc invocations. |
Which is to say, the fully qualified path of the imported protofiles (things like github.com/golang/protobufs/ptypes/empty/empty.proto) will different depending on where they are built, as we are pulling them in from vendor/ ) |
Errr, sorry, I mispoke. It's not the For example:
Notice that all three have different values? This behavior is surprising, but is an artifact of how protobufs were originally designed. That is, they assume a single root where all source files universally lived under in a build system similar to Bazel. It's not great for today's world of independently distributed build processes, but such is the status quo. |
OK.. so that’s a super helpful hint... let me go look for those differences and see if I can think of a solution. |
@dsnet OK... so I can't quite square this with my understanding of your explanation: - proto.RegisterFile("networkservice.proto", fileDescriptor_networkservice_efee0c6a43fef699)
+ proto.RegisterFile("networkservice.proto", fileDescriptor_networkservice_1d42c3056c917b94) |
So clearly something else is feeding into the hash. What versions of |
dep has our vendored version of github.com/golang/protobuf/protoc-gen-go/ at version v1.2.0. we are building protoc-gen-go using: go install ./vendor/github.com/golang/protobuf/protoc-gen-go/ So I'm going to go out on a limb and say its proto-gen-go version v1.2.0 (I couldn't find any way for the protoc-gen-go binary to output a version, would love to know if there's a trick there :) ).
|
And what are the versions going into the other |
The other go version is: |
OK... this is super weird. I installed a downgraded go version on my laptop: and created a fresh new alternate GOPATH and cloned the PR that is failing our CI into it. And I am not getting different hashes. So I suspect that its not the location of the GOPATH, or the go version feeding into the hash difference... what else might it be? |
It's not so much that it depends directly on the Go version, but rather parts of the standard library like |
Ah... OK... that makes a lot of sense... hmm... I have now tried with downgrade to the same go version, but on the Mac, and seem to be OK in terms of the output not changing... This is super weird. I appreciate your patience talking me through some of the ins and outs of how things work it makes it much much easier to figure out how to debug :) |
* 1. usage of NetworkServiceDiscovery 2. services registry to do connections to internal services 3. services apiRegistry for starting up network apis. 4. partial test of NSC -> NSMD/Registry 5. NSMD <-> NSMD client -> Request. Signed-off-by: Andrey Sobolev <[email protected]> * Overly simple fix to get memif working again. Signed-off-by: Ed Warnicke <[email protected]> * Switch to using NSM Name rather than URL. Signed-off-by: Ed Warnicke <[email protected]> * Added Machinery to make testing in multi-node vagrant doable Signed-off-by: Ed Warnicke <[email protected]> * Fixed vppagent-dataplane to send the correct srcIP in response to MonitorMechanisms Signed-off-by: Ed Warnicke <[email protected]> * Update remote mechaism selections + Fix tests Signed-off-by: Andrey Sobolev <[email protected]> * + Register discovery inside nsmd-k8s + Fixes Public API port. + Dst/Src fips. Signed-off-by: Andrey Sobolev <[email protected]> * It's Alive! Alive! make k8s-check works 1. Fixed the Dataplane to attach vpp to the Pod interface Otherwise... we can't send or receive the VXLAN packets 2. Factored out Selector, and implemented RoundRobin Selector 3. We need to set the workspace parameter on the local side of CrossConnects that come in remotely. Did a terrible terrible hack on serviceRegistry to make that doable. I would love to see that done cleaner. 4. nsmd-k8s wasn't correctly returning the NSM.Name ... this was causing weird situations where an nsmd was treating itself as remote. Fixed. 5. Added network_service_endpoint to remote.Connection 6. Setup CI for remote case. Signed-off-by: Ed Warnicke <[email protected]> * Undo miscommit of kubeadm join command and other CI fixes. Signed-off-by: Ed Warnicke <[email protected]> * dep ensure fix Signed-off-by: Ed Warnicke <[email protected]> * #522 Fix tests Signed-off-by: Andrey Sobolev <[email protected]> * #522 Add ifconfig dump for ping test failures Signed-off-by: Andrey Sobolev <[email protected]> * #522 Fixing tests Signed-off-by: Andrey Sobolev <[email protected]> * Fix issues with make k8s-check and vagtant Signed-off-by: Andrey Sobolev <[email protected]> * Fix machinery to work in both Virtualbox and VMWare Signed-off-by: Ed Warnicke <[email protected]> * Fix shellcheck error Signed-off-by: Ed Warnicke <[email protected]> * More shellcheck fixes Signed-off-by: Ed Warnicke <[email protected]> * Fix .circleci test for out of date protofiles Signed-off-by: Ed Warnicke <[email protected]> * Fixed issue with code generation. github.com/golang/protobuf/protoc-gen-go installed via go get is from master. master just introduced breaking changes in protoc-gen-go, that are not in any released version. So we go install ./vendor/github.com/golang/protobuf/protoc-gen-go/ to make sure the protoc-gen-go matches the various things they import. Sadly, the results in somewhat different code generation than we had before :( Signed-off-by: Ed Warnicke <[email protected]> * Make sure we can include the proto from vendor. Signed-off-by: Ed Warnicke <[email protected]> * Fix include paths so that generation here works with older protoc-gen-go Signed-off-by: Ed Warnicke <[email protected]> * Temporarily disable regen check for proto till things get sorted out. See: golang/protobuf#763 Signed-off-by: Ed Warnicke <[email protected]> * Switch to ip addr as ifconfig isn't in all containers Signed-off-by: Ed Warnicke <[email protected]> * Experiment with go 1.11 Signed-off-by: Ed Warnicke <[email protected]> * Get go version in sanity check Signed-off-by: Ed Warnicke <[email protected]> * Switch back to go version 1.10 Signed-off-by: Ed Warnicke <[email protected]> * Add a bit more output for debugging in logs. Signed-off-by: Ed Warnicke <[email protected]> * Wait for nsc pods to come up before running make k8s-check Signed-off-by: Ed Warnicke <[email protected]> * Switch to weave, until we can figure out why Calico blocks VXLAN. For reasons unclear yet, Calico results in inability for VXLAN packets to flow between Nodes. Weave seems to work. Signed-off-by: Ed Warnicke <[email protected]> * Response to comments on PR Signed-off-by: Ed Warnicke <[email protected]> * One more comment response Signed-off-by: Ed Warnicke <[email protected]>
Hi there, this issue hit me this morning. It was fine locally due to having an older version installed, but CI uses clean docker builds. I switched to installing the vendored The actual error I saw was
|
Hi everyone, I apologize that 8d0c54c recently broke a lot of people's workflows. The breakage is unfortunate and several people have written to me personally requesting that we issue v1.3.0 of Go protobufs "to resolve the issue". However, I'm going to make the case that the problem at hand is not solved by any form of versioning that we do on golang/protobuf's end. Breakage is still going to happen (unless Fundamentally, a CI system must be hermetic such that testing a given commit of your project uses a consistent set of dependencies and provides repeatability. This implies that running the CI script on a specific version of your project today, last year, or 10 years from now should produce the exact same test results. CI scripts that hard-code a specific version of the Here are things that we can do to help mitigate, but not automatically resolve the issue:
Either way, if your CI system broke as a result of 8d0c54c, then some form of change needs to happen to the CI script to ensure hermetic builds. This is not something that we can fix on golang/protobuf's end. It is unfortunate that a change needs to be made in the user's codebase, but in this situation, I strongly believe it is actually going to put your CI script in a healthier situation. As an example of a hermetic CI scripts, our own tests for the Go protobuf project hardcodes the exact versions of non-Go package dependencies for |
…760) The marshaler, unmarshaler, and sizer functions are unused ever since the underlying implementation was switched to be table-driven. Change the function to only return the wrapper structs. This change: * enables generated protos to drop dependencies on certain proto types * reduces the size of generated protos * simplifies the implementation of oneofs in protoc-gen-go Updates #708
I'd like to second @dsnet 's point about hermetic CI. I was frankly a bit embarrassed to discover that our CI was vulnerable to this kind of thing. It was vulnerable because we were go getting (which means the version we were getting tracked master). It's a testament to the overall stability of protoc-gen-go that it took us this long to hit any issues. @dsnet Thanks for taking the time to talk through with me my hash issues... still trying to get to the bottom of them, but your insight helped a lot. |
This is in the hopes of easing folks hitting golang#763 in getting to a proper use of CI. Signed-off-by: Ed Warnicke <[email protected]>
Do keep an eye out for some of the other issues I've been seeing. I'm still uncertain as to whether the issues I'm hitting from downgrading to v1.2.0 are purely of my own making (most probable), but if downgrading from master to v1.2.0 is part of the cause, best to huddle together for mutual assistance in resolving them :) |
I'm also a bit embarrassed that our CI got caught by this too. Something that could be helpful coming from the golang/protobuf side is to put binaries at a well known stable location. In the repo release would be great, or something like the k8s release notes binaries section. Forcing users to build the tool is a bit of a complexity leak. |
You can then install $ go install ./vendor/github.com/golang/protobuf/protoc-gen-go I'm not sure what the go modules equivalent workflow is. |
Not sure what was causing the "undefined: proto.ProtoPackageIsVersion3" error for me. These are the steps I did and your go get is what fixed it. go get -u github.com/golang/protobuf using protoc --version 3.6.1 |
I want to note some caveats with #763 (comment) suggestion:
|
works for me. confirmed! thanks @trtg |
problem solved. |
Peg at commit 8d0c54c1246661d9a51ca0ba455d22116d485eaa in order to avoid golang/protobuf#763.
Didn't solve for me. Arrrggggghhh, it's frustrating....! |
even tag v1.2.0 didn't solve it either |
did you find the answer yet? Please Help Me |
No man nothing so far. |
This adds modules support so that we know what versions of our various tools we are tracking. Note that there are a lot of indirect dependencies added on part of the fact we are pinning golang/protobuf at master (go get -u github.com/golang/protobuf@master) to ensure that we don't run into golang/protobuf#763.
In the spirit of @dsnet's comment above about hermetic builds and because i just solved this with go modules in 1.11 I made a repeatable process you can follow so you don't have to waste all the time I did. TLDR: I pulled the commit from master and checkout & build protobuf-gen-go on that commit, then i did a Dump of the pertinent files and build process. go.mod -- after running the above go get command. ^^
Dockerfile.build
Then run...
Hope that helps some of you wayward internet travelers. 👋 |
To fix golang/protobuf#763 Signed-off-by: Keith Smiley <[email protected]>
The Go protoc plugin (protoc-gen-go) and the protobuf package itself need to match versions. The previous Makefile installed the master version of the plugin, but dep pinned protobuf itself to 1.1.0, and now we're seeing problems on some developer machines where protobuf complains about a mismatch. This commit fixes the problem using an implementation from developer discussion on golang/protobuf#763 and regenerates the protobuf using the new (old) plugin. Fixes https://github.com/greenplum-db/gpupgrade/issues/99 .
The Go protoc plugin (protoc-gen-go) and the protobuf package itself need to match versions. The previous Makefile installed the master version of the plugin, but dep pinned protobuf itself to 1.1.0, and now we're seeing problems on some developer machines where protobuf complains about a mismatch. This commit fixes the problem using an implementation from developer discussion on golang/protobuf#763 and regenerates the protobuf using the new (old) plugin. Fixes greenplum-db/gpupgrade#99 .
I am hopeful that there is a simple solution to get around this issue :)
A few hours ago, this commit introduced proto.ProtoPackageIsVersion3
and set
const generatedCodeVersion = 3
Our CI system (and I suspect many other peoples) do a:
as part of setting up the system, and also do a:
go generate ./...
and check to make sure that regenerated files match the files in the commit (basically, checking for regen on updating the .proto files).
This has been going swimmingly until a few hours ago. In trying to debug this problem, I came to realize that the fact this worked at all is a testament to how good you guys are at backward compatibility 👍
What I discovered digging in is that we have been vendoring github.com/golang/protobuf/ v1.2.0 (because of other dependencies we have), but of course go get pulls the latest from master.
v1.2.0 lacks proto.ProtoPackageIsVersion3, so our generated code suddenly doesn't compile.
So I fell back to using:
in our CI. Unfortunately, this seems to result in some generation differences. If I run it locally, push, and then our CI runs it, the generated code doesn't quite match.
I get diffs like:
I don't precisely consider this a bug on your end, you guys are super good about backward compatibility, and at some point you do have to make the change.
Do you have any ideas about how I might navigate all of this? I suspect lots and lots of folks will shortly have similar issues...
The text was updated successfully, but these errors were encountered: