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.

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

Update protobuf version #2239

Merged
merged 36 commits into from Oct 10, 2019
Merged

Conversation

zaqqwerty
Copy link
Contributor

Resolves #2238

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Sep 29, 2019
@maffoo
Copy link
Contributor

maffoo commented Sep 30, 2019

We should also update the protobuf version in requirements.txt to match what's in the bazel build. Also note that we have a script dev_tools/build-protos.sh to build protos that uses the grpcio-tools python library, which bundles protoc. I think this is typically what we've been running to regenerate protos and check them in. We have the version of that library pinned in the devtools requirements, though it's not immediately clear what version of protoc that bundles. It looks like we never set up CI jobs to ensure that the checked-in generated code is up to date, but we should set that up (also need to update docs to describe how we're handling protos, because there's potential confusion about whether to use check/bazel or dev_tools/build-protos.sh.

@zaqqwerty
Copy link
Contributor Author

zaqqwerty commented Oct 1, 2019

Thank you for the comments. I had indeed used dev_tools/build-protos.sh to regenerate the protos. I find that the grpcio-tools version in the devtools requirements, 1.21.0, points at protobuf version 3.7.0 (third party tools -> protobuf submodule). I updated requirements accordingly.

I find that the bazel machinery in master is actually not functional, I am working on remedying this.

@Strilanc
Copy link
Contributor

Strilanc commented Oct 3, 2019

I think we used to check that the bazel build worked, but then it kept breaking for reasons out of our control (e.g. the link where the protobuf installer was located stopped working) so I removed the check. If you can get it working consistently, that would be awesome.

@zaqqwerty
Copy link
Contributor Author

zaqqwerty commented Oct 7, 2019

Made some further changes to address your comments:

  • Updated the WORKSPACE and BUILD files with the relevant rules for building version 3.8.0 protos
  • Added the script check/build-protos to bazel build both the python and C++ proto rules, and added a stage to the travis build to call the script
  • Updated the grpcio-tools version to 1.24.0, which uses proto version 3.8.0; ran /dev_tools/build-protos.sh and checked in the updated protos
  • Added a section to docs/development describing how to approach these proto tools

@Strilanc
Copy link
Contributor

Strilanc commented Oct 8, 2019

This looks good to me. @maffoo you agree? If so automerge it.

@Strilanc Strilanc added automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Oct 10, 2019
@CirqBot CirqBot merged commit c8714f3 into quantumlib:master Oct 10, 2019
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Oct 10, 2019
@dabacon dabacon mentioned this pull request May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protobuf version compatibility with Bazel
5 participants