Refactor registry and generator to use protogen

[adambabik]

References to other Issues or PRs

This is continuation of work submitted and merged in #1668.

Brief description of what is fixed or changed

This PR switches fully to google.golang.org/protobuf/compiler/protogen which is a new plugin to protoc to generate Go code.

The most important changes are:

• rely on google.golang.org/protobuf/compiler/protogen to figure out Go package names and file names,
• removal of flags that are no longer support by protoc-gen-go
• moving handling of some flags like -Mpath/to/another.proto=example.com/example or -paths to protoc-gen-go

Other comments

The follow up work:

• test whether it solves the problem described in protoc-gen-grpc-gateway: can't generate gateway stubs for multiple packages in one go #1612
• disable warnings from protoc-gen-go in tests

[johanbrandhorst]

Note that #1752 and #1757 may require you to rebase this PR a few times, I hope that's okay. Thanks for all your work Adam, it's greatly appreciated.

[adambabik]

That's cool, this PR is taking long time so it's expected. I think I will have it ready today/tomorrow. I just fixed the last test, yay!

No problem at all, it's a great open source tool and I am glad I can give something to the community as well :)

[adambabik]

@johanbrandhorst I can't reproduce this failed bazel build. I run both docker run commands from CONTRIBUTING.md and they worked fine. Is it possible it's a flake?

[johanbrandhorst]

Is it possible that FieldMaskFromRequestBody returns the old type field mask? It looks like a build error, does it not reproduce with go test ./...?

[adambabik]

Oh yeah, that might be it, thanks! I refactored code to protobuf/types/known/fieldmaskpb but it looks like only *_test.go files were changed.

[adambabik]

I needed to revert usage of google.golang.org/protobuf/types/known/fieldmaskpb. It looks like there is some problem with package versions when building with bazel. I can't reproduce it locally unfortunately.

In generated .pb.go, it looks like this:

wrappers "github.com/golang/protobuf/ptypes/wrappers"
field_mask "google.golang.org/genproto/protobuf/field_mask"

github.com/golang/protobuf/ptypes/wrappers has aliases to google.golang.org/protobuf/types/known/wrapperspb. Similarly google.golang.org/genproto/protobuf/field_mask has aliases to google.golang.org/protobuf/types/known/fieldmaskpb but in bazel build in the CI, there was an error that google.golang.org/genproto/protobuf/field_mask needs to be update to a version released after 26 May, 2020 which actually is a case currently.

[johanbrandhorst]

Yeah, I had a similar experience, I think it's going to take until rules_go has switched to a reasonably recent version of golang/protobuf. Just leave it for now, I will be making that change when we can.

[adambabik]

Got it. In my opinion, it's good for a thorough review. There are still two todo items in the description but I think whatever will result from them, it will be a separate issue.

[johanbrandhorst]

Could you rebase on master again please? I just merged another PR that affects these files.

[codecov-io]

Codecov Report

Merging #1756 into master will decrease coverage by 0.57%.
The diff coverage is 48.14%.

@@            Coverage Diff             @@
##           master    #1756      +/-   ##
==========================================
- Coverage   57.86%   57.28%   -0.58%     
==========================================
  Files          34       34              
  Lines        3669     3615      -54     
==========================================
- Hits         2123     2071      -52     
+ Misses       1286     1284       -2     
  Partials      260      260              

Impacted Files	Coverage Δ
protoc-gen-grpc-gateway/main.go	7.14% <0.00%> (+0.24%)	⬆️
internal/descriptor/registry.go	55.23% <61.11%> (-8.99%)	⬇️
internal/descriptor/types.go	50.44% <100.00%> (ø)
...-gen-grpc-gateway/internal/gengateway/generator.go	32.92% <100.00%> (-2.79%)	⬇️
...otoc-gen-openapiv2/internal/genopenapi/template.go	58.52% <0.00%> (-0.44%)	⬇️
internal/descriptor/services.go	76.02% <0.00%> (+1.53%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update caf1cf5...bb76634. Read the comment docs.

[johanbrandhorst]

Really nice to see all the logic we can move out and consume from protogen, nice work! I'm curious about some of the flag removals and the test removals, should we worry that this will break any users? The removal of the module flag for example, does that now get consumed by protogen?

[adambabik]

I removed four flags:

• paths, import_path, and module are all handled by protogen,
• import_prefix is deprecated in protobuf-go and I followed the same logic.

The problem might be with different interpretation of these flags. I tried to figure this out and it looked the same but that might not be 100% true.

Regarding the removed tests, I removed everything related to methods that were also removed. The main important thing that got removed was generating the output path (for example TestGenerateOutputPath) which now is a responsibility of protogen. Some other tests were testing removed flags like TestLoadFileWithPrefix which tested import_prefix or TestLoadSetInputPath and TestLoadGoPackageInputPath which tested import_path.

[johanbrandhorst]

Ugh, we really should've got that import_prefix removal in before v2.0.0 but I guess the protoc-gen-go precedent might be good enough, and the workaround is to stay on v1.