Allow protobuf v5 as dependency #8627
Conversation
LukasMasuch
added
the
dev:upgrade-dependencies
LukasMasuch
changed the title
LukasMasuch
changed the title
.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.
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.
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.
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 --versionI 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.
Sounds good, I will try to use this to install the protoc version
There was a problem hiding this comment.
@raethlein, I updated it to install it from the GitHub release page 👍
There was a problem hiding this comment.
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.
The includes where needed. Something did fail in the CI so I added it back again
LukasMasuch
changed the title
|
@raethlein ready for a second review iteration. |
Describe your changes
This PR updates the pin in
setup.pyto 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.