protoc-gen-gogoroach: use gogo/protobuf fork

[RaduBerinde]

Switch to using the cockroachdb/gogoproto fork for code generation
(we added a new code generation feature that we will want to make use
of).

The generated code still uses gogo/protobuf because it is necessary
for interop with other gogo packages. Our fork made no changes to
the packages used by generated code.

The diffs in the .pb.go files are inconsequential (mostly just
internal package alias names). They can be inspected here:
https://gist.github.com/RaduBerinde/e1a20396bad4004d84546b83c0cdd1fa

Informs: #134805
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[rickystewart]

The generated code still uses gogo/protobuf because it is necessary
for interop with other gogo packages.

Why is this? Can you provide some examples?

On its face, it is very confusing to have the original version of the code and the fork imported in the same repo. I can see how that could introduce issues down the line. I would prefer not to do this unless we have no other option.

[RaduBerinde]

Why is this? Can you provide some examples?

Any dependency that has its own .pb.go files will use the original gogoproto. We could potentially generate our own versions of those generated files with bazel, but I don't see how that would ever work in a normal go build.

[rickystewart]

Any dependency that has its own .pb.go files will use the original gogoproto. We could potentially generate our own versions of those generated files with bazel, but I don't see how that would ever work in a normal go build.

Can you be more specific? What dependencies have their own .pb.go files?

If we are cutting over to the new fork, we should cut over to the fork (i.e. update the dependencies with the hard-coded references to gogoproto to use a non-deprecated option)

[RaduBerinde]

Can you be more specific? What dependencies have their own .pb.go files?

One is ours (cockroachdb/errors/errorspb) but we can easily switch that one over. The others are from gogo: https://github.com/gogo/googleapis, https://github.com/gogo/protobuf (this one doesn't have .pb.go files but includes gogo/protobuf directly.

There are probably others.

I think it will be inevitable to eventually fork all of them so maybe I should just bite the bullet and do it now?

[rickystewart]

I think it will be inevitable to eventually fork all of them so maybe I should just bite the bullet and do it now?

If you must get this PR in soon to address some sort of issue, I will approve this PR and we can take this as a follow-up item. However as you can see it puts us in a very confusing state where we actually have two separate forks of the same repo vendored into CRDB. I can very easily see someone making a change in our fork and updating go.mod here, making the reasonable assumption that they have addressed the issue, when in fact they only addressed the issue in one of two forks. Debugging the issue is going to be very confusing to them and the version of the code being used is going to vary based on where it was imported from and whether the imports have happened to be updated.

(Yes, you currently have all this information in your head, but not everyone will have access to all this knowledge.)

It will of course be more difficult to get in there, do the forks, update the paths, etc., but at the end of that process we will have an understandable network of forks that we fully manage and will not be left in this "purgatory" state where we have two forks of the same code handing off between the other in confusing/surprising ways.

[RaduBerinde]

The immediate motivation for this PR is to bake in the improvements in #134481 instead of maintaining separate marshal/unmarshal code.

This is a full list of packages that depend on gogo/protobuf:
https://gist.github.com/RaduBerinde/5f1f483421a350a9756f0f0e9aef98ee

We'd need to fork all of these if we wanted to completely get rid of the original one.. I don't think that's feasible.. I really with go.mod had a replace version that can replace indirect deps too..

[RaduBerinde]

Actually, I might be wrong about replace.. What would happen if we add a replace github.com/gogo/protobuf => github.com/cockroachdb/gogoproto directive in go.mod. Would it use the latter one for all instances where github.com/gogo/protobuf is imported?

[RaduBerinde]

Actually, I might be wrong about replace.. What would happen if we add a replace github.com/gogo/protobuf => github.com/cockroachdb/gogoproto directive in go.mod. Would it use the latter one for all instances where github.com/gogo/protobuf is imported?

I think for that to work, we'd need to keep the original paths in the fork's go.mod. I updated them (cockroachdb/gogoproto@e506ef9 and cockroachdb/gogoproto@fd7236b) because it was impossible to run any of the tests inside the repo. But maybe we can maintain a "mirror" of master that has the updated paths that can be used for development inside the fork.

[RaduBerinde]

Good news, I reverted the module path changes in our fork and used the replace directive and that seems to work just fine and we're switching everything over. Opened separate PR #134918.

[tbg]

Just for completeness, the googleapis dep is pretty small, it's just through this package I think: https://github.com/gogo/googleapis/tree/master/google/rpc

We could probably just bring this in-house and then also generate the pb.go file.