Skip to content

dependencies: update protobuf to 3.8.0#7510

Merged
htuch merged 6 commits intoenvoyproxy:masterfrom
asraa:protocolbufferpatch
Jul 10, 2019
Merged

dependencies: update protobuf to 3.8.0#7510
htuch merged 6 commits intoenvoyproxy:masterfrom
asraa:protocolbufferpatch

Conversation

@asraa
Copy link
Copy Markdown
Contributor

@asraa asraa commented Jul 9, 2019

In addition to updating protobuf to 3.8.0, this PR also

  1. Removes old protobuf patch now included in 3.8.0
  2. Patches fix a UBSAN warnings. protocolbuffers/protobuf#6333 that fixes a UBSAN error in the protobuf library.
  3. Patches protobuf's BUILD to depend on foreign_cc zlib

Risk level: low/medium
Testing: bazel test //test/...

asraa added 5 commits July 8, 2019 10:51
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
@moderation
Copy link
Copy Markdown
Contributor

With 3.8.0 changing the zlib handling again and your changes in this PR I'm wondering if zlib is required in bazel/repository_locations.bzl any more? https://github.com/envoyproxy/envoy/blob/master/bazel/repository_locations.bzl#L141-L148

@asraa
Copy link
Copy Markdown
Contributor Author

asraa commented Jul 9, 2019

With 3.8.0 changing the zlib handling again and your changes in this PR I'm wondering if zlib is required in bazel/repository_locations.bzl any more? https://github.com/envoyproxy/envoy/blob/master/bazel/repository_locations.bzl#L141-L148

Hm, I saw your comment in the bzl file. Is zlib required besides as a dependency for protobuf? If not, I think you're right, perhaps it's not needed and we can use protobuf_deps() instead.
https://github.com/protocolbuffers/protobuf/blob/master/protobuf_deps.bzl

@moderation
Copy link
Copy Markdown
Contributor

Hm, I saw your comment in the bzl file. Is zlib required besides as a dependency for protobuf? If not, I think you're right, perhaps it's not needed and we can use protobuf_deps() instead.
https://github.com/protocolbuffers/protobuf/blob/master/protobuf_deps.bzl

I think that the compressor, decompressor and quiche use zlib.

If it's possible to have these reference the zlib brought in for protobuf it would mean one less dependency to maintain but it would also wouldn't be as clear.

@htuch
Copy link
Copy Markdown
Member

htuch commented Jul 9, 2019

I think we should stick with our own zlib rather than anything from protobuf, since we might want to rev versions independently. Also, I think foreign_cc is a bit cleaner than the artisinal BUILD file in protobuf third_party.

@htuch
Copy link
Copy Markdown
Member

htuch commented Jul 9, 2019

@asraa the api CI failure looks like it's potentially related to this change, maybe the precision or handling of floats has somehow changed in protobuf? You might want to update the test.

@htuch htuch added the waiting label Jul 9, 2019
Signed-off-by: Asra Ali <asraa@google.com>
@moderation
Copy link
Copy Markdown
Contributor

moderation commented Jul 10, 2019

Timed out on macOS network listener tests

//test/common/network:listener_impl_test                                TIMEOUT in 301.0s
  /private/var/tmp/_bazel_vsts/c4c3c293a43c83b45613c302d61e9e23/execroot/envoy/bazel-out/darwin-fastbuild/testlogs/test/common/network/listener_impl_test/test.log
//test/common/network:udp_listener_impl_test                            TIMEOUT in 301.2s
  /private/var/tmp/_bazel_vsts/c4c3c293a43c83b45613c302d61e9e23/execroot/envoy/bazel-out/darwin-fastbuild/testlogs/test/common/network/udp_listener_impl_test/test.log

Flake? We need to document the Azure test commands.

@repokitteh-read-only
Copy link
Copy Markdown

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #7510 (comment) was created by @moderation.

see: more, trace.

@moderation
Copy link
Copy Markdown
Contributor

/azp run envoy-macos

@azure-pipelines
Copy link
Copy Markdown

Commenter does not have sufficient privileges for PR 7510 in repo envoyproxy/envoy

@htuch
Copy link
Copy Markdown
Member

htuch commented Jul 10, 2019

/azp run envoy-macos

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@moderation
Copy link
Copy Markdown
Contributor

LGTM

@htuch htuch merged commit 8246167 into envoyproxy:master Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants