Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
deps: update protobuf to 3.14 #14253
deps: update protobuf to 3.14 #14253
Changes from all commits
5313dc7
919e1cd
18691fb
6e8acda
5a51bdc
2fe0b36
c113ef7
494314b
b34523b
0a1acbe
233b6d9
de32d28
7e73763
1b37116
8eab565
722db6d
dc33299
d961c83
f01d9f1
c8ef105
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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 aftergoogle.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 viasetup.py install
?), but othergoogle
packages using native namespaces. (This also gets into what Bazel does in its pip rules....)Anyhow, some fixes might be:
google/__init__.py
from googleapis/google-auth-library-python. (@busunkim96 to comment on that.)pip
, which should install using the native namespace.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing
google/__init__.py
from thegoogle-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 likegoogle-api-core
,google-cloud-core
, andgoogle-auth
have keptgoogle/__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 folkspip install
packages.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-protosThere was a problem hiding this comment.
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 #14329There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.