Replace prototool with buf by johanbrandhorst · Pull Request #847 · improbable-eng/grpc-web

johanbrandhorst · 2021-02-03T10:47:29Z

Changes

Replace prototool generation and linting with buf

Verification

Reran generation and lint locally. Need to see what CI thinks about the changes.

johanbrandhorst · 2021-02-03T11:01:25Z

+    ENUM_VALUE_PREFIX:
+      - improbable/grpcweb/test/test.proto
+    ENUM_ZERO_VALUE_SUFFIX:
+      - improbable/grpcweb/test/test.proto
+    FIELD_LOWER_SNAKE_CASE:
+      - improbable/grpcweb/test/test.proto
+    PACKAGE_VERSION_SUFFIX:
+      - improbable/grpcweb/test/test.proto
+    RPC_REQUEST_RESPONSE_UNIQUE:
+      - improbable/grpcweb/test/test.proto
+    RPC_REQUEST_STANDARD_NAME:
+      - improbable/grpcweb/test/test.proto
+    RPC_RESPONSE_STANDARD_NAME:
+      - improbable/grpcweb/test/test.proto


These ignores are based on the existing failures in this file. These are the warnings:

$ buf lint integration_test/proto/improbable/grpcweb/test/test.proto:3:1:Package name "improbable.grpcweb.test" should be suffixed with a correctly formed version, such as "improbable.grpcweb.test.v1". integration_test/proto/improbable/grpcweb/test/test.proto:10:5:Enum value name "NONE" should be prefixed with "FAILURE_TYPE_". integration_test/proto/improbable/grpcweb/test/test.proto:10:5:Enum zero value name "NONE" should be suffixed with "_UNSPECIFIED". integration_test/proto/improbable/grpcweb/test/test.proto:11:5:Enum value name "CODE" should be prefixed with "FAILURE_TYPE_". integration_test/proto/improbable/grpcweb/test/test.proto:13:5:Enum value name "CODE_UNICODE" should be prefixed with "FAILURE_TYPE_". integration_test/proto/improbable/grpcweb/test/test.proto:28:10:Field name "Value" should be lower_snake_case, such as "value". integration_test/proto/improbable/grpcweb/test/test.proto:39:3:".google.protobuf.Empty" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:39:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:39:17:RPC request type "Empty" should be named "PingEmptyRequest" or "TestServicePingEmptyRequest". integration_test/proto/improbable/grpcweb/test/test.proto:39:49:RPC response type "PingResponse" should be named "PingEmptyResponse" or "TestServicePingEmptyResponse". integration_test/proto/improbable/grpcweb/test/test.proto:41:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:41:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:43:3:".google.protobuf.Empty" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:43:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:43:17:RPC request type "PingRequest" should be named "PingErrorRequest" or "TestServicePingErrorRequest". integration_test/proto/improbable/grpcweb/test/test.proto:43:39:RPC response type "Empty" should be named "PingErrorResponse" or "TestServicePingErrorResponse". integration_test/proto/improbable/grpcweb/test/test.proto:45:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:45:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:45:16:RPC request type "PingRequest" should be named "PingListRequest" or "TestServicePingListRequest". integration_test/proto/improbable/grpcweb/test/test.proto:45:45:RPC response type "PingResponse" should be named "PingListResponse" or "TestServicePingListResponse". integration_test/proto/improbable/grpcweb/test/test.proto:47:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:47:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:47:27:RPC request type "PingRequest" should be named "PingPongBidiRequest" or "TestServicePingPongBidiRequest". integration_test/proto/improbable/grpcweb/test/test.proto:47:56:RPC response type "PingResponse" should be named "PingPongBidiResponse" or "TestServicePingPongBidiResponse". integration_test/proto/improbable/grpcweb/test/test.proto:49:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:49:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:49:25:RPC request type "PingRequest" should be named "PingStreamRequest" or "TestServicePingStreamRequest". integration_test/proto/improbable/grpcweb/test/test.proto:49:47:RPC response type "PingResponse" should be named "PingStreamResponse" or "TestServicePingStreamResponse". integration_test/proto/improbable/grpcweb/test/test.proto:51:3:RPC "Echo" has the same type ".improbable.grpcweb.test.TextMessage" for the request and response. integration_test/proto/improbable/grpcweb/test/test.proto:51:12:RPC request type "TextMessage" should be named "EchoRequest" or "TestServiceEchoRequest". integration_test/proto/improbable/grpcweb/test/test.proto:51:34:RPC response type "TextMessage" should be named "EchoResponse" or "TestServiceEchoResponse". integration_test/proto/improbable/grpcweb/test/test.proto:67:3:".google.protobuf.Empty" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:67:54:RPC response type "Empty" should be named "ContinueStreamResponse" or "TestUtilServiceContinueStreamResponse". integration_test/proto/improbable/grpcweb/test/test.proto:73:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:73:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs. integration_test/proto/improbable/grpcweb/test/test.proto:73:19:RPC request type "PingRequest" should be named "NonExistantRequest" or "FailServiceNonExistantRequest". integration_test/proto/improbable/grpcweb/test/test.proto:73:41:RPC response type "PingResponse" should be named "NonExistantResponse" or "FailServiceNonExistantResponse".

I didn't want to fix any warnings in this PR to make the impact as small as possible.

johanbrandhorst · 2021-02-03T11:02:41Z

+fi
+
+curl -sSL \
+  https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VER}/protoc-${PROTOC_VER}-linux-$(uname -m).zip \


Note that this doesn't use uname -s since protoc uses lowercase names. I figured since this is for CI it doesn't matter, but I can make probably do a lowercase transformation somehow if necessary.

johanbrandhorst · 2021-02-03T12:09:34Z

Tests that failed are giving me an npm error:

/tmp/.bash_env-601a8141e5331f7178d9a11c-0-build: line 2: /home/circleci/.nvm/nvm.sh: No such file or directory
/bin/bash: npm: command not found

No idea what's going on there, any ideas @MarcusLongmuir?

johanbrandhorst · 2021-02-03T12:10:21Z

Rerunning the tests now.

MarcusLongmuir · 2021-02-05T12:20:22Z

Do we still need to install these?

- run: go install ./vendor/github.com/golang/protobuf/protoc-gen-go
- run: export PATH=/go/src/github.com/improbable-eng/grpc-web/protobuf/bin:$PATH

What's the relationship between buf and protoc in this configuration? From my understanding buf doesn't require protoc, but it's optional.

johanbrandhorst · 2021-02-05T12:23:57Z

Do we still need to install these?

run: go install ./vendor/github.com/golang/protobuf/protoc-gen-go

run: export PATH=/go/src/github.com/improbable-eng/grpc-web/protobuf/bin:$PATH

We still have to install the protobuf plugins, as buf doesn't handle language specific generation. This is consistent with protoc when not looking at the built-in generators.

What's the relationship between buf and protoc in this configuration? From my understanding buf doesn't require protoc, but it's optional.

Indeed, buf doesn't depend on protoc, however, since we're generating using --js_out we need the JavaScript generator plugin, which is built into protoc, so that's the only reason we still have to download protoc.

johanbrandhorst · 2021-03-13T12:53:27Z

Bump

johanbrandhorst · 2021-03-16T12:12:46Z

Rebased on master and removed some old dep references I missed in the modules migration, please take a look when you can @MarcusLongmuir.

MarcusLongmuir

Thanks for updating this 👍

johanbrandhorst · 2021-03-17T10:23:31Z

Hm, not sure about the test errors, could it be flakes? The logic shouldn't have changed and all the other tests passing is suspicious.

MarcusLongmuir · 2021-03-17T10:24:38Z

Looks like the testserver is panicing with panic: rpc error: code = Unavailable desc = transport is closing. This is unrelated so it's safe to merge this.

MarcusLongmuir · 2021-03-17T10:26:16Z

You should be able to retry just the failing tests and merge when they pass, but comment if that doesn't work.

johanbrandhorst · 2021-03-17T10:39:11Z

Thanks for the quick review!

improbable-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. labels Feb 3, 2021

johanbrandhorst force-pushed the replace-prototool-with-buf branch from 3db5016 to c74f699 Compare February 3, 2021 10:53

johanbrandhorst commented Feb 3, 2021

View reviewed changes

johanbrandhorst marked this pull request as ready for review February 5, 2021 12:24

improbable-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2021

johanbrandhorst requested a review from MarcusLongmuir March 13, 2021 12:53

johanbrandhorst force-pushed the replace-prototool-with-buf branch from c74f699 to 1a272a9 Compare March 16, 2021 12:08

MarcusLongmuir reviewed Mar 16, 2021

View reviewed changes

Comment thread .circleci/config.yml

Comment thread integration_test/package.json

johanbrandhorst added 6 commits March 17, 2021 09:49

Replace prototool with buf

9faba85

Remove dep tooling references

8ddb5d4

Update protoc and buf versions

24f50e1

Add CI test to ensure files are consistently generated

c56ff3f

Update Go version to 1.16

800bb5d

Remove dependencies added by tool installs in CI

b89cb7f

johanbrandhorst force-pushed the replace-prototool-with-buf branch from bd79d71 to b89cb7f Compare March 17, 2021 09:50

Update module file

3260fd2

MarcusLongmuir approved these changes Mar 17, 2021

View reviewed changes

johanbrandhorst merged commit 84ab65f into master Mar 17, 2021

johanbrandhorst deleted the replace-prototool-with-buf branch March 17, 2021 10:39

Uh oh!

Conversation

johanbrandhorst commented Feb 3, 2021

Changes

Verification

Uh oh!

johanbrandhorst Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

johanbrandhorst Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

johanbrandhorst Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

johanbrandhorst commented Feb 3, 2021

Uh oh!

johanbrandhorst commented Feb 3, 2021

Uh oh!

MarcusLongmuir commented Feb 5, 2021

Uh oh!

johanbrandhorst commented Feb 5, 2021

Uh oh!

johanbrandhorst commented Mar 13, 2021

Uh oh!

johanbrandhorst commented Mar 16, 2021

Uh oh!

MarcusLongmuir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

johanbrandhorst commented Mar 17, 2021

Uh oh!

MarcusLongmuir commented Mar 17, 2021

Uh oh!

MarcusLongmuir commented Mar 17, 2021

Uh oh!

johanbrandhorst commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants