Update protoconverter patch

[asedeno]

This serves to decouple protoconverter from the protobuf version it was forked off of.

These changes should really be upstream, but upstream is experiencing technical issues taking patches right now, so we'll carry them here until those are resolved.

Risk Level: low
Testing: CI

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #36290 was opened by asedeno.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #36290 was opened by asedeno.

see: more, trace.

[asedeno]

This is one of the roadblocks for updating our C++ Protobuf dependency.

[mattklein123]

Do you have any more context on this? This is a huge patch. What was actually changed here?

[asedeno]

When protoconverter forked off of protobuf, it brought with it things it should not have. The crippling one was port_def.h, which is a protobuf internal header that among other things defines of PROTOBUF_VERSION, errors out if it finds it is already defined, and checks for a minimum version of GOOGLE_PROTOBUF_VERSION.
https://github.com/grpc-ecosystem/proto-converter/blob/main/src/google/protobuf/util/converter/port_def.inc#L224-L227

Since the fork, this file has changed in protobuf, and importantly, the definition of PROTOBUF_VERSION moved to runtime_version.h.

This parch is large, yes, but also very negative. Some of it is already in upstream, but not exported to github due to technical issues.

The patch removes these files:

• common.h
• common.cc
• common_unittests.cc
• port_def.inc
• port_undef.inc

Additionally, it:

• Removes #includes for the .h and .inc files that were removed.
• Removes the use of the PROTOBUF_EXPORT macro.
• Replaces PROTOBUF_PREDICT_FALSE with ABSL_PREDICT_FALSE

And in a few places it reformats things slightly. I tried to filter these formatting-only changes out, but some crept in.

[asedeno]

I've gone ahead and moved the file deletions to separate patch_cmds to make the patch itself a little easier to digest.

[asedeno]

/retest

[asedeno]

/retest

[asedeno]

Okay, I don't know what's up with Envoy/Publish and verify.

Extracting Bazel installation...
Starting local Bazel server and connecting to it...
INFO: Invocation ID: 51df4730-5be8-4a3b-8e97-52d8f99ea0ce
ERROR: io.grpc.StatusRuntimeException: UNAUTHENTICATED

[phlax]

I don't know what's up with Envoy/Publish and verify.

that should be fixed now - i broke it earlier and landed a patch soon after

[asedeno]

@phlax, when I merged in main it had all your recent changes up to fcfae60.

[phlax]

argh - another 8( - ill fix it now ...

[phlax]

should be half fixed - will follow up asap when i figure out the issue

[asedeno]

Carving off more of #36167.

[asedeno]

Nope, still fails.

2024/09/24 21:16:11 Downloading https://releases.bazel.build/6.5.0/release/bazel-6.5.0-linux-x86_64...
Extracting Bazel installation...
Starting local Bazel server and connecting to it...
INFO: Invocation ID: 3dafc16c-40a1-4dc6-8559-8708e30f4d82
ERROR: io.grpc.StatusRuntimeException: UNAUTHENTICATED

[phlax]

im debugging it atm - but its taking a while

[asedeno]

Okay, we're good to go on CI again. Anything else on your end, @mattklein123?

[phlax]

im really concerned about the amount of patching going on here - if we are going to land this there needs to be a strong justification for doing so

my 2 questions are

• do we need to update this dep right now
• can we upstream these changes instead

[asedeno]

We can't update protobuf without landing this because protoconverter upstream is currently tightly coupled to protobuf <27 and builds with newer protobuf will fail. It doesn't need to be nor should it be this tightly coupled.

We can't upstream these changes until upstream fixes their export process from Google's internal repo. Or, rather, I can upstream them but it won't do us any good here until the export is fixed. In fact, some of these changes have already landed but have not been exported.

[phlax]

if its to update protobuf - we arent going to do that until after the release anyway - so probably i would say take this one slowly and try to get the upstream issues resolved in the meantime

[asedeno]

I assume you mean 1.32.0, and not the 1.31.2 that just happened?

Trying to fix upstream's issues is a parallel track I am working on, yes.

[phlax]

yeah 1.32 is due in the next few weeks

[asedeno]

I've attempted to fix protoconverter upstream, but it has gotten itself tangled up with things marked google-internal and is currently not exportable in a way that builds.

[mattklein123]

We can merge this to unblock things as long as you are still working towards fixing the upstream issues. Thank you!

[asedeno]

Thanks @mattklein123; to alleviate concerns, as soon as upstream export issues are fixed it is my intention that we be able to import this with no patches.