[grpc/protobuf] Update grpc to 1.65.5 and update protobuf to 5.26.1 #39800

Tradias · 2024-07-09T08:43:59Z

Fixes
#37440
#29697
#38316
#37729
#30583
#40803

Upstream issues:
Ultimaker/libArcus#156
apache/brpc#2757
sogou/srpc#406

Update to latest protobuf isn't possible because gRPC latest does not support it and it is difficult to patch (unstable upb API being the primary reason).

Some ports had to be patched to support protobuf v5.

upb now has (hopefully) proper stage0 build to generate descriptor.upb.h/c files. Seeing that the supposedly tested-for-staleness files here: https://github.com/protocolbuffers/protobuf/tree/v25.1/upb/cmake/google/protobuf are actually stale.

Changes comply with the maintainer guide.
SHA512s are updated for each updated download.
The "supports" clause reflects platforms that may be fixed by this new version.
Any fixed CI baseline entries are removed from that file.
Any patches that are no longer applied are deleted from the port's directory.
The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
Only one version is added to each modified port's versions file.

Tradias · 2024-07-12T09:47:45Z

@MonicaLiu0311 ready for review.

MonicaLiu0311 · 2024-07-15T02:33:57Z

grpc & protobuf & upb

All features are tested successfully in the following triplet:

x86-windows
x64-windows
x64-windows-static

openvino

Features onnx,paddle are tested successfully in the following triplet:

x64-windows
x64-windows-static

MonicaLiu0311 · 2024-07-15T02:34:51Z

@Tradias Please resolve the conflict.

JavierMatosD

Partial Review:

This PR is claiming to update grpc and protobuf, but I am also seeing updates for:

utf8-range
upb
srpc
openvino
brpc
arcus

Why are these changes needed for the version update? If they are not, please split apart into their own respective PRs. I'm not saying that the changes here wouldn't be accepted just that they should be reviewed on their own.

Update to latest protobuf isn't possible because gRPC latest does not support it and it is difficult to patch (unstable upb API being the primary reason).

Can you explain why grpc doesn't support latest protobuf? i.e., did grpc drop support for protobuf or is this an upstream bug? In general, we don't accept patches that add functionality that isn't present in upstream. That is, if grpc doesn't support latest protobuf and that is by design then we will not add the functionality through vcpkg. We typically only accept version updates to latest barring unique circumstances so this needs a bit more explanation.

In general, there are a lot of modifications to patches here. I need a bit of context as to why you are making those changes and whether or not they have been reported to upstream. Overall, these changes look fine, but I want to make sure we aren't creating more maintenance burden here by adding support for stuff that upstream by design does not support.

JavierMatosD · 2024-07-15T19:45:19Z

ports/brpc/protobuf.patch

@@ -1,5 +1,262 @@
+diff --git a/src/brpc/esp_message.cpp b/src/brpc/esp_message.cpp


Can you explain where this patch is coming from? What problem does it solve and why is it needed to update grpc and protobuf? In general, this patch looks upstream-able.

It addresses the breaking changes caused by the protobuf update from v4 to v5. It can certainly be upstreamed.

JavierMatosD · 2024-07-15T19:47:04Z

ports/arcus/0001-fix-protobuf-deprecated.patch

+     ErrorCollector() : _error_count(0) { }
+
+-    void AddError(const std::string& filename, int line, int column, const std::string& message) override
+    void RecordError(absl::string_view filename, int line, int column, absl::string_view message) override


Please explain why this patch is needed. This looks like something that is upstream-able.

It addresses the breaking changes caused by the protobuf update from v4 to v5. It can certainly be upstreamed.

@JavierMatosD It's been a long time. Have you noticed?

Tradias · 2024-07-16T12:05:28Z

@JavierMatosD Thank you for looking into this pr (again). The story is very similar to the previous one #35781. We are trying to force a protobuf version onto libraries that don't support it (yet) or are unmaintained. I haven't created any reports upstream and honestly don't feel like doing it, so feel free to do so.

protobuf, utf8-range and upb all come from the same repository. I think it would be best to avoid updating one without the others. That also means that we cannot update to latest protobuf (which is support by gRPC) because the latest upb is not.

pedrolamarao · 2024-08-28T19:33:53Z

Thank you for your work on upgrading gRPC!
Is there anything the community can do to help this PR move forward?

Tradias · 2024-09-04T12:06:42Z

Upstream "support protobuf v5" issues:

Ultimaker/libArcus#156
apache/brpc#2757
sogou/srpc#406

AlangabSistemi · 2024-09-06T07:19:24Z

When will this change be accepted? I need uds in windws (grpc>=1.63)

pedrolamarao · 2024-09-17T17:11:59Z

I have started to experiment with this branch to prepare my project for this upgrade.

It seems that I have hit a known problem with protobuf.

P:\dev\keys-native\build\vcpkg_installed\x64-windows-static-md\include\google\protobuf\map_field.h(682,66): error C2370
: 'google::protobuf::internal::MapField<Derived,Key,T,kKeyFieldType_,kValueFieldType_>::kVTable': redefinition; differe
nt storage class [P:\dev\keys-native\build\protobuf\keys-protobuf.vcxproj]
P:\dev\keys-native\build\vcpkg_installed\x64-windows-static-md\include\google\protobuf\map_field.h(682,66): error C2370
: 'google::protobuf::internal::MapField<Derived,Key,T,kKeyFieldType_,kValueFieldType_>::kVTable': redefinition; differe
nt storage class [P:\dev\keys-native\build\protobuf\keys-protobuf.vcxproj]
  (compiling source file 'src/main/proto/keys.pb.cc')
      P:\dev\keys-native\build\vcpkg_installed\x64-windows-static-md\include\google\protobuf\map_field.h(671,37):
      see declaration of 'google::protobuf::internal::MapField<Derived,Key,T,kKeyFieldType_,kValueFieldType_>::kVTable'

  (compiling source file 'src/main/proto/keys.grpc.pb.cc')
      P:\dev\keys-native\build\vcpkg_installed\x64-windows-static-md\include\google\protobuf\map_field.h(671,37):
      see declaration of 'google::protobuf::internal::MapField<Derived,Key,T,kKeyFieldType_,kValueFieldType_>::kVTable'

This seems the same symptom as #39459

Tradias · 2024-09-18T15:59:23Z

@pedrolamarao You describe an issue that exists in the protobuf version on master. In the linked vcpkg issue you will find the protobuf commit that fixed it and if you check it out you can see that the fix made it into the protobuf version that this pull request introduces protocolbuffers/protobuf@f78f9c5. I also just compiled a small program with vcpkg master branch (failed as expected) and then with this branch, succeeded.

In other words, it looks like you did something wrong when trying to test this branch and instead used current master branch. Possible causes:

Forgot to remove builtin-baseline from your vcpkg.json file
VCPKG_ROOT env value is pointing at a different clone of vcpkg instead of the one where you checked out this branch

pedrolamarao · 2024-09-18T22:16:33Z

@Tradias thanks, that sounds likely, sorry for the noise.

korDen · 2024-09-19T21:49:27Z

What needs to be done to move this forward? @JavierMatosD are you able to do a second pass?

MonicaLiu0311 · 2024-09-20T03:00:34Z

@BillyONeal Please take a look at this PR.

FAKERINHEART · 2024-09-26T06:06:06Z

hey, buddies when will it be merged?

JavierMatosD · 2024-09-26T23:23:33Z

@BillyONeal, @vicroms, @data-queue discussed this today.

@BillyONeal - "protobuf and upb are owned by the same people, we should be picking versions that are intrinsically compatible with each other. If that means we can't update grpc, so be it. We are not upb, grpc, or protobuf maintainers and we are not qualified to review that. We have to pick some form of versioning/packaging story that preserves that and if it means we can't update that then we can't update that. We would need to file issues with upstream and get them to implement the fix."

@ras0219-msft - "It is a reimplementation of protobuf that is small, so it has independent implementation from protobuf but it is c, small, and unstable. It sounds like upb is not suitable for consumption. We should declare that upb only exists as a dependency of grpc and treated as such. Maybe the path forward is protobuf and upb become decoupled and upb and grpc become coupled."

@BillyONeal - "Maybe upb should be delisted"

@ras0219-msft - "Why was upb added in the first place?"

#8460 (comment) -> implies it is needed for grpc

@ras0219-msft - "grpc has their own vendor copies of all these dependencies. So presumably, they have a set of dependencies that works for them. We could just say that grpc owns this whole ecosystem and we use whatever versions grpc needs."

@BillyONeal - "That seems to be the thing that people want."

@BillyONeal - "We should delist upb. https://github.com/protocolbuffers/protobuf/tree/main/upb -> "For this reason, upb is not generally offered as a C library for direct consumption, and there are no releases."

Actions:

Delist upb and instruct grpc to use its vendored copy
Use the actual change that srpc accepted or even better update srpc instead of patching.

Questions:

Is there a newer version of grpc that still uses the 4.x series of protobuf? See, https://protobuf.dev/support/version-support/. If so, we could do an update to that while we wait for other ports to catch up.

Since upstream has been notified approx 3 ago, we will give them another 3 weeks to respond before we start delisting ports.

Tradias · 2024-09-27T07:15:42Z

@JavierMatosD Great conversation. This matches my knowledge of grpc, protobuf, upb ecosystem exactly. I would throw utf8-range into the mix too but I think it is OK to keep it as is for now. I have been advocating for removing upb for a while now. Seeing that I have been doing the latest gRPC updates, I know how painful it is to patch CMake support into upb and patch gRPC to use it; two things that upstream never intended for.

GRPC v1.62.3 seems to be the last version to use protobuf 4.x: Look at the commit message on protobuf submodule between https://github.com/grpc/grpc/tree/v1.62.3/third_party with https://github.com/grpc/grpc/tree/v1.63.2/third_party

anders-wind · 2024-10-08T10:07:57Z

clang-19 fails at compiling grpc on (1.60.0, vcpkg master) (due to a compiler warning)
This is however fixed (at least) in grpc 1.65.4 (see grpc/grpc#36805).
We would greatly appreciate an update.

olejniczak · 2024-10-10T11:25:23Z

Hi, it is great job, but the question is: when could it be merged? @JavierMatosD are all your requested changes already done?

JavierMatosD · 2024-10-10T17:19:08Z

@Tradias, please mark this PR "ready for review" when you want me to review this again.

Tradias · 2024-10-10T17:26:48Z

@JavierMatosD what is the verdict regarding upb now? Are we merging this pr first and then delist it?

korDen · 2024-10-15T18:26:05Z

Perhaps mark the PR as ready for review, otherwise it may not get the attention.

JavierMatosD · 2024-10-16T17:43:42Z

@Tradias, sorry for the late response. I believe the action items requested by the team last we spoke were:

Delist upb and instruct grpc to use its vendored copy
Use the accepted change in srpc or update it to include the desired changes.

We also agreed to give upstream 3 weeks to respond before we begin delisting ports. I believe it's been 3 weeks since libarcus, brpc, and srpc were notified.

…. Add proper stage0 to upb

… srpc

Tradias · 2024-10-20T19:58:16Z

GRPC now uses built-in upb, tested successfully on x64-windows-static. Note that some patching is still needed to make it use the vcpkg utf8-range port.

SRPC now uses official patch, build successfully on x64-windows-static.

BRPC is also working on protobuf v5 support but I am waiting for it to be merged: apache/brpc#2782

FrankXie05 assigned MonicaLiu0311 Jul 9, 2024

MonicaLiu0311 added the category:port-update The issue is with a library, which is requesting update new revision label Jul 9, 2024

Tradias marked this pull request as ready for review July 12, 2024 09:46

Tradias mentioned this pull request Jul 12, 2024

[protobuf] update to 5.27.1 #39515

Open

MonicaLiu0311 added requires:testing Needs tests added before merging and removed requires:testing Needs tests added before merging labels Jul 12, 2024

Tradias force-pushed the grpc-1-65-0 branch from e707ef6 to 3e13edc Compare July 15, 2024 07:00

JavierMatosD requested changes Jul 15, 2024

View reviewed changes

JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jul 16, 2024

Tradias force-pushed the grpc-1-65-0 branch from 3e13edc to 48fced3 Compare July 26, 2024 07:22

Tradias changed the title ~~[grpc/protobuf] Update grpc to 1.65.0 and update protobuf to 5.26.1~~ [grpc/protobuf] Update grpc to 1.65.2 and update protobuf to 5.26.1 Jul 27, 2024

Tradias force-pushed the grpc-1-65-0 branch 2 times, most recently from 72241a3 to 9240d3c Compare August 2, 2024 18:47

Tradias changed the title ~~[grpc/protobuf] Update grpc to 1.65.2 and update protobuf to 5.26.1~~ [grpc/protobuf] Update grpc to 1.65.4 and update protobuf to 5.26.1 Aug 2, 2024

Tradias force-pushed the grpc-1-65-0 branch from 9240d3c to ea222cd Compare August 15, 2024 06:20

Cheney-W mentioned this pull request Sep 5, 2024

[grpc] update to v1.63 #40803

Open

JavierMatosD marked this pull request as draft September 26, 2024 23:23

ddelgrosso1 mentioned this pull request Oct 1, 2024

Cleanup (storage): Remove gRPC version guards from async_throughput_benchmark.cc googleapis/google-cloud-cpp#14750

Open

Tradias force-pushed the grpc-1-65-0 branch from 052b06a to f566184 Compare October 10, 2024 12:44

Tradias added 9 commits October 19, 2024 09:29

grpc: Update to 1.65.0

f9e8e8f

protobuf: Patch/update arcus, brpc, openvino. Fix grpc otel pkgconfig…

71f5554

…. Add proper stage0 to upb

upb: Fix using host tools

82db8e3

grpc: Update to 1.65.2

5ece5f6

upb: Integrate upb de-vendoring changes

72acc1f

protobuf: Adjust fix-arm64-msvc patch to v5.26.1

39fe1a4

grpc: Update to 1.65.4

9056a29

brpc: Fix protobuf patch for version 1.10.0

9c39b7b

openvino: Update protobuf patch to 2024.4

c0ee4b6

Tradias force-pushed the grpc-1-65-0 branch from f566184 to 809e731 Compare October 19, 2024 08:22

Tradias changed the title ~~[grpc/protobuf] Update grpc to 1.65.4 and update protobuf to 5.26.1~~ [grpc/protobuf] Update grpc to 1.65.5 and update protobuf to 5.26.1 Oct 19, 2024

grpc: No longer use upb port. Also use official protobuf-v5 patch for…

f66ed8a

… srpc

Tradias force-pushed the grpc-1-65-0 branch from 809e731 to f66ed8a Compare October 19, 2024 13:09

Tradias marked this pull request as ready for review October 20, 2024 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grpc/protobuf] Update grpc to 1.65.5 and update protobuf to 5.26.1 #39800

[grpc/protobuf] Update grpc to 1.65.5 and update protobuf to 5.26.1 #39800

Tradias commented Jul 9, 2024 •

edited by Cheney-W

Loading

Tradias commented Jul 12, 2024

MonicaLiu0311 commented Jul 15, 2024

MonicaLiu0311 commented Jul 15, 2024

JavierMatosD left a comment

JavierMatosD Jul 15, 2024

Tradias Jul 26, 2024 •

edited

Loading

JavierMatosD Jul 15, 2024

Tradias Jul 26, 2024

Benjamin-Miller1 Aug 13, 2024

Tradias commented Jul 16, 2024

pedrolamarao commented Aug 28, 2024

Tradias commented Sep 4, 2024

AlangabSistemi commented Sep 6, 2024

pedrolamarao commented Sep 17, 2024

Tradias commented Sep 18, 2024

pedrolamarao commented Sep 18, 2024

korDen commented Sep 19, 2024

MonicaLiu0311 commented Sep 20, 2024

FAKERINHEART commented Sep 26, 2024 •

edited

Loading

JavierMatosD commented Sep 26, 2024

Tradias commented Sep 27, 2024

anders-wind commented Oct 8, 2024 •

edited

Loading

olejniczak commented Oct 10, 2024

JavierMatosD commented Oct 10, 2024

Tradias commented Oct 10, 2024

korDen commented Oct 15, 2024

JavierMatosD commented Oct 16, 2024

Tradias commented Oct 20, 2024

		@@ -1,5 +1,262 @@
		diff --git a/src/brpc/esp_message.cpp b/src/brpc/esp_message.cpp

[grpc/protobuf] Update grpc to 1.65.5 and update protobuf to 5.26.1 #39800

Are you sure you want to change the base?

[grpc/protobuf] Update grpc to 1.65.5 and update protobuf to 5.26.1 #39800

Conversation

Tradias commented Jul 9, 2024 • edited by Cheney-W Loading

Tradias commented Jul 12, 2024

MonicaLiu0311 commented Jul 15, 2024

grpc & protobuf & upb

openvino

MonicaLiu0311 commented Jul 15, 2024

JavierMatosD left a comment

Choose a reason for hiding this comment

JavierMatosD Jul 15, 2024

Choose a reason for hiding this comment

Tradias Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

JavierMatosD Jul 15, 2024

Choose a reason for hiding this comment

Tradias Jul 26, 2024

Choose a reason for hiding this comment

Benjamin-Miller1 Aug 13, 2024

Choose a reason for hiding this comment

Tradias commented Jul 16, 2024

pedrolamarao commented Aug 28, 2024

Tradias commented Sep 4, 2024

AlangabSistemi commented Sep 6, 2024

pedrolamarao commented Sep 17, 2024

Tradias commented Sep 18, 2024

pedrolamarao commented Sep 18, 2024

korDen commented Sep 19, 2024

MonicaLiu0311 commented Sep 20, 2024

FAKERINHEART commented Sep 26, 2024 • edited Loading

JavierMatosD commented Sep 26, 2024

Tradias commented Sep 27, 2024

anders-wind commented Oct 8, 2024 • edited Loading

olejniczak commented Oct 10, 2024

JavierMatosD commented Oct 10, 2024

Tradias commented Oct 10, 2024

korDen commented Oct 15, 2024

JavierMatosD commented Oct 16, 2024

Tradias commented Oct 20, 2024

Tradias commented Jul 9, 2024 •

edited by Cheney-W

Loading

Tradias Jul 26, 2024 •

edited

Loading

FAKERINHEART commented Sep 26, 2024 •

edited

Loading

anders-wind commented Oct 8, 2024 •

edited

Loading