-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove bazel from repo #4297
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
Remove bazel from repo #4297
Conversation
bazel build "${base}_py_proto"; \ | ||
bazel build "${base}_cc_proto") | ||
|
||
python -m grpc_tools.protoc -I=cirq-google --python_out=cirq-google --mypy_out=cirq-google ${base}.proto |
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 it is useful to have a job that checks for whether the checked-in pb2 files are up to date with the protos or not. Thus, we should keep the grpc_tools generation part. Also, this might be a good point to clean up the duplication between this job and dev_tools/build-protos.sh.
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.
Thanks for picking this up! I'd keep a simplified version of the build-changed-protos job.
How do you propose we keep an alternate version of the Are you thinking we add some kind of test that actually builds new pb2 files from the .proto files on the repo and do some functionality tests there ? |
This script did test whether the proto definitions are consistent with the pb2 files or not, based on the assumption that previously they were. If that assumption is true, and the proto files change, the pb2 files might change. In that case the developer should commit the new version of the pb2 file. If they forgot to do that, these lines check whether there is a change simply using git after rebuilding the pb2 files using grpcio: |
Ok, so do we want to just keep these "change checker" style of tests from the |
Yes! |
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.
LGTM
Automerge cancelled: A required status check is not present. Missing statuses: ['Changed files test', 'Format check'] |
Fixes #4280