Do not require bazel for check_grpcio_tools.py#13463
Merged
mehrdada merged 1 commit intogrpc:masterfrom Nov 20, 2017
mehrdada:fix-check_grpcio_tools
Merged
Do not require bazel for check_grpcio_tools.py#13463mehrdada merged 1 commit intogrpc:masterfrom mehrdada:fix-check_grpcio_tools
mehrdada merged 1 commit intogrpc:masterfrom
mehrdada:fix-check_grpcio_tools
Conversation
|
|
|
1 similar comment
|
kpayson64
suggested changes
Nov 20, 2017
Contributor
kpayson64
left a comment
kpayson64
left a comment
There was a problem hiding this comment.
Nice Fix! Just one suggestion.
Contributor
There was a problem hiding this comment.
Given that this is a python file, does it make sense to just have the commit hash be a global variable, and then you can import this file and check it
Contributor
Author
There was a problem hiding this comment.
I tried doing that first, but the challenge was importing the file with its path being read from a variable, and it seemed different things worked differently in various python versions and the existing test relied on opening the raw file content for reading. This approach sounded like more robust across python versions. If we want to do that, we should probably go about hardcoding the module path.
Contributor
There was a problem hiding this comment.
I see. If we need to add a hack to have it imported, I'm fine with it as is.
Contributor
Author
There was a problem hiding this comment.
PTAL. I made it a variable but verify its presence as a raw string, not by parsing Python.
kpayson64
approved these changes
Nov 20, 2017
|
1 similar comment
|
|
The `make_grpcio_tools.py` rube-goldberg machinery relies on `bazel` to extract the list of files required to compile the `grpcio-tools` package that are provided by the `protobuf` submodule. In order to ensure that list is up to date, `check_grpcio_tools.py` sanity check does the same `bazel` query, and checks the full contents against the already existing list in the repository. This has the downside of requiring `bazel` to run that particular check at sanity test time, and flakiness has been seen there. This commit changes the code generation process to include the git hash of the submodule at the time `make_grpcio_tools.py` is invoked and stores it in the generated file and instead of doing the whole process at test time, the `check_grpcio_tools.py` sanity test simply checks the submodule version at test time and verifies it against the version included in the file by `make_grpcio_tools.py` thus removing the `bazel` dependency at test time and increasing test robustness and speed.
|
|
|
|
1 similar comment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Fixes #13404.
The
make_grpcio_tools.pyrube-goldberg machinery relies onbazelto extract the list of files required to compile thegrpcio-toolspackage that are provided by theprotobufsubmodule. In order to ensure that list is up to date,
check_grpcio_tools.pysanity check does the same bazelquery, and checks the full contents against the already
existing list in the repository. This has the downside of
requiring
bazelto run that particular check at sanity testtime, and flakiness has been seen there.
This commit changes the code generation process to include
the git hash of the submodule at the time
make_grpcio_tools.pyis invoked and stores it in the generated file and instead of
doing the whole process at test time, the
check_grpcio_tools.pysanity test simply checks the submodule version at test time
and verifies it against the version included in the file by
make_grpcio_tools.pythus removing the bazel dependencyat test time and increasing test robustness and speed.