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

connectrpc: generate code with buf #4540

Merged
merged 4 commits into from
Sep 24, 2024
Merged

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Jun 5, 2024

Generate the additional code required for connectrpc with the buf tool. Currently, there is no bazel ruleset available for buf generation. However, we can leverage buf as a protoc plugin as shown in https://github.com/abitofhelp/connect-go-example.
With the help of some convenience macro, source code is copied with our regular write_all_source_files Make target.

This PR is broken out of the
https://github.com/scionproto/scion/tree/connectrpc branch to make incremental review easier.

Contributes to #4434

@oncilla oncilla requested review from matzf and jiceatscion June 5, 2024 21:22
@matzf
Copy link
Contributor

matzf commented Jun 5, 2024

This change is Reviewable

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, there is no bazel ruleset available for buf.

Couldn't we use the connect plugin for protoc directly of using buf? I saw some mentions of plugins in the documentation of the bazel protobuf rules that we use (rules_proto_grpc). Does this not work, too complicated, or other reasons to prefer buf?

While this could be a shell script, this is more readable and maintainable.

I know this is very much opinion based, but I personally do not find this go program is more readable than an equivalent shell script. It only does things that shell excells at (massaging paths, invoking a binary).

Reviewed 27 of 28 files at r1, all commit messages.
Reviewable status: 27 of 28 files reviewed, 7 unresolved discussions (waiting on @jiceatscion and @oncilla)


Makefile line 84 at r1 (raw file):

	chmod 0644 pkg/proto/*/*.pb.go

	bazel run //tools/buf

Clean files first to make sure no stale files are left?
rm -f pkg/proto/*/v1/*connect/*.connect.go


pkg/proto/control_plane/v1/control_planeconnect/cppki.connect.go line 39 at r1 (raw file):

const (
	// TrustMaterialServiceName is the fully-qualified name of the TrustMaterialService service.
	TrustMaterialServiceName = "proto.control_plane.v1.TrustMaterialService"

We talked about "fixing" this namespace also in the same go, as we can use a different "package" in connect than in grpc.
Do you think this could be addressed already in this code generation step, or should we do this later?


private/app/xbazel/xbazel.go line 27 at r1 (raw file):

)

const binDir = "bazel-out/k8-fastbuild/bin/"

Nit: Possibly unreliable. The k8 is not fixed, it's the value of the --cpu flag; fastbuild is the --compilation_mode.
But see comment below in resolveFile, I believe binDir is not necessary.


private/app/xbazel/xbazel.go line 65 at r1 (raw file):

	// XXX: Strip the bin directory from the file path. This is required for
	// some go binaries.
	if strings.HasPrefix(filepath.ToSlash(file), binDir) {

Not entirely sure, but this workaround can be removed if we use $(rootpath ...) instead of (location @com_connectrpc_connect//cmd/protoc-gen-connect-go). rootpath should result in a path relative to runfiles.


private/app/xbazel/xbazel.go line 76 at r1 (raw file):

	}

	abs, err := filepath.Abs(file)

Isn't this a nop after filepath.Join(runfilesDir, file)? Or maybe filepath.Clean?


tools/buf/BUILD.bazel line 16 at r1 (raw file):

go_binary(
    name = "buf",

Somewhat of a nitpick; this script here always invokes buf generate with specific flags; it does not allow passing any flags or other parameters to buf. I think this should be reflected in the name of the binary and the bazel target, e.g. call it buf-generate-connect-go or so.


tools/buf/main.go line 50 at r1 (raw file):

	for _, file := range must.Get(filepath.Glob("bazel-*")) {
		args = append(args, "--exclude-path", file)
	}

On my machine, this still seems to get hung up in some symlink trap. It works fine when I change this to pass --path, proto/ instead of the --exclude-paths.

Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 27 of 28 files reviewed, 8 unresolved discussions (waiting on @matzf and @oncilla)


tools/buf/main.go line 14 at r1 (raw file):

// See the License for the specific language governing permissions and
// limitations under the License.

I really don't see the benefit of having this go program just to invoke one executable with a trivial template. As it is, that's 4 extra moving parts just to run a simple command.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 27 of 28 files reviewed, 8 unresolved discussions (waiting on @jiceatscion and @matzf)


tools/buf/main.go line 14 at r1 (raw file):

Previously, jiceatscion wrote…

I really don't see the benefit of having this go program just to invoke one executable with a trivial template. As it is, that's 4 extra moving parts just to run a simple command.

Every single bash script I have written for bazel rules/macros I have regretted after some time.
This is my personal bias of course.

If you feel really strongly about it, we can drop it.
If you do not feel strongly, I would really prefer to keep it anyhing other than bash.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 27 of 28 files reviewed, 9 unresolved discussions (waiting on @jiceatscion and @matzf)

a discussion (no related file):
Reminder to self: https://github.com/abitofhelp/connect-go-example


Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 28 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @matzf and @oncilla)


tools/buf/main.go line 14 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Every single bash script I have written for bazel rules/macros I have regretted after some time.
This is my personal bias of course.

If you feel really strongly about it, we can drop it.
If you do not feel strongly, I would really prefer to keep it anyhing other than bash.

I do not feel strongly enough about it to block this PR.

Generate the additional code required for connectrpc with the buf
tool. Currently, there is no bazel ruleset available for buf. We work
around this by invoking the buf binary directly and commiting the
generated code in git. To deal with generation, we pin the
`protoc-gen-connect-go` version in the go.mod file. A small scaffolding
go binary is added to deal with setting up the environment correctly
for the buf tool. While this could be a shell script, this is more
readable and maintainable.

This PR is broken out of the
https://github.com/scionproto/scion/tree/connectrpc branch to make
incremental review easier.

TL;DR: Run `bazel run //tools/buf` to generate the connectrpc code.
Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @matzf from 7 discussions.
Reviewable status: 3 of 33 files reviewed, all discussions resolved (waiting on @jiceatscion and @matzf)


tools/buf/main.go line 14 at r1 (raw file):

Previously, jiceatscion wrote…

I do not feel strongly enough about it to block this PR.

Gone now anyway

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 28 files at r1, 30 of 30 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

@oncilla oncilla merged commit 355515d into scionproto:master Sep 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants