rules_go icon indicating copy to clipboard operation
rules_go copied to clipboard

remove dependency on go-genproto

Open codyoss opened this issue 4 years ago • 9 comments

What version of rules_go are you using?

v0.26.0

What version of gazelle are you using?

v0.23.0

What version of Bazel are you using?

4.0.0

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

Linux / MacOS, AMD64.

What did you do?

Used rules_go with cloud.google.com/go.

What did you expect to see?

A build that compiled.

What did you see instead?

The build failed due to rules_go declaring an older version of go-genproto. It is possible to get everything working by using the override instructions but that is not super intuitive. Unless there is a need for the genproto to be bundled I suggest removing the dependency on it so users of rules_go can more easily use the latest version of cloud.google.com/go without the need for overriding dependencies.

codyoss avatar Mar 12 '21 15:03 codyoss

Sorry for the slow response; I don't have much time to work on Bazel these days, but I'm trying to get through the backlog.

I think I agree with this. I see @noahdietz gave a thumbs up, so that gives more confidence this is a good idea.

I don't think rules_go depends on anything in go-genproto, so it probably doesn't need to be part of go_rules_dependencies. It probably doesn't help to have an infrequently updated version in there either.

I'll mark this for the next minor release. Not sure when that will be yet.

jayconrod avatar May 14 '21 19:05 jayconrod

Thank you @jayconrod! Let us know if we can help somehow, with testing or whatever.

noahdietz avatar May 14 '21 19:05 noahdietz

Hey there folks (cc @achew22) so we over at google-cloud-go/go-genproto are going to be moving much (but not all) of the protobuf/gRPC generated code from go-genproto into google-cloud-go. We will be doing this in a backwards compatible way, first generating/releasing the types in google-cloud-go, then type-aliasing the "old" types in go-genproto to the "new" types defined in google-cloud-go, and stopping generation of those types in go-genproto.

In order to reduce the potential friction on rules_go users utilizing the go-genproto dependency that rules_go supplies, I think we should go through with the removal described in this issue. We had some agreement based on the comments, it seems.

I can provide PRs for this if desired, but want the support of the current maintainers as well as an idea of when y'all would want to get this into a release.

noahdietz avatar Aug 03 '22 20:08 noahdietz

@noahdietz thanks for following up on this! Do you have any PRs or even docs that stand as an example of what the end state would look like? It would be helpful for understanding what to expect.

achew22 avatar Aug 03 '22 22:08 achew22

Do you have any PRs or even docs that stand as an example of what the end state would look like?

I haven't made a PR for this yet, no. For this repo it will mean the removal of this dependency, associated patch files, and various bits of documentation (see relevant item in table, see mention as a gRPC dependency).

For end-users, it would likely involve needing to add a go_repository dependency on google.golang.org/genproto to their own project if they didn't already have one and were depending on rules_go's distribution of it. I acknowledge (and I think other commenters on the thread) that this means users potentially have to change their own projects to update to whatever version of rules_go this change goes out in. I'd say that rules_go goes just a tad too far in "helping the user" by providing all of these dependencies sometimes - my understanding of bazel best practice is to declare all versions of the dependencies that a project needs in your WORKSPACE (or in a macro in something like a repositories.bzl file). As such, I don't think it's unfair of rules_go to stop providing a dependency if it doesn't depend on it for its own work. I don't mean to come off as argumentative, but rather trying to assert a perspective/best practice that might help us reason about this change. I don't personally like adding friction for users.

noahdietz avatar Aug 03 '22 23:08 noahdietz

One of the holy grail issues of this project is getting the chance to delete these lines of code right here:

https://github.com/bazelbuild/bazel-gazelle/blob/f46349dc1444316605a8cee1a6a8805f161535e2/language/proto/known_go_imports.go?q=googleapis#L21-L423

Those exist because in ye olden days of rules_go there were a lot of difficulties around getting go_proto_library. I don't remember enough to be able to say anything articulate, but it was a pretty good solution at the time.

If someone were to add a go_repository dep on google.golang.org/genproto directly right now and rewrite all of their deps on targets from there, do you think it would work? If that's the case then we can "start the migration" now by removing those rewrite rules from the go plugin of Gazelle, and then we should ship a few Gazelle's without it included to get people migrated over. Then we could remove everything in rules_go related to this and I think that would solve the matter.

@linzhp / @fmeum can you check my math on this one before I get too excited?

achew22 avatar Aug 05 '22 05:08 achew22

That sounds correct. We may have to add the repository under both the old and the new name for the transitionary period.

fmeum avatar Aug 05 '22 06:08 fmeum

Let me first say: I'm not all that experienced in the inner workings of this project, nor gazelle, I've only looked through the plumbing in the context of these few issues relating to googleapis<->rules_go<->go-genproto. That said...

go_googleapis is actually the rules_go (re)distribution of com_google_googleapis (github.com/googleapis/googleapis) with all of the BUILD.bazel files in googleapis removed and replaced with rules_go's patches. Then, with the aliases you linked to, gazelle knows to remap google.golang.org/genproto dependencies to the go_proto_library targets provided by rules_go's go_googleapis distribution. Those aliases aren't actually using the google.golang.org/genproto module/source code, but rather the code generated by the go_googleapis go_proto_library targets that rules_go patches in.

All this to say - I think what you've mentioned @achew22 is actually more part of #1986, based on Jay's comment.

The issue I've opened here is to just remove the distribution of google.golang.org/genproto that rules_go provides, but that it does not actually use itself in its own implementation. As I see it, rules_go bringing in google.golang.org/genproto is a convenience thing for users so that they do not need to explicitly import google.golang.org.genproto themselves.

Does that make sense / seem correct?

noahdietz avatar Aug 05 '22 15:08 noahdietz

I was trying to fix this as part of #3603, but I decided not to do it for two reasons:

  1. During the transition period, there are alias under //proto/wkt pointing to go-genproto. Even after https://github.com/bazelbuild/bazel-gazelle/pull/1567 and #3613, we still need to keep //proto/wkt a while to ease transition
  2. gogo proto compilers still use the go-genproto: https://github.com/bazelbuild/rules_go/blob/488384fe68bb197cc56f38f5a4074cee7267c8bb/proto/BUILD.bazel#L75

linzhp avatar Jul 02 '23 16:07 linzhp