Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

deps: update protobuf to 3.14 #14253

Merged
merged 20 commits into from Dec 16, 2020
Merged

deps: update protobuf to 3.14 #14253

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5313dc7
update protobuf
kyessenov Dec 3, 2020
919e1cd
revert googleapis change; it is also breaking :(
kyessenov Dec 3, 2020
18691fb
typo
kyessenov Dec 3, 2020
6e8acda
code review
kyessenov Dec 3, 2020
5a51bdc
fix tests
kyessenov Dec 4, 2020
2fe0b36
testing
kyessenov Dec 4, 2020
c113ef7
testing
kyessenov Dec 4, 2020
494314b
fix deps
kyessenov Dec 4, 2020
b34523b
patch protobuf rule
kyessenov Dec 4, 2020
0a1acbe
Merge remote-tracking branch 'upstream/master' into protobuf_update
kyessenov Dec 7, 2020
233b6d9
relax tests
kyessenov Dec 7, 2020
de32d28
try windows debug
kyessenov Dec 8, 2020
7e73763
Revert "try windows debug"
kyessenov Dec 8, 2020
1b37116
missing include
kyessenov Dec 9, 2020
8eab565
missing include
kyessenov Dec 9, 2020
722db6d
missing include
kyessenov Dec 9, 2020
dc33299
hack to get rid of bad files
kyessenov Dec 9, 2020
d961c83
debug CI
lizan Dec 10, 2020
f01d9f1
update go build
kyessenov Dec 11, 2020
c8ef105
update udpa
kyessenov Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 10 additions & 3 deletions api/bazel/repository_locations.bzl
View file
Open in desktop
Expand Up @@ -14,12 +14,19 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "protoc-gen-validate (PGV)",
project_desc = "protoc plugin to generate polyglot message validators",
project_url = "https://github.com/envoyproxy/protoc-gen-validate",
version = "278964a8052f96a2f514add0298098f63fb7f47f",
sha256 = "e368733c9fb7f8489591ffaf269170d7658cc0cd1ee322b601512b769446d3c8",
version = "1bcea29601b5624234a19b3d7f0ebd9e9984f583",
sha256 = "2062bbe50eddf3c98490339721fb02b5b5cd78f610f163b98bbf95ba7105553f",
strip_prefix = "protoc-gen-validate-{version}",
urls = ["https://github.com/envoyproxy/protoc-gen-validate/archive/{version}.tar.gz"],
release_date = "2020-06-08",
release_date = "2020-11-30",
use_category = ["api"],
implied_untracked_deps = [
"com_github_iancoleman_strcase",
"com_github_lyft_protoc_gen_star",
"com_github_spf13_afero",
"org_golang_google_genproto",
"org_golang_x_text",
],
),
com_github_cncf_udpa = dict(
project_name = "Universal Data Plane API",
Expand Down
20 changes: 19 additions & 1 deletion bazel/dependency_imports.bzl
View file
Open in desktop
Expand Up @@ -16,7 +16,7 @@ load("@thrift_pip3//:requirements.bzl", thrift_pip_install = "pip_install")
load("@rules_antlr//antlr:deps.bzl", "antlr_dependencies")

# go version for rules_go
GO_VERSION = "1.14.7"
GO_VERSION = "1.15.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

1.15.6 is out but I can bump this in a follow up


def envoy_dependency_imports(go_version = GO_VERSION):
rules_foreign_cc_dependencies()
Expand Down Expand Up @@ -57,6 +57,24 @@ def envoy_dependency_imports(go_version = GO_VERSION):
sum = "h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=",
version = "v0.3.0",
)
go_repository(
name = "com_github_spf13_afero",
importpath = "github.com/spf13/afero",
sum = "h1:8q6vk3hthlpb2SouZcnBVKboxWQWMDNF38bwholZrJc=",
version = "v1.3.4",
)
go_repository(
name = "com_github_lyft_protoc_gen_star",
importpath = "github.com/lyft/protoc-gen-star",
sum = "h1:sImehRT+p7lW9n6R7MQc5hVgzWGEkDVZU4AsBQ4Isu8=",
version = "v0.5.1",
)
go_repository(
name = "com_github_iancoleman_strcase",
importpath = "github.com/iancoleman/strcase",
sum = "h1:ux/56T2xqZO/3cP1I2F86qpeoYPCOzk+KF/UH/Ar+lk=",
version = "v0.0.0-20180726023541-3605ed457bf7",
)

config_validation_pip_install()
configs_pip_install()
Expand Down
14 changes: 14 additions & 0 deletions bazel/protobuf.patch
View file
Open in desktop
Expand Up @@ -21,3 +21,17 @@ index efc3d8e7f..746ad4851 100644

################################################################################
# Protobuf Runtime Library
diff --git a/python/google/protobuf/__init__.py b/python/google/protobuf/__init__.py
index 97ac28028..8b7585d9d 100644
--- a/python/google/protobuf/__init__.py
+++ b/python/google/protobuf/__init__.py
@@ -31,3 +31,9 @@
# Copyright 2007 Google Inc. All Rights Reserved.

__version__ = '3.14.0'
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to maintain a patch or can we get a change landed upstream to avoid this? CC @envoyproxy/dependency-shepherds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I don't quite understand what's the deal with google dependencies, python, and bazel. If someone who understands it better could pick it up, I'd appreciate it. I only see it at the level that this was there before, and it's gone, and it works with it.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is from protocolbuffers/protobuf@66e3562. @dlj-NaN do you know why that PR would have caused google.api imports to start breaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing to keep in mind is that I can't update googleapis because some python rules are being removed (I need to find out why). So there's that.

Copy link
Member

Choose a reason for hiding this comment

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

OK. I don't think any of our build experts has a much better grasp of the Python/Bazel/GoogleAPIs internals, I'd probably start from first principles debugging :(

Copy link

Choose a reason for hiding this comment

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

protocolbuffers/protobuf@66e3562 gets pretty deep into the semantics of Python imports and different packaging. The interaction addressed by that commit is very tricky, and may be hidden by varying the order of imports.

(I'm guessing the reason this appeared with a protobuf commit was exactly due to order of imports... google.auth comes after google.api alphabetically, but due to transitive imports, google.protobuf is effectively a very early import in many scenarios.)

The upshot is that there shouldn't be any files named google/__init__.py in other installed packages. Just as a first guess, this might be caused by installing googleapis/google-auth-library-python as an egg (maybe via setup.py install?), but other google packages using native namespaces. (This also gets into what Bazel does in its pip rules....)

Anyhow, some fixes might be:

  1. Remove google/__init__.py from googleapis/google-auth-library-python. (@busunkim96 to comment on that.)
  2. Pre-install the package with an up-to-date version of pip, which should install using the native namespace.
  3. Update Bazel's pip rules to make sure it does (2).

Copy link

Choose a reason for hiding this comment

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

Remove google/init.py from googleapis/google-auth-library-python.

Removing google/__init__.py from the google-auth will probably be done next quarter as we officially drop 2.7. google-auth is last on the list since it is transitively required by nearly all the cloud client libraries.

Over the past year we've gradually moved google-cloud-API packages to using native namespace packages (example: google-cloud-texttospeech) while other libraries like google-api-core, google-cloud-core, and google-auth have kept google/__init__.py with pkg_resources style namespace packages. I see now that setuptools does not recommend this, but we have not seen customers open issues about it. Our docs always point folks pip install packages.

One thing to keep in mind is that I can't update googleapis because some python rules are being removed (I need to find out why). So there's that.

Are you referring to Bazel rules in https://github.com/googleapis/googleapis or something else?

I don't have much context here so I'm not sure it matters, but the google-cloud-* libraries get their copy of many commonly used protos (google.api, google.type, ...) from https://github.com/googleapis/python-api-common-protos, published on PyPI as google-apis-common-protos

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is related but I'm trying to land an update to how we process pip dependencies using the latest rules_python at #14329

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened an issue for further discussion #14412.
We're using googleapis/googleapis here because some protos are missing from https://github.com/googleapis/python-api-common-protos (e.g. google.api.expr for CEL). Does this mean we can't use PIP approach since there's no pre-generated package I am aware for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

pip handling changes with the latest rules_python - https://github.com/bazelbuild/rules_python/releases/tag/0.1.0. PR #14329 that moves us to that handling is blocked due to Windows (/cc @envoyproxy/windows-dev). In theory pip supports installing from Git - https://pip.pypa.io/en/stable/reference/pip_install/#git but no idea if this works for the current deprecated pip method or the new method we are trying to get to.

+
+if __name__ != '__main__':
+ try:
+ __import__('pkg_resources').declare_namespace(__name__)
+ except ImportError:
+ __path__ = __import__('pkgutil').extend_path(__path__, __name__)
18 changes: 9 additions & 9 deletions bazel/repository_locations.bzl
View file
Open in desktop
Expand Up @@ -15,10 +15,10 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "Gazelle",
project_desc = "Bazel BUILD file generator for Go projects",
project_url = "https://github.com/bazelbuild/bazel-gazelle",
version = "0.21.1",
sha256 = "cdb02a887a7187ea4d5a27452311a75ed8637379a1287d8eeb952138ea485f7d",
version = "0.22.2",
sha256 = "b85f48fa105c4403326e9525ad2b2cc437babaa6e15a3fc0b1dbab0ab064bc7c",
urls = ["https://github.com/bazelbuild/bazel-gazelle/releases/download/v{version}/bazel-gazelle-v{version}.tar.gz"],
release_date = "2020-05-28",
release_date = "2020-10-02",
use_category = ["build"],
),
bazel_toolchains = dict(
Expand Down Expand Up @@ -495,12 +495,12 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "Protocol Buffers",
project_desc = "Language-neutral, platform-neutral extensible mechanism for serializing structured data",
project_url = "https://developers.google.com/protocol-buffers",
version = "3.13.0",
sha256 = "465fd9367992a9b9c4fba34a549773735da200903678b81b25f367982e8df376",
version = "3.14.0",
sha256 = "6dd0f6b20094910fbb7f1f7908688df01af2d4f6c5c21331b9f636048674aebf",
strip_prefix = "protobuf-{version}",
urls = ["https://github.com/protocolbuffers/protobuf/releases/download/v{version}/protobuf-all-{version}.tar.gz"],
use_category = ["dataplane_core", "controlplane"],
release_date = "2020-08-14",
release_date = "2020-11-13",
cpe = "cpe:2.3:a:google:protobuf:*",
),
grpc_httpjson_transcoding = dict(
Expand All @@ -520,11 +520,11 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "Go rules for Bazel",
project_desc = "Bazel rules for the Go language",
project_url = "https://github.com/bazelbuild/rules_go",
version = "0.23.7",
sha256 = "0310e837aed522875791750de44408ec91046c630374990edd51827cb169f616",
version = "0.25.0",
sha256 = "6f111c57fd50baf5b8ee9d63024874dd2a014b069426156c55adbf6d3d22cb7b",
urls = ["https://github.com/bazelbuild/rules_go/releases/download/v{version}/rules_go-v{version}.tar.gz"],
use_category = ["build", "api"],
release_date = "2020-08-06",
release_date = "2020-12-02",
implied_untracked_deps = [
"com_github_golang_protobuf",
"io_bazel_rules_nogo",
Expand Down
13 changes: 10 additions & 3 deletions generated_api_shadow/bazel/repository_locations.bzl
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion test/common/access_log/access_log_impl_test.cc
View file
Open in desktop
Expand Up @@ -1132,7 +1132,7 @@ name: accesslog
)EOF";

EXPECT_THROW_WITH_REGEX(AccessLogFactory::fromProto(parseAccessLogFromV3Yaml(yaml), context_),
EnvoyException, ".*\"NOT_A_VALID_CODE\" for type TYPE_ENUM.*");
EnvoyException, ".*\"NOT_A_VALID_CODE\" for type ");
}

TEST_F(AccessLogImplTest, GrpcStatusFilterBlock) {
Expand Down
2 changes: 1 addition & 1 deletion test/common/upstream/cluster_manager_impl_test.cc
View file
Open in desktop
Expand Up @@ -272,7 +272,7 @@ TEST_F(ClusterManagerImplTest, UnknownClusterType) {
)EOF";

EXPECT_THROW_WITH_REGEX(create(parseBootstrapFromV3Json(json)), EnvoyException,
"invalid value \"foo\" for type TYPE_ENUM");
"invalid value \"foo\" for type ");
}

TEST_F(ClusterManagerImplTest, LocalClusterNotDefined) {
Expand Down
3 changes: 2 additions & 1 deletion test/common/upstream/upstream_impl_test.cc
View file
Open in desktop
Expand Up @@ -1929,7 +1929,8 @@ TEST_F(StaticClusterImplTest, UnsupportedLBType) {
EnvoyException,
"Protobuf message (type envoy.config.cluster.v3.Cluster reason "
"INVALID_ARGUMENT:(lb_policy): invalid "
"value \"fakelbtype\" for type TYPE_ENUM) has unknown fields");
"value \"fakelbtype\" for type type.googleapis.com/envoy.config.cluster.v3.Cluster.LbPolicy) "
htuch marked this conversation as resolved.
Show resolved Hide resolved
"has unknown fields");
}

TEST_F(StaticClusterImplTest, MalformedHostIP) {
Expand Down