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

benjaminp · 2024-06-24T15:43:09Z

Fixes #1437.

sampajano · 2024-06-24T21:28:41Z

@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 · 2024-06-24T21:38:33Z

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 · 2024-06-25T00:33:34Z

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 · 2024-06-25T00:34:46Z

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 · 2024-06-25T01:59:13Z

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 · 2024-06-25T02:30:40Z

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 · 2024-06-25T02:40:42Z

Aha got it!!

You need to add an extra line:

COPY ./.bazelrc ./.bazelrc

below this line:

grpc-web/net/grpc/gateway/docker/prereqs/Dockerfile

Line 77 in 8ab32b9

COPY ./WORKSPACE ./WORKSPACE

Then it should work :)

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

Fixes grpc#1437.

benjaminp · 2024-06-25T02:42:41Z

Nice find. done

sampajano

Thank you SO MUCH for this contribution.. This is wonderful.. such a great simplification across the board and I much appreciate you help us make our lives easier using these new APIs.. Thank you 😊

Only one request — to remove the --host_copt — other comments are all just thoughts :)

Thank you again :)

sampajano · 2024-06-25T02:45:52Z

.bazelrc

@@ -0,0 +1 @@
+build --copt=-Wno-error=deprecated-declarations --host_copt=-Wno-error=deprecated-declarations


I think --host_copt=-Wno-error=deprecated-declarations is no longer necessary.

@benjaminp Could you update this file? I'll re-run CI and merge this PR afterwards.. Thanks! 😊

I think it's probably safest to keep this, and there's minimal downside. If any protobuf code ends up in the Bazel exec configuration, the compilation problem will reappear.

Ok! Sure! I think you understand bazel (and protobuf for the matter) way more than me so I trust your judgement here :)

sampajano · 2024-06-25T03:06:50Z

javascript/net/grpc/web/generator/grpc_generator.cc

-    if (field->has_optional_keyword() ||
-        (field->type() == FieldDescriptor::TYPE_MESSAGE &&
-            !field->is_repeated() && !field->is_map())) {
+    if (field->has_presence()) {


what a simplification! 😊

sampajano · 2024-06-25T03:07:06Z

javascript/net/grpc/web/generator/grpc_generator.cc

+  for (int i = 0; i < desc->real_oneof_decl_count(); i++) {
+    const OneofDescriptor *oneof = desc->real_oneof_decl(i);


sampajano · 2024-06-25T03:18:03Z

javascript/net/grpc/web/generator/grpc_generator.cc

@@ -904,8 +900,7 @@ void PrintProtoDtsMessage(Printer* printer, const Descriptor* desc,
    }
    vars["js_field_name"] = js_field_name;
    vars["js_field_type"] = AsObjectFieldType(field, file);
-    if ((field->type() != FieldDescriptor::TYPE_MESSAGE && !field->has_optional_keyword()) ||


Trying to understand all the cases is literally giving me a headache.. 😅

so I asked ChatGPT if the before and after is equivalent and it says yet — so i trust it (and you) :D

I honestly wouldn't be surprised if the code before has bugs which would be fixed by the new code.

thanks so much for the simplification.. so much nicer :)

sampajano · 2024-06-25T04:00:29Z

@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 · 2024-06-25T04:49:50Z

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 · 2024-06-25T05:07:08Z

I've temporarily protoc-plugin in CI in #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 #1446 :):)

sampajano added the kokoro:run label Jun 24, 2024

grpc-kokoro removed the kokoro:run label Jun 24, 2024

benjaminp force-pushed the protobuf-27.1 branch from d3bf6ed to 60db577 Compare June 24, 2024 21:37

sampajano added the kokoro:run label Jun 25, 2024

grpc-kokoro removed the kokoro:run label Jun 25, 2024

benjaminp force-pushed the protobuf-27.1 branch from 60db577 to 52fbf35 Compare June 25, 2024 01:58

sampajano added the kokoro:run label Jun 25, 2024

grpc-kokoro removed the kokoro:run label Jun 25, 2024

Upgrade protobuf to 27.1.

577e30c

Fixes grpc#1437.

benjaminp force-pushed the protobuf-27.1 branch from 52fbf35 to 577e30c Compare June 25, 2024 02:42

sampajano added the kokoro:run label Jun 25, 2024

grpc-kokoro removed the kokoro:run label Jun 25, 2024

sampajano approved these changes Jun 25, 2024

View reviewed changes

sampajano changed the title ~~Upgrade protobuf to 27.1.~~ Upgrade protobuf to 27.1 and modernize codegen using new APIs (e.g. has_presence()) Jun 25, 2024

This was referenced Jun 25, 2024

Question: Why message types are treated as optional property? (TypeScript) #715

Closed

Improve Typescript oneof generated types #947

Open

Some types generated from oneof should always be required? #1125

Open

sampajano merged commit 2c39859 into grpc:master Jun 25, 2024

benjaminp deleted the protobuf-27.1 branch June 25, 2024 04:00

sampajano added the release notes: major label Jun 25, 2024

sampajano mentioned this pull request Jun 25, 2024

Temporarily disable protoc-plugin in CI #1447

Merged

sampajano mentioned this pull request Jun 25, 2024

protoc-plugin image is currently broken #1449

Closed

luangong mentioned this pull request Jul 18, 2024

protoc-gen-js 3.21.4 Homebrew/homebrew-core#177388

Closed

sampajano mentioned this pull request Aug 22, 2025

Bump version to 2.0.0 #1493

Merged

		@@ -0,0 +1 @@
		build --copt=-Wno-error=deprecated-declarations --host_copt=-Wno-error=deprecated-declarations

		for (int i = 0; i < desc->real_oneof_decl_count(); i++) {
		const OneofDescriptor *oneof = desc->real_oneof_decl(i);

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

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

Uh oh!

Conversation

benjaminp commented Jun 24, 2024

Uh oh!

sampajano commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminp commented Jun 24, 2024

Uh oh!

sampajano commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sampajano commented Jun 25, 2024

Uh oh!

benjaminp commented Jun 25, 2024

Uh oh!

sampajano commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sampajano commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminp commented Jun 25, 2024

Uh oh!

sampajano left a comment

Choose a reason for hiding this comment

Uh oh!

sampajano Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

sampajano Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

benjaminp Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

sampajano Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

sampajano Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

sampajano Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

sampajano Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

sampajano commented Jun 25, 2024

Uh oh!

sampajano commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sampajano commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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

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

sampajano commented Jun 24, 2024 •

edited

Loading

sampajano commented Jun 25, 2024 •

edited

Loading

sampajano commented Jun 25, 2024 •

edited

Loading

sampajano commented Jun 25, 2024 •

edited

Loading

sampajano commented Jun 25, 2024 •

edited

Loading

sampajano commented Jun 25, 2024 •

edited

Loading