-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Allow protobuf v5 as dependency #8627
Conversation
.github/actions/make_init/action.yml
Outdated
@@ -50,7 +50,7 @@ runs: | |||
# Actions moves to a newer version of Ubunutu. | |||
- name: Add vendored `protoc` to $PATH | |||
run: | | |||
echo "./vendor/protoc-3.20.3-linux-x86_64/bin" >> $GITHUB_PATH | |||
echo "./vendor/protoc-26.1-linux-x86_64/bin" >> $GITHUB_PATH |
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.
Was the decision to include protoc
in our repo made to make the GitHub CI more stable and don't depend on too many external factors? I think that might make sense although we have to install a lot of external dependencies, so I am curious
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 believe the decision to vendor it in was made because the apt-get
version you can install on the ubuntu version used by our CI is quite old (see the comment above). I double checked this and the CI would install a protoc < 3.20 version. I think the alternative would be to download the protoc version from a remote location as part of our CI workflow.
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 have recently updated our docs and included the commands used to install protoc on ubuntu, see the "On Linux (ARM) section":
curl -OL https://github.com/protocolbuffers/protobuf/releases/download/v3.20.0/protoc-3.20.0-linux-aarch_64.zip
sudo unzip -o protoc-3.20.0-linux-aarch_64.zip -d /usr/local bin/protoc
sudo unzip -o protoc-3.20.0-linux-aarch_64.zip -d /usr/local 'include/*'
# (optional) remove old version
rm /usr/bin/protoc
ln -s /usr/local/bin/protoc /usr/bin/protoc
# Print out your System's Protoc version
protoc --version
I think for non-ARM Linux, the aarch_64
would need to be replaced with x86_64
, but then it should work. It might be nicer than vendor it in our own repo as updating in the future would be just a single line (if we extract the version to a variable). What do you think?
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.
Sounds good, I will try to use this to install the protoc version
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.
@raethlein, I updated it to install it from the GitHub release page 👍
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.
hmhm good question, if everything works then probably not needed 🙂 I just checked real quick the official installation instructions but did not find anything, so probably a legacy artifact
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.
The includes where needed. Something did fail in the CI so I added it back again
@raethlein ready for a second review iteration. |
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!
Describe your changes
This PR updates the pin in
setup.py
to allow protobuf 5.x as dependency. It also removes the vendored protoc dependency and installs 26.1 from the official Github release page.Related to #8624 and #8701
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.