Proporal: use Buf CLI to compile our proto files #6784
Conversation
nopcoder
added
improvement
infrastructure
|
♻️ PR Preview 1cc816a has been successfully destroyed since this PR has been closed.
🤖 By surge-preview |
There was a problem hiding this comment.
The change itself is good. I am unsure whether it improves things enough to make it worth our while.
Pros
- Makes generating all protobufs much simpler.
- AFAIR Buf were planning a global protobuf registry, so we should be able to publish our protobuf and make life simpler for those of our users who want to consume it
Cons
- protoc now becomes a dependency of buf. It may take longer to fix security bugs in buf+protoc than it does in protoc, merely in terms of waiting until the new protoc version shows up
- Installation of the dev toolchain is now more complex, particularly on systems that do not support Homebrew
Should we discuss on the issue?
| @@ -44,6 +44,7 @@ Our [Go release workflow](https://github.com/treeverse/lakeFS/blob/master/.githu | |||
| * Apple M1 users can install this from [Azul Zulu Builds for Java JDK](https://www.azul.com/downloads/?package=jdk). Builds for Intel-based Macs are available from [java.com](https://www.java.com/en/download/help/mac_install.html). | |||
| 1. *Optional* - [PostgreSQL 11](https://www.postgresql.org/docs/11/tutorial-install.html) (useful for running and debugging locally) | |||
| 1. *Optional* - [Rust & Cargo](https://www.rust-lang.org/tools/install) (only needed if you want to build the Delta Lake diff plugin) | |||
| 1. *Optional* - [Buf CLI](https://buf.build/docs/installation) (only needed if you like to update Protocol Buffer files) | |||
There was a problem hiding this comment.
It appears there that they produce no Linux packages. So IIUC on Linux we still have to install protoc. Maybe not for Mac/Homebrew. Add those instructions, too?
Also, it is always required to run checks-validator AFAIU.
There was a problem hiding this comment.
There are two parts the cli and the go plugin. The cli installation is curl one-liner, it is a Go project. The cli uses go run to use the plugin.
So we require less from the OS in terms of packages and depds.
About the validation - you are right, I'll need to find a better way to run this part or instruct the user in case of a change.
There was a problem hiding this comment.
Updated the code to use go run instead of require the use to install the tool.
It should solve the issue for running checks-validator or worried running under linux or windows.
Makefile
Outdated
| $(PROTOC) --proto_path=pkg/kv --go_out=pkg/kv --go_opt=paths=source_relative secondary_index.proto | ||
| $(PROTOC) --proto_path=pkg/kv/kvtest --go_out=pkg/kv/kvtest --go_opt=paths=source_relative test_model.proto | ||
| gen-proto: ## Build Protocol Buffers (proto) files using Buf CLI | ||
| buf generate |
There was a problem hiding this comment.
So now users who want to develop lakeFS need to install the buf CLI on their env?
Previously we used a docker so no added installation was needed.
I think this needs to be reflected in our contributing guide (it seems that this step is optional)
There was a problem hiding this comment.
We commit the source code, so usually contribute will not require to build proto.
Also, protoc doesn't have a formal docker image and we build it and need to maintain it.
I've added a note to the contrib guide to install the cli the case proto files require an update.
There was a problem hiding this comment.
@N-o-Z updated the PR - used go run to run buf cli, so now it is no diff from running lint or other Go dependencies.
nopcoder
force-pushed
the
improve/protoc-to-buf
branch
from
6a3dc8d to
1cc816a
Compare
| @@ -33,6 +33,8 @@ jobs: | |||
| go-version: "1.20.6" | |||
| id: go | |||
|
|
|||
| - uses: bufbuild/buf-setup-action@v1.27.1 | |||
There was a problem hiding this comment.
Slightly odd that we install it differently on the GitHub workflow and on a local dev box. Price of using GH actions, I guess.
This PR introduces changes to our Protocol Buffers compilation process.
Replaced the usage of
protocand Makefile with theBufCLI.Close #6783