Migrate from go_googleapis to com_google_googleapis

[vam-google]

rules_go depends on googleapis repository by applying gazelle-generated patch. googleapis is now Bazel-aware and rules_go should depend on googleapis directly without overwriting its BUILD.bazel files. Also the repository name used within rules_go should be migrated from go_googleapis to com_google_googleapis.

This is expected to be done in few phases (with deprecation period, when both go_googleapis and com_google_googleaspis are available).

Without this change there is a circular dependency between googleapis and rules_go (because googleapis uses rules_go to build go clients).

Currently, the build of googleapis produces the following warnings because of this issue:

GoLink: warning: package "google.golang.org/genproto/googleapis/api/annotations" is provided by more than one rule:
    @go_googleapis//google/api:annotations_go_proto
    //google/api:annotations_go_proto
Set "importmap" to different paths in each library.
This will be an error in the future.

Using importmap should work as a short term solution, but proper migration to com_google_googleapis would eliminate these warnings automaticaly.

[ashi009]

Is anyone working on this? Looking forward to having a language-agnostic repo to use.

[ashi009]

🤷‍♂️ It turns out that go_googleapis disagrees with the proto_library defined in com_google_googleapis.

https://github.com/bazelbuild/rules_go/blob/ef7cca8857f2f3a905b86c264737d0043c6a766e/third_party/go_googleapis-deletebuild.patch#L11-L18

https://github.com/bazelbuild/rules_go/blob/ef7cca8857f2f3a905b86c264737d0043c6a766e/third_party/go_googleapis-gazelle.patch#L512-L520

[vam-google]

@ashi009 Yes, those should converge (go_googleapis and com_google_googleapis), this is what this issue is tracking. Unfortunately we did not have enough resources to do the proper migration yet, but it is still the plan.

Any help on this will be greatly appreciated =)

[gonzojive]

ping? not sure what's required to implement this... maybe somebody can jot down thoughts that might help the ambitious contributor.

[jayconrod]

I don't believe anyone is working on this at the moment. This should eventually done by either me or someone who maintains com_google_googleapis. I'd rather not take PRs from anyone else.

I had a conversation with @noahdietz (one of the googleapis maintainers) a couple weeks ago. To summarize what needs to be done:

• In the go_googleapis patches, go_proto_library targets should be replaced with alias targets pointing to com_google_googleapis equivalents. That should allow existing projects to migrate incrementally.
• Gazelle should resolve Go and proto imports to com_google_googleapis instead of go_googleapis.
• After a while, go_googleapis should be removed.

[mmellin]

Hi there. I'm currently trying to upgrade a fairly sizable Bazel monorepo from Go 1.13 to Go 1.16+ and rules_go from 0.24.0 to the latest and run into many issues, including now this one.

• What is the status of this issue and is there work planned to fix it?
• Is there an older combination of rules_go, protobuf, etc to work around this issue that still support Go 1.16+?
• Is there a concrete example of using the importmap as a workaround?

Currently we are not using gazelle to generate proto file, only BUILD. We're using rules_proto proto_library and rules_go go_proto_library with a variety of compilers :

 compilers = [
        "@io_bazel_rules_go//proto:go_grpc",
        "@com_github_grpc_ecosystem_grpc_gateway//protoc-gen-grpc-gateway:go_gen_grpc_gateway",
    ],

I've tried to fix some of this by using rules_proto_grpc grpc_gateway_library() but ran into this linker issue with googleapis. I'm stuck at what to do next. Yes, I have read through the avoiding conflicts section, but we have so many targets and deps that its not straightforward and I'm not a proto/grpc/gazelle expert. We certaintly don't pregenerate .pb.go files, this is what we use rules_go for.

Thanks.

[robfig]

There has been no work planned on our end, and Jay is on sabbatical for a while. If @noahdietz has the bandwidth, I'd be happy to review and contribute in a secondary capacity.

Workarounds to use the latest Go / rules_go / gazelle are almost certainly possible, although I'm afraid that I don't really know how to help you. I'd be happy to share the combination of versions that we're using -- we have gRPC-based services using gogo, although we do not get too deep into the ecosystem. For example, we do not use grpc gateway. We are on Go 1.16 and use the go_googleapis provided by go_rules_dependencies. We do not use gazelle to generate our proto rules; we use a macro with a lot of customizations; reviewing it now I see some significant workarounds to accommodate gogo, and there appears to be conflict between gogo and the use of grpc server reflection.

I'm happy to share a snippet from our rules, although it should include the caveat that this isn't an official recommendation or anything, I'm not confident that there aren't simpler ways to accomplish it, and I'm not even sure that it's helpful for your scenario.

With those caveats, the relevant portions expand to something like this:

def corp_protos(name, srcs = [], deps = [], grpc = False, verbose = 0, **kwargs):
    """Defines protobuf targets for all supported languages.

    Args:
        name: The name of the protobuf library; see notes on naming
        srcs: The protobuf files
        deps: Any proto_library dependencies
        grpc: Whether or not to generate gRPC libraries
        importpath: The Go import path for this library (override)
        **kwargs: Arguments to apply to all of the created targets

    Naming:
        The Bazel-recommended naming convention for a protobuf library that builds something.proto
        is 'something_proto' for the base protobuf compilation and 'something_LANG_proto' for
        language-specific libraries (e.g., 'something_java_proto'). This macro adheres to this
        convention as long as the name given to the macro (which is the name of the proto_library)
        ends with '_proto'.

    Example:
        corp_protos(
            name = 'foo_proto',
            srcs = ["foo.proto"],
            deps = [":bar_proto"],
        )

        This will generate the following targets:
        - proto_library(name = "foo_proto", ...)
        - proto_library(name = "foo_proto.withgogoimport", ...)
        - java_library(name = "foo_java_proto", ...)
        - go_proto_library(name = "foo_go_proto", ...) (when grpc=False)
          or
          go_library(name = "foo_go_proto", ...) (when grpc=True)
        - closure_proto_library(name = "foo_js_proto", ...)

        The :foo_proto target may be used as a dependency to other proto libraries, while the
        :foo_LANG_proto targets may be used as dependencies to libraries of that language.
    """

    package_name = native.package_name()

    visibility = kwargs.pop("visibility", None)
    importpath = kwargs.pop("importpath", "alpha/" + package_name)

    proto_library(
        name = name,
        srcs = srcs,
        deps = deps,
        visibility = visibility,
        **kwargs
    )

    # This guard exists since the genrule below relies on it.
    if any([not s.endswith(".proto") for s in srcs]):
        fail("All srcs to corp_protos must be .proto files")

    gogo_protobuf_name = name + "_gogo_protobuf"

    # Support cases where corp_protos is used to group a bunch of other *_proto
    # targets together; in those cases, srcs is empty, but deps is not.
    if not (srcs or deps):
        fail("At least one of srcs or deps must be on-empty.")

    # Having a genrule without outputs (which would be the case if srcs is empty)
    # is not allowed.
    if srcs:
        gogo_proto_outs = ["withgogoimport/{}/{}".format(package_name, s) for s in srcs]
        gogo_cmd_args = " ".join([
            '"$(location {src})=$(location {out})"'.format(src = s[0], out = s[1])
            for s in zip(srcs, gogo_proto_outs)
        ])
        native.genrule(
            name = gogo_protobuf_name,
            srcs = srcs,
            outs = gogo_proto_outs,
            cmd = "$(location @corp//tools/protobuf:append_gogoimport) " + gogo_cmd_args,
            tools = ["@corp//tools/protobuf:append_gogoimport"],
            visibility = ["//visibility:private"],
        )

    gogoimport_deps = [
        "@corp//thirdparty/protos/gogo/protobuf/gogoproto:gogoproto_proto",
    ] + [
        (_target_name(d, "_proto.withgogoimport") if d.endswith("_proto") and not is_wkt_proto(d) else d)
        for d in deps
    ]

    proto_library(
        name = _target_name(name, "_proto.withgogoimport"),
        srcs = [":" + gogo_protobuf_name] if srcs else [],
        deps = gogoimport_deps,
        strip_import_prefix = "withgogoimport/",
        visibility = visibility,
        **kwargs
    )

    _java_protos(name, grpc, visibility, **kwargs)
    _go_protos(name, deps, grpc, importpath, visibility, verbose, **kwargs)
    _js_protos(name, visibility)

def _go_protos(name, proto_deps, grpc, importpath, visibility, verbose, **kwargs):
    go_library_name = _target_name(name, "_go_proto")

    if grpc:
        go_compiler = "@io_bazel_rules_go//proto:gogofast_grpc"
    else:
        go_compiler = "@io_bazel_rules_go//proto:gogofast_proto"

    deps = [
        "@com_github_gogo_protobuf//gogoproto:go_default_library",
        "@com_github_gogo_protobuf//proto:go_default_library",
        "@com_github_gogo_protobuf//types:go_default_library",
        "@com_github_gogo_protobuf//protoc-gen-gogo/descriptor:go_default_library",
        "@com_github_golang_protobuf//proto:go_default_library",
    ]
    deps += [_target_name(d, "_go_proto") for d in proto_deps if not is_wkt_proto(d)]
    if grpc:
        # Wrap the go_proto_library so that we can include the source proto files as data.
        # TODO: Get rid of this when we stop using Gogo, or there's a way for Gogo
        # Protobuf to work with gRPC server reflection
        go_proto_library_name = go_library_name + ".withoutprotosourcefiles"

        go_proto_library(
            name = go_proto_library_name,
            proto = _target_name(name, "_proto.withgogoimport"),
            compilers = [go_compiler],
            deps = deps,
            importpath = importpath,
            visibility = ["//visibility:private"],
            **kwargs
        )

        go_library(
            name = go_library_name,
            data = [name],
            embed = [go_proto_library_name],
            importpath = importpath,
            visibility = visibility,
        )
  else: 
     ...

[noahdietz]

Hey folks, I totally understand the friction/frustration here.

I've brought it to the attention of my team (maintainers of the googleapis repo) again and we will try to prioritize the work at the beginning of the new year.

[mmellin]

Thanks @robfig for the reply. What is the purpose of the go_googleapis-deletebuild.patch and why remove google/api/BUILD.bazel? Looks like many of my current BUILD proto targets in current source use deps from the @com_google_googleapis//google/api: targets which are removed via the patch. Are there recommended replacements, or should I try and not use that patch?

[aiuto]

I'm new to this problem, so feel free to tell me my suggestions are junk.

It looks like go_googleapis-deletebuild.patch is getting rid of the declarations for other languages than Go. My hunch is that you are doing that because you don't want the dependencies on the other language rules. It causes bloat.

Perhaps the best way to solve that is refactor googleapis. The basic idea would
be that for each API, the proto_library is at .../fooservice/v1/BUILD, and each of the languages is in a different place. It might be .../fooservice/v1/python/BUILD, or maybe an entire shadow tree by language - //python/fooservice/v1/BUILD. The goal would be that users can trivially strip out what they do not need to minimize their dependencies.
It might make googleapis CI runs more informative. If someone breaks the Go setup, only that slice fails, rather than the similar rules in every API.

Of course, this causes a slow transition for direct users of googleapis.
You would have to leave a forwarding alias in the original location pointing to the
other rule definition.

[sergiitk]

Friendly bump here. We ended up depending on @go_googleapis in golang, and on @com_google_googleapis (specific version pinned and downloaded by http_archive) in the rest of the languages because of this issue - which effectively results in different versions of googleapis protos being used in go, and other languages.

https://github.com/cncf/xds/pull/31/files#diff-ebfc9ed19ee06c7f9220e98d7a0b1f8ff02a48ece093cc054348a4b49a5a40a8R27-R31

[achew22]

I don't think anyone objects to doing the switch over at this point, I think we are only limited by spare cycles. @sergiitk, it sound like you might have some. Can I clear any roadblocks on the way to you taking this task on?

[sergiitk]

@achew22 thank you for following up. As much as I would want to help, I have zero spare cycles (or even negative 😄). I just wanted to bump this to show there are customers actively affected by the issue.

[achew22]

I'm glad to hear that there are users, but I must push back on the notion that there are customers. At this point in time, everyone who maintains rules_go is a volunteer, helping out on nights and weekends. We have 0 support from Google beyond CI and what we get from your staff as volunteer PRs (which we haven't had since f1019ef in September 2021). I guess what I'm saying is that I'd have a hard time describing this as a product with customers given that it is entirely a for fun effort by a rag-tag band of buccaneers (yaar!).

Given that you're working on gRPC at Google, presumably in Go, maybe this is a place where we could do a little bit of collaborating. How can I help you to surface this gRPC pain-point that is affecting the ecosystem? I imagine that if the gRPC team itself might be interested in ensuring that using gRPC is a good experience for your customers. What can we do as a team to help in this regard?

[achew22]

Oh, I guess that should have been @sergiitk. Sorry for not tagging you in the original comment.

[sergiitk]

Given that you're working on gRPC at Google, presumably in Go

That's the thing, I'm on gRPC Java. I only had to deal with this because of contributing to another, non-Google and non-gRPC project.

That said, I'd be happy to raise this issue with gRPC Go team, and the Envoy team. Can't promise anything, but maybe someone will consider volunteering.

[noahdietz]

@sergiitk The Client Libraries team (which @vam-google and I work on) maintains the com_google_googleapis WORKSPACE. We are a multi-language team and haven't had support to dive into this Go-specific issue (as much as we want to, as it bothers us as well). We'd like to tackle this, but need to get the proposal in front of the right eyes. I will cc you on some communications (and you can include whomever you'd like to or not) to provide your first-hand account/support, but I'm not sure that gRPC/Envoy teams should be responsible for drumming up support. It is our bag, I don't want you to have to hold it, but we (my team) need to commit to getting this done.

[dsphper]

The solution：
like this:

go_repository(
    name = "com_github_google_cel_go",
    build_file_generation = "on",
    build_naming_convention = "go_default_library",
    importpath = "github.com/google/cel-go",
    sum = "h1:MnUpbcMtr/eA8vRTEYSru+fyCAgGUYLrY/49vUvphbI=",
    version = "v0.11.3",
)

It Works!

[linzhp]

bazel-contrib/bazel-gazelle#1561 and #3595 should fix this issue. Please give them a try