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

Remove generated go bindings #177

Open
bergsieker opened this issue Oct 13, 2020 · 11 comments
Open

Remove generated go bindings #177

bergsieker opened this issue Oct 13, 2020 · 11 comments

Comments

@bergsieker
Copy link
Collaborator

Many of the open PRs and issues on this repository have to do with the go bindings. This problem seems generally intractable in an open source environment: we could probably patch up the current issues and add CI to help us keep on top of things, but that only really allows us to support a single version of Go. What about C# or other languages? What about different versions of Go?

We should deprecate the existing bindings and remove them once clients have had a chance to migrate. If we don't want to go all the way to requiring clients to generate their own bindings, one possibility would be to move the bindings to the remote-apis-sdks repo. Not sure if that's viable or not, but it seems like a better fit given that that repo is already an SDK...

@bergsieker
Copy link
Collaborator Author

@ola-rozenfeld What are your thoughts on moving these bindings to remote-apis-sdks?

@ola-rozenfeld
Copy link
Contributor

Wait, so you want to force everyone using these protos with Go to use the SDK, right? This might be de-facto doable (the SDK should be good enough by now), but I'm not sure it's a good idea in general. I'm wondering, how do other OSS APIs solve this problem? Do they all provide Go bindings?

@sstriker
Copy link
Collaborator

@ola-rozenfeld I prefer to think about this repo as where we document the Remote APIs. We happen to express these in the form of a proto definition, with supporting documentation in markdown files. For implementations of the APIs I don't think it is unreasonable to have to translate from the specification to insert-favorite-programming-language. And that translation may happen via tools, or manually, as long as the end result follows the specification. As such, I think the SDK repo is an excellent place to put language specific artifacts. Make sense?

@EdSchouten
Copy link
Collaborator

The googleapis repo also doesn’t provide generated source files, and we rely on it for bytestream and longrunning. With that in mind, I don’t think it makes sense for us to provide generated sources on our end.

@ola-rozenfeld
Copy link
Contributor

Okay, then I don't mind if we follow suit and move all language-specific stuff to the clients (maybe only Go to start?).
This will require to migrate the existing Go clients first, and then remove the bindings -- right?

@mostynb
Copy link
Contributor

mostynb commented Oct 14, 2020

I wonder if we could solve #105 at the same time?

@bergsieker
Copy link
Collaborator Author

Mostyn: yes, I believe this will fix that issue as well (by pushing it back to the consuming repo). At the monthly sync we determined that a large chunk of the open issues and PRs on this repo have to do with go bindings, which it doesn't really make sense to include in this repo.

tdyas pushed a commit to toolchainlabs/remote-api-tools that referenced this issue Nov 18, 2020
This repository relied on the generated Go files for the remote execution protos that were in [remote-apis repository](https://github.com/bazelbuild/remote-apis). As per bazelbuild/remote-apis#177, that repo is dropping support for the generated Go files, so vendor the protos and generate the code in the repo.

The Google "well-known protos" are from the [googleapis repository](https://github.com/googleapis/googleapis).
@sstriker
Copy link
Collaborator

It seems #220 went in the opposite direction?

@hunshcn
Copy link
Contributor

hunshcn commented Jun 20, 2024

What is the current state? pb.go has not been updated for a long time.

We need to make some changes, delete or update it.

@EdSchouten
Copy link
Collaborator

My recommendation would actually to keep it. Or alternatively, provide another repo where we serve pre-generated copies of this file. Otherwise there is no uniform URL under which this protocol can be imported.

@bergsieker
Copy link
Collaborator Author

Yeah, I think we're basically stuck with these generated bindings for now, and can reevaluate for v3. I'd still prefer it if we didn't have them here, but there's not AFAIK a workable alternative.

What I would like to see is better guidance, here or globally, on how to review PRs with generated code. I don't like rubber-stamping PRs, but I also don't like reviewing 1000 lines of generated boilerplate code.

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

No branches or pull requests

6 participants