grpc / grpc-web

gRPC for Web Clients
https://grpc.io
Apache License 2.0
8.45k stars 760 forks source link

Upgrade protobuf to 27.1 and modernize codegen using new APIs (e.g. `has_presence()`) #1445

Closed benjaminp closed 1 week ago

benjaminp commented 1 week ago

Fixes https://github.com/grpc/grpc-web/issues/1437.

sampajano commented 1 week ago

@benjaminp Wow thanks so much for help working on this!

I just ran our CI and ran into this error tho:

https://btx.cloud.google.com/invocations/d6c6aeb6-b46f-4bca-a17a-6be28b1edbb2/targets/grpc%2Fweb%2Fpresubmit/log

Not sure if it's directly related (or related to our CI setup or our other dependencies in general) though.

I could try take a closer look later.

benjaminp commented 1 week ago

I've noticed that older GCC versions give a false deprecation warning in part of protobuf, which then explodes because protobuf passes -Werror. I can throw -Wno-deprecated-declarations in .bazelrc.

sampajano commented 1 week ago

I've noticed that older GCC versions give a false deprecation warning in part of protobuf, which then explodes because protobuf passes -Werror. I can throw -Wno-deprecated-declarations in .bazelrc.

oh wow! good to note!! Thanks for the suggestion!

Do you think -Wno-error=deprecated-declarations might work too (but doesn't completely remove warning messages)?

sampajano commented 1 week ago

I've noticed that older GCC versions give a false deprecation warning in part of protobuf, which then explodes because protobuf passes -Werror. I can throw -Wno-deprecated-declarations in .bazelrc.

Hmm CI still showing same errors it seems:

https://btx.cloud.google.com/invocations/e1797645-dda6-4134-885a-6daf896213a7/targets/grpc%2Fweb%2Fpresubmit/log

benjaminp commented 1 week ago

Do you think -Wno-error=deprecated-declarations might work too (but doesn't completely remove warning messages)?

Should work, yes.

Hmm CI still showing same errors it seems:

https://btx.cloud.google.com/invocations/e1797645-dda6-4134-885a-6daf896213a7/targets/grpc%2Fweb%2Fpresubmit/log

Okay, maybe I need to also pass --host_copt as in the latest revision?

sampajano commented 1 week ago

Thanks for the update!!

Still same error tho... the bazel flag might not have been working..

Does it make things build for you?

When running this local command (on my Mac) shows me the same error as in shown in CI:

docker-compose up --build prereqs

sampajano commented 1 week ago

Aha got it!!

You need to add an extra line:

COPY ./.bazelrc ./.bazelrc

below this line:

https://github.com/grpc/grpc-web/blob/8ab32b945ca0530222593127e9431a455eff24a8/net/grpc/gateway/docker/prereqs/Dockerfile#L77

Then it should work :)


And then i think you don't need --host_copt also anymore.

benjaminp commented 1 week ago

Nice find. done

sampajano commented 1 week ago

@benjaminp Thank you SO much for the wonderful contribution here again :)

Let me know if you need a new release to be cut to be able to leverage some of the change here, and if so i'd be happy to do one soon (after July 4th tho) :)

sampajano commented 1 week ago

FYI i think this has broken the protoc-plugin image.

Can be built by:

docker-compose up --build protoc-plugin

Error logs (internal):

 /bin/mkdir -p '/usr/local/bin'
  /bin/bash ../libtool   --mode=install /usr/bin/install -c protoc '/usr/local/bin'
libtool: install: /usr/bin/install -c .libs/protoc /usr/local/bin/protoc
make[2]: Leaving directory '/github/grpc-web/third_party/protobuf/src'
make[1]: Leaving directory '/github/grpc-web/third_party/protobuf/src'
Removing intermediate container f2b2b179b086
 ---> d990ba2c668c
Step 6/6 : RUN cd ./javascript/net/grpc/web/generator &&   make protoc-gen-grpc-web
 ---> Running in 52e698a00ba2
g++ -std=c++11 -I/usr/local/include -pthread  -c -o grpc_generator.o grpc_generator.cc
grpc_generator.cc: In function 'void grpc::web::{anonymous}::PrintProtoDtsMessage(google::protobuf::io::Printer*, const google::protobuf::Descriptor*, const google::protobuf::FileDescriptor*)':
grpc_generator.cc:869:42: error: 'const class google::protobuf::Descriptor' has no member named 'real_oneof_decl'; did you mean 'oneof_decl'?
  869 |     const OneofDescriptor *oneof = desc->real_oneof_decl(i);
      |                                          ^~~~~~~~~~~~~~~
      |                                          oneof_decl
make: *** [<builtin>: grpc_generator.o] Error 1
Service 'protoc-plugin' failed to build: The command '/bin/sh -c cd ./javascript/net/grpc/web/generator &&   make protoc-gen-grpc-web' returned a non-zero code: 2

Maybe somehow the third_party/protobuf module update hasn't taken effect?

sampajano commented 1 week ago

I've temporarily protoc-plugin in CI in https://github.com/grpc/grpc-web/pull/1447.

Would like to have that fixed soon tho!

@benjaminp If you could help take a look it'll be appreciated too!

My macbook is showing some unrelated errors but i think this error can probably be reproduced by:

docker-compose up --build protoc-plugin


UPDATE:

This will (probably) be fixed by https://github.com/grpc/grpc-web/pull/1446 :):)