Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

deps: Update protbuf to v23.1 #28075

Merged
merged 18 commits into from
Jul 10, 2023
Merged

Conversation

yanavlasov
Copy link
Contributor

@yanavlasov yanavlasov commented Jun 21, 2023

New patches:

  1. PGV to work with handling of new C++20 keywords added in protobuf (upstreaming change to PGV now)
  2. absl patch to exclude stack symbolization for WASM modules. The implementation is buggy and causes existing WASM filter tests to fail. This is being fixed in emscripten SDK and absl env.emscripten_stack_snapshot not available in STANDALONE emscripten-core/emscripten#19753
  3. Compilation issues in protobuf. Fixed on protbuf main, waiting for a new version to be cut

Breaking changes:

  • Substitution formatter of the filter dynamic metadata no longer supports NaN and Inf floating point numbers, producing an error messages instead of JSON object of the proto. This is deemed to be an edge case.

Fixed a bunch of change detector tests that relied on specific error messages emitted by the previous version of the protobuf JSON parser.

Some dependencies are not at the bleeding edge versions, but this PR brings Envoy code over the biggest hump and allows to upgrade to newer version with smaller changes later.

Risk Level: Low
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Part of #27757

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jun 21, 2023
@repokitteh-read-only
Copy link

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: #28075 was opened by yanavlasov.

see: more, trace.

@yanavlasov yanavlasov marked this pull request as draft June 21, 2023 19:46
@phlax
Copy link
Member

phlax commented Jun 21, 2023

patch looks good, thanks for working on this!

@yanavlasov yanavlasov force-pushed the protobuf-update branch 2 times, most recently from c050d13 to e950871 Compare June 22, 2023 15:01
@yanavlasov
Copy link
Contributor Author

Slow progress. Many dependencies are broken. Actively working on it.

@phlax
Copy link
Member

phlax commented Jun 22, 2023

possibly related protocolbuffers/protobuf#12523

(EDIT: probs not actually - i was trying to see what was going on with python headers)

@yanavlasov yanavlasov force-pushed the protobuf-update branch 3 times, most recently from 358f8e9 to 8725ec6 Compare June 22, 2023 18:19
@phlax
Copy link
Member

phlax commented Jun 22, 2023

re the python_headers im not sure we still need to alias them - but if we do i believe we can use

diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl
index fd13e73479..ac50ff6276 100644
--- a/bazel/repositories.bzl
+++ b/bazel/repositories.bzl
@@ -894,7 +894,7 @@ def _com_google_protobuf():
     # https://github.com/google/protobuf/blob/v3.6.1/util/python/BUILD#L6-L9
     native.bind(
         name = "python_headers",
-        actual = "@com_google_protobuf//util/python:python_headers",
+        actual = "@system_python//:python_headers",
     )
 
 def _io_opencensus_cpp():

@yanavlasov
Copy link
Contributor Author

There is still a ton of test failures due to a change to the error messages in protobuf JSON parser.

Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
@yanavlasov
Copy link
Contributor Author

Still about 15 tests are failing. Mobile build is broken for unknown reason. @RyanTheOptimist

Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jun 30, 2023
@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jun 30, 2023
Signed-off-by: Yan Avlasov <yavlasov@google.com>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jul 1, 2023
Signed-off-by: Yan Avlasov <yavlasov@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #28075 was synchronize by yanavlasov.

see: more, trace.

Signed-off-by: Yan Avlasov <yavlasov@google.com>
@alyssawilk
Copy link
Contributor

Looks like this one is stalled. Did we decide to merge now or wait for release? If we're waiting let's tag it as such just to keep it off the daily on-call spam, and if not let's merge today to max out production time :-)

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @yanavlasov

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jul 10, 2023
@phlax
Copy link
Member

phlax commented Jul 10, 2023

per offline discussion, landing

@phlax phlax merged commit 65273b2 into envoyproxy:main Jul 10, 2023
@yanavlasov yanavlasov deleted the protobuf-update branch July 10, 2023 17:27
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
@phlax
Copy link
Member

phlax commented Jul 11, 2023

/coverage

@repokitteh-read-only
Copy link

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/28075/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #28075 (comment) was created by @phlax.

see: more, trace.

sumitd2 pushed a commit to sumitd2/envoy that referenced this pull request Jul 12, 2023
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Sumit Dubey <sumit.dubey2@ibm.com>
phlax pushed a commit to phlax/envoy that referenced this pull request Oct 9, 2023
Signed-off-by: Yan Avlasov <yavlasov@google.com>

Signed-off-by: yanavlasov <yavlasov@google.com>
@keith
Copy link
Member

keith commented May 24, 2024

it doesn't look like this pgv patch ever made it upstream, sent it here bufbuild/protoc-gen-validate#1110

rodaine pushed a commit to bufbuild/protoc-gen-validate that referenced this pull request May 24, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants